Fix #1311

[dcooper16]

This PR proposes a fix for #1311. At the moment, it is just a rough proposal, and is not ready to be merged.

The basic idea is that for each protocol for which a cipher order could be enforced but isn't (SSL3, TLS 1, TLS 1.1, TLS 1.2, TLS 1.3), ciphers_by_strength() determines the rating (using get_cipher_quality()) of each supported cipher. If all of the ciphers have the same rating (even if it isn't very good), then the lack of a server-enforced cipher order is considered okay. If not, then the greater the difference between the best and worst cipher, the higher the warning level.

For the "Has server cipher order?" line, the rating is based on the worst rating for each of the individual protocols. At the moment, it is only this overall rating that is sent to fileout().

I would appreciate any thoughts on whether this is the correct overall approach for addressing #1311 as well as any suggestions for the details of how to determine the rating ("INFO", "LOW", "MEDIUM", etc.")

[drwetter]

Thanks, David.

I had a quick look at the results only. For dev.testssl.sh ist seems to be still penalizing the order for TLS 1.2.

For a broken VM with lots of legacy ciphers and no cipher order all seems correct (was also the case before).

I have not done tests against a host which has e.g. 3DES ciphers

[drwetter]

grading/rating: good question, don't know. This is currently for SSLlabs only so we should stick to what they do.

Maybe @magnuslarsen has some insights which he likes to share ;-)

[dcooper16]

For dev.testssl.sh ist seems to be still penalizing the order for TLS 1.2.

Yes. For TLS 1 and TLS 1.1, all ciphers supported by dev.testssl.sh are rated by get_cipher_quality() as 4, so there is no penalty for the server not enforcing an order. (Even if the ciphers are bad, if all of the offered ciphers are equally bad, then enforcing a cipher order doesn't help.)

For TLS 1.2, dev.testssl.sh supports ciphers with ratings of 4, 6, and 7, so there is a penalty for the server not enforcing an order (and hopefully preferring the ciphers with the higher ratings).

grading/rating: good question, don't know. This is currently for SSLlabs only so we should stick to what they do.

SSLlabs just seems to report whether or not the server enforces a cipher order, without indicating whether that is good or bad. For testssl.sh, whether or not the server enforces a cipher order does not affect the grade that is assigned to the server, just the severity level that is assigned in the one call to fileout().

[drwetter]

(Even if the ciphers are bad, if all of the offered ciphers are equally bad, then enforcing a cipher order doesn't help.)

good point

For TLS 1.2, dev.testssl.sh supports ciphers with ratings of 4, 6, and 7, so there is a penalty for the server not enforcing an order (and hopefully preferring the ciphers with the higher ratings).

I maybe need to have a look at what 4,6 and 7 is but realistically the read color doesn't fit spread of ciphers. It's not in line with the cipher categories.

SSLlabs just seems to report whether or not the server enforces a cipher order, without indicating whether that is good or bad.

sigh

[magnuslarsen]

As David said, Qualys mentions nothing about good/bad on cipher order, not in the Server Rating Spec nor their TLS Deployment Best Practices. I did make a quick search through their forums, but I couldn't find anything

[drwetter]

Thanks.

Here I still have a problem with the red color (dev.testssl.sh:)

For TLS 1.1 and TLS 1.0 it's fine as they are all CBC ciphers which are in the same bucket here:

However for TLS 1.2 flagging the cipher order red is a bit too much IMO. And it's inconsistent with the last picture. So yellow would be fine here. "Red" is a relic from early times where testssl.sh didn't look at the ciphers at all. So it was assuming the worst, like RC4. And often during the bad old times that wasn't far off.

[drwetter]

For rating / grading: ¯_(ツ)_/¯

There are basically two choices, both having catches:

• Leave it like it is, i.e. whatever the setting is for the cipher order: testssl.sh doesn't care.
• Consider an exception to the SSLlabs rating, like e.g. if there's a CBC cipher like above it'd be penalized. The worse the cipher the higher the penalty. Not sure (means: not sure) we should go down to F here.

Argument for the second: People wanted to have for non-HTTP protocols a rating which SSLlabs doesn't have. This is already a difference. But it opens the door to change the SSLlabs rating even further as people might start to argue "hey, you changed it there already, how about ...". The much I appreciated having a good standard but I am afraid this opens a can of worms. A proper own rating might be difficult to maintain -- and there are standards (also something which David implemented). I prefer the idea of pluggable rating standards which we discussed a while back.

Argument for the first: The SSLlabs rating stays like it is. There might be occasions where a penalty seems appropriate. But It'd be in line with SSLlabs which we can blame ;-)

"Leave it like it is" seems easier to me.

[dcooper16]

Hi Dirk,

Here is another attempt at specifying what the finding should be if the server does not enforce a cipher order.

In the table below, the columns represent the quality of the best cipher for the protocol version and the row represent the quality of the worst cipher. The upper part of the table is empty, since the level of the worst cipher can't be higher than that of the best cipher. Each entry with a column or row of "5" is "N/A" since there are no ciphers for which get_cipher_quality() returns 5. (In the https://github.com/dcooper16/testssl.sh/tree/nist branch that I created, get_cipher_quality() does sometimes return 5 when in --nist mode, but that can be addressed separately.)

When the quality of the best and worst ciphers are the same, the finding is always "INFO," since enforcing a cipher order does not help. When the best cipher has a quality of 7, the finding for not enforcing a cipher order is the same as the finding for the worst cipher. All other entries in the table are in italics. I've proposed values for them, but they could certainly change.

On the right side of the table, the severity levels are higher for small differences, because there seems to be larger differences in the cipher quality. For example, it seems that the quality difference between AES-CBS and 3DES (4 and 3) is not as great as between 3DES and DES (3 and 2).

	7	6	5	4	3	2	1
7	INFO
6	INFO	INFO
5	N/A	N/A	N/A
4	LOW	LOW	N/A	INFO
3	MEDIUM	MEDIUM	N/A	LOW	INFO
2	HIGH	HIGH	N/A	HIGH	HIGH	INFO
1	CRITICAL	CRITICAL	N/A	CRITICAL	CRITICAL	HIGH	INFO

[drwetter]

Thanks for the explanation / table, David.

The table looks good.

But still I do net get why does the cipher order for dev.testssl.sh and TLS 1.2 return red? As you mentioned above the ciphers on dev.testssl.sh are between 4 (AES-CBC) and 6/7 (GCM|CCM|*CHACHA20). That should be be flagged as LOW and should be in line what I said before.

Am I missing something or wasn't that just a suggestion, pending an implementation?

Thanks!

[dcooper16]

Am I missing something or wasn't that just a suggestion, pending an implementation?

The table is just a suggestion, and it would result in your example being flagged as LOW.

The current code is much simpler. If calculates the difference between the best and worst cipher and then computes the severity level as follows:

case $difference in
     0) outln " (no server order, thus listed by strength)" ;;
     1) prln_svrty_low " (no server order, thus listed by strength)" ;; 
     2) prln_svrty_medium " (no server order, thus listed by strength)" ;;
     *) prln_svrty_high " (no server order, thus listed by strength)" ;;
esac

In the example, the difference is 3, so it is flagged as HIGH.

I'll update the PR soon to implement what is in the table.

[drwetter]

I'll update the PR soon to implement what is in the table.

Perfect!

[dcooper16]

The PR should now be implementing what is in the table. Please let me know if you have any suggested changes.

[drwetter]

Hi David,

thanks a lot! Yup, that's good (broken server):

dev.testssl.sh:

One thing though. Sorry f I may appear nit-picking but there's a loss of information and a discrepancy between screen and file output: The verdict in the file output is a general one. It distinguishes between TLS 1.3 and TLS <=1.2 . Thus it looks a bit like the effort you spend e.g. for equally good or equally bad ciphers per protocol is lost when just looking at the file output. E.g. for dev.testssl.sh for TLS 1.0 and TLS 1.1 the cipher quality is the same, for TLS 1.2 it is not.

IIRC there were also not common servers reported here(?) where one can set the cipher order per protocols <= TLS 1.2. There it might be a problem bc other than for Nginx/Apache/IIS(?) the file output doesn't tell where it is important to change the cipher order.

Thx, Dirk

[dcooper16]

Okay, I added a new commit that sends information to the file output about whether a cipher order is enforced for every protocol version (except SSLv2). In addition, if a cipher order is enforced, the information sent to the file output indicates if the prioritize ChaCha fag is set.

[drwetter]

🎉 Perfect! Thanks a lot.

Is there any reason we need the line "Has server cipher order?" and "cipher_order" in fileout?

[dcooper16]

Is there any reason we need the line "Has server cipher order?" and "cipher_order" in fileout?

I don't think so. I think the "cipher_order" fileout is just a summary of what is now provided by the individual "cipher_order-${proto}" fileouts provide.

The "Has server cipher order? also seems to be just a summary, except in one case. If run_server_preference() is unable to determine whether the server enforces a cipher order, then nothing is printed (or sent to fileout) for each of the individual protocol versions, but the "Has server cipher order?" line says "unable to determine" (nothing is sent to fileout). Removing this does not seem to result in a loss of information, however, since code earlier in run_server_preference() already outputs a warning that the cipher order could not be determined (and also sends that warning to fileout).

[drwetter]

I had a quick look yesterday. Perhaps it suffices when run_server_preference() just contains the while loop, i.e. remove "negotiated protocols / cipher".

The negotiated protocol and cipher is derived from what we send in the client hello. That is different from client to client. We have a client section for that. If we want to keep that it would make sense to rephrase that so that it becomes clearer how we retrieved this information and what our point is :-)

Has anybody an opinion on this?