[WIP] Fix #714 - Switch to (mainly) sslyze for TLS testing

[mxsasha]

Known changes/fixes

• Server cipher order preference detects issues that were missed by the old code.
• Sometimes some ciphers are not detected by the old code.

Specifics:

• cbc.badssl.com web has good cipher order in the old code, but new implementation says it's bad for preferring TLS_RSA_WITH_AES_256_CBC_SHA256 (phase out) over TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (sufficient) - SSL labs and testssl confirm my new order detection.
• sidn.nl mx has TLS_ECDHE_RSA_WITH_CAMELLIA_256_CBC_SHA* which is insufficient and not detected by the old code
• sidn.nl mx prefers TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 over TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 which sufficient>good, not detected in old code.
• github.com offers both ECDSA and RSA certificate and this causes two glitches in RSA authenticated ciphers that are not detected by testssl and ssllabs, nor the old code which counts it as good. First, it has TLS_RSA_WITH_AES_256_GCM_SHA384 amongst others which is phase out. Second, it prefers TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 over TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256 which is bad as AES-128-CBC is sufficient only in NCSC guidelines.

TODO

• ECDH-X25519 is falsely counted as insufficient
• Make sure we catch sslyze error states
• Get the logging framework to work and make useful logs
• We're not using the internetnl trust store
• Trust isn't actually checked
• Some mix of web/mail scorings in cert_checks() and check_pubkey()
• check_pubkey() might need some correction in DH checking
• Mail server cache currently disabled
• Only one mailserver tested right now
• check_mail_tls() used to depend SNI on whether DANE was present??? Look into that.
• Check whether we're handling connection issues correctly
• OCSP is probably not using the right trust store
• Check if DANE validation actually works
• Re-enable OCSP check - might be in certificate info scan?
• Re-enable our trust store
• Make sure sslyze dependency is in dep files correctly
• Key exchange hash function (kex_hash_func) doesn't seem supported by sslyze or has a different name or I'm overlooking it
• Hostname matching may be wrong for mail - and check score penalty this is partually handled a layer up
• Is SoftTimeLimitExceeded ever raised?
• Parallelise different mail server tests?
• Review/refactoring
• Duplicate ciphers in cipher list
• Duplicate kex parameters
• On cbc.badssl.com, Cipher order fails with first pair "None"
• Verify kex sha2 detection
• TLS versions not ordered
• Look into SSL_OP_PRIORITIZE_CHACHA (Support for SSL_OP_PRIORITIZE_CHACHA #461)
• Clean out print statements
• Extract my arm64 hacks into a separate PR
• Add CAMELLIA/SEED which are missing from appendix C
• Think about improving 4xx detection

Fixes to sslyze:

• Set proper SMTP HELO hostname PR
• Return more detail on HELO rejected PR
• Set custom user agent on HTTP requests
• Server cipher order is unsupported by sslyze

Later:

• Remove custom nassl from docker
• Document any known differences
• Validate against NCSC guidelines, especially the footnotes
• Add normative references
• Remove OpenSSL names from cipher names
• Fix up the tests
• Run comparison

Content updates:

• We no longer use the difference between "cipher order wrong" and "cipher order not enforced", the only verdict labels are now bad/good/na
• We use IANA cipher names now.
• Update labels for cipher order tech table (IP address, Preferred by server, Expected preference). This was not noticeable before due to a rendering bug.

[mxsasha]

Summary:

• Anything not noted in here should work.
• There are a number of unclear things after evaluating the old code and the TLS guidelines, noted below.
• I've tried to extract lists of ciphers etc from the code in the new branch to make it easier to understand and update our policies.
• Server cipher order is entirely unsupported and currently always OK.
• Mail server tests through sslyze are somewhat slower.
• Cipher list currently outputs full IANA names too to aid in debugging.
• I've not run a full check of the code against the TLS guidelines, so there are probably unidentified differences with the new code. It is also likely that the old code has bugs fixed by this PR, and that the new code has new, different bugs.
• Something is off with certificate hostname matching for mail.
• Looking for input on the points below.

Parallel mail server tests

Not immediately related but associated, I want to try parallelizing the mail server tests on individual different mail servers. The upside is faster results, especially useful now that the test per server is a bit slower. The downside is potential increased blocking from services that do not check the number of connections to an IP per mail server, but for the entire pool. But I can't say whether anyone does that.

SNI sending on mail server connections and DANE

Our current code only sends SNI on mailservers when DANE is present. Is that desired and intentional behavior? The new code always sends SNI, but it's not hard to change.

Public key DH

Old code / current new code

I am puzzled by some details of this. This concerns the validation of the certificate sent by the server, referring NCSC guidelines B3-3, B5-1, which in turn refer table 8 and 9. In my reading, the old code accepted RSA>=2048, DSA>=2048, DH>=2048 and SECP521R1/SECP384R1/SECP256R1/X25519/X448 >= 224, plus SECP224R1 as phase out.

Two questions:

• Isn't the key size check for named curves superfluous because all these curves have a fixed key size?
• I can not find a reference in the TLS guidelines for acceptance of DH keys >= 2048. Am I overlooking it or should we not accept this?

Hash function for key exchange

We check a hash function for key exchange, for "SHA2 support for signatures", referring table 5. I can not figure out the context of this check - this is separate from the hash in the cipher, right?

[baknu]

* Isn't the key size check for named curves superfluous because all these curves have a fixed key size?

* I can not find a reference in the TLS guidelines for acceptance of DH keys >= 2048. Am I overlooking it or should we not accept this?

For DHE we also check if finite field groups (RFC 7919) are used. The test explanation might also give some more insight: https://en.internet.nl/site/example.nl/2591039/#control-panel-14. This mentions guideline B5-1 and table 9 for ECDHE, and guideline B6-1 and table 10 for DHE. It also states that the RSA public parameters are tested in the subtest 'Public key of certificate'.

[mxsasha]

* Isn't the key size check for named curves superfluous because all these curves have a fixed key size?

* I can not find a reference in the TLS guidelines for acceptance of DH keys >= 2048. Am I overlooking it or should we not accept this?

For DHE we also check if finite field groups (RFC 7919) are used.

Currently we do not do this for the certificate, only for the key exchange. For public keys, the current implementation accepts any DH key >= 2048 bits.

This mentions guideline B5-1 and table 9 for ECDHE, and guideline B6-1 and table 10 for DHE.

My reading of this is also that this applies to any use of ECDHE/DHE, so the test should check DH public keys for specific finite field groups.

It also states that the RSA public parameters are tested in the subtest 'Public key of certificate'.

I have not been able to locate code that currently does this, or what the requirements are that we have of the parameters.

[baknu]

@gthess Can you shed your light on this? Thanks!

[gthess]

The following is input based on my memory and a quick look over the (old) code. Both can be disputed :)

SNI sending on mail server connections and DANE

I believe the condition on the SNI is there because without DANE there is nothing to verify for a mail client, there is no certificate store for email; any certificate is good and is used for encryption. DANE changed that (other than DANE-EE) and requires name verification. So I believe internet.nl tries to behave like a non-DANE capable client when DANE is absent, and like a DANE capable client when DANE is present. I believe you can find corner cases for a testing platform with both behaviors so I don't think it ultimately matters if SNI is sent or not. What may matter is an extra check to see if DANE and non-DANE clients get different certificates (with different properties). But that is way out of scope for this question.

Isn't the key size check for named curves superfluous because all these curves have a fixed key size?

For these curves yes. But I believe it has to do with the following text from the explanation "We check if the bit-length of the used elliptic curves is a least 224 bits. Currently we are not able to check the elliptic curve name." The size check may be a remnant from a previous version of the guidelines that I can no longer find; I don't see a size check in the current version. So the curve names are there for comparison but I don't think we were able to get them from nassl back then so the 'or' is excercised instead.

Currently we do not do this for the certificate, only for the key exchange. For public keys, the current implementation accepts any DH key >= 2048 bits.
My reading of this is also that this applies to any use of ECDHE/DHE, so the test should check DH public keys for specific finite field groups.

The checking of finite fields is happening on the test "key exchange parameters" because that is what DH et al. is used for. Now it seems that DH gets a pass for size just to pass the public key test. Can't remember if that was deliberate or it used to work like that on the previous guideline version and we just left it there because of no harm. Maybe in the "Public key of certificate" test we can now say that DH et al. is already checked on the key exchange test. Otherwise checking the finite field on both tests would be confusing.

It also states that the RSA public parameters are tested in the subtest 'Public key of certificate'.

I have not been able to locate code that currently does this, or what the requirements are that we have of the parameters.

I understand that this is the RSA check on the public key (table 8).

Hash function for key exchange
I can not figure out the context of this check - this is separate from the hash in the cipher, right?

It is different indeed, this is negotiated as a Hello extension to designate the hash algorithm to use before signing for key exchange, when applicable.

[mxsasha]

sslyze will not integrate server cipher order preference checking. However, it's easier than we thought to do it ourselves. We only need to check that good>sufficient>phase out>bad. So once we know accepted ciphers and their goodness, we can make a connection with client preference for e.g. all sufficient ciphers, followed by all good ciphers. If the connection ends up using a good cipher, we are sure cipher order preference is enforced and at least one good is preferred over all sufficient. The same can be done for any tuple of cipher sets. Therefore, we can determine enough with just 3 connection attempts, much less than for a full cipher order.

Part of the needed API is visible in https://github.com/nabla-c0d3/sslyze/blob/release/sslyze/plugins/openssl_cipher_suites/_test_cipher_suite.py#L35 - so this shouldn't be too hard/complex to write ourselves.

[gthess]

Indeed, this is what the old code was doing, although it was weirdly mixed with the connection test to avoid extra connections; mostly made sense for the mail test.

[mxsasha]

SNI sending on mail server connections and DANE

I believe the condition on the SNI is there because without DANE there is nothing to verify for a mail client, there is no certificate store for email; any certificate is good and is used for encryption. DANE changed that (other than DANE-EE) and requires name verification. So I believe internet.nl tries to behave like a non-DANE capable client when DANE is absent, and like a DANE capable client when DANE is present. I believe you can find corner cases for a testing platform with both behaviors so I don't think it ultimately matters if SNI is sent or not. What may matter is an extra check to see if DANE and non-DANE clients get different certificates (with different properties). But that is way out of scope for this question.

It seems at least Exim and Postfix do not send SNI when DANE is absent (unless specifically configured to do so), so I think we should follow and only send SNI with present DANE. That means our testing stays close to real scenarios, and the existing code.

[mxsasha]

Suspected bug in existing implementation: cbc.badssl.com has good cipher order in production, but my implementation says it's bad for preferring TLS_RSA_WITH_AES_256_CBC_SHA256 (phase out) over TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA (sufficient) - tested on TLS 1.2. SSL labs and testssl confirm my new order detection.

[mxsasha]

There is some complication regarding cipher orders and TLS versions. We currently test three things entirely separate from each other: TLS versions, ciphers enabled, and cipher order. So a server with "TLS_AES_256_GCM_SHA384:TLS_DHE_RSA_WITH_3DES_EDE_CBC_SHA" enforced order with TLS 1.3 and 1.2, will fail ciphers enabled, but succeed TLS versions and cipher order.

Also, we have decided that we will test cipher order using only the highest TLS version supported, ignoring TLS 1.3.

This creates a complexity for 6 cipher suites, which are listed under sufficient in NCSC appendix C, with the footnote:

These algorithm selections, combined with TLS 1.3 are Good. However, the (old) ciper suite notation used here will frequently result in the use of at most TLS 1.2 in software, which is Sufficient.

(note that the affected ciphers are also listed in the good section in their TLS 1.3 notation)

In the ciphers enabled code, I have currently counted those as "good", e.g. TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, because that test looks only at the ciphers, and does not care about TLS versions.

Question 1: is this the desired behaviour? This means TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 TLS 1.2 is good on ciphers enabled, sufficient on TLS version. Alternative: score sufficient on both, which means only TLS 1.3 specific ciphers can be rated good in ciphers enabled.

If my current implementation is desired behaviour, it creates a complication in the cipher order. If we see TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 as good, it means it should be preferred over e.g. TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384, a "true" sufficient cipher. This is my current new implementation, and it fails on e.g. forumstandaardisatie MX (on TLS 1.2), as that prefers the CBC over the GCM cipher: the server prefers a "true" sufficient cipher over a "sufficient-kinda-but-only-because-it's-TLS-1.2" cipher. Current production succeeds, i.e. allows this.

Question 2: should a cipher order of TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384 > TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 on TLS 1.2 lead to a failed cipher order test?

Pro: test should fail. In isolation, looking only at the cipher suite parameters, a sufficient (CBC) is preferred over a good (GCM). The fact that the TLS version would downgrade a real client connection to sufficient rating on either cipher, is out of scope as the test only concerns cipher order.

Con: test should succeed, as a client connecting on TLS 1.2 would receive a sufficient rating with either of these ciphers. Side effect: we would never compare good ciphers in ordering. In this interpretation, good ciphers are exclusive to TLS 1.3, and on TLS 1.2 any cipher can be sufficient at best. Suspected current production implementation.

The fundamental problem is: we separate ciphers enabled, cipher order and TLS version into 3 isolated and independent ratings. But a connection from a client has one rating in the end, which involves multiple of these factors. Then again, we could make the same argument for RSA key size, though isolating that is much more unambiguous.

My leaning: we should rate TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 as good in the ciphers enabled test, as that test should be entirely isolated from the TLS version. We should allow (i.e. con section) the cipher order mentioned above as for real world use, either cipher provides an equal rating.

Note: this is unrelated to the suspected bug I identified in the previous comment: that concerned phase out vs sufficient ciphers. The ambiguity is only in 6 ciphers that NCSC lists as sufficient in the appendix but on their own merits are good.

[gthess]

If these are upgraded to good when combined with TLS1.3 and the cipher order test does not run on TLS1.3 (because everything is supposed to be good there), I would say treat them as sufficient for the cipher order test.

[bwbroersma]

It seems the code has different time-outs than the current code.
After @nlitsme forced me with 🍺 to add httpdelay= to b6a.nl, setting it to 10000 (10s) triggered expected 'not reachable' in internet.nl (test), while the current sslyze rewrite just finished without a not reachable.

[bwbroersma]

It seems the current PR handles tls_mail_smtp_starttls task different in terms of finishing and timeouts. It correctly finishes, but reports the TLS test failed as a timeout, while the current code continues in the back-end (while erroring the front-end) and can then provide a result with the /results url-hack.
See #1425 (comment)

BTW I tried to run the sslyze branch, but got an error:

worker-1     | ModuleNotFoundError: No module named 'sslyze'

probably because the development is done on a special internetnl sslyze fork? Would be nice to really test both versions (on the same IP's).