Ignore disabled ciphers when deciding if we are using ECC

[mattcaswell]

use_ecc() was always returning 1 because there are default (TLSv1.3)
ciphersuites that use ECC - even if those ciphersuites are disabled by
other options.

Fixes #7471

Checklist

• documentation is added or updated
• tests are added or updated

[kroeckx]

Looking at #7471, I don't see why this actually fixes the error that was reported. We still malloc(0), which can return NULL, so we can still return malloc failure.

[kroeckx]

I can't help thinking that this fix is at the wrong place. I don't expect SSL_get_ciphers() to return disabled ciphers.

[mattcaswell]

But that is what it is documented to do, e.g. compare the description of SSL_get_ciphers() with the other SSL_get*_ciphers() on this page:

https://www.openssl.org/docs/manmaster/man3/SSL_get_ciphers.html

[kroeckx]

Yes, this is the difference of the -s switch to the ciphers command. They both only return ciphers that match the cipher string. But SSL_get1_supported_ciphers() takes more into account, and so lists fewer ciphers. If TLS 1.3 is not supported, both functions should not return TLS 1.3 ciphers. So while I think this at least fixes something, I think there is also some other bug.

I wonder why SSL_get1_supported_ciphers() isn't used instead.

[kroeckx]

SSL_get1_supported_ciphers() does not return the TLS 1,3 ciphers when build using no-tls1_3, while SSL_get_ciphers() does.

[mattcaswell]

SSL_get1_supported_ciphers() does not return the TLS 1,3 ciphers when build using no-tls1_3, while SSL_get_ciphers() does.

Right - but it does that by calling ssl_cipher_disabled on the return from SSL_get_ciphers() - which is exactly what I'm doing here. On the downside it makes the memory management (a bit) more complicated. It makes little difference to me either way so I swapped it to use SSL_get1_supported_ciphers() instead.

[kroeckx]

I think you're still missing my point. SSL_get_ciphers() should not know anything about TLS 1.3 ciphers when build using no-tls1_3. It should never return TLS 1.3 ciphers.

[mattcaswell]

Hmm. This sounds like a potential "thin end of the wedge". SSL_get_ciphers() will exclude some ciphers if the underlying algorithm has been compiled out. E.g. no-camellia will remove any ciphers based on Camellia. So far we have not extended that to protocol versions. So if you ask for no-tls1_2, you'll still get TLSv1.2 only ciphersuites returned. Similarly you could ask for "no-tls1 no-tls1_1 no-tls1_2" and still get pre-TLSv1.3 ciphersuites back. I'm concerned that the conditional logic required for this in s3_lib.c (where the ciphersuites are defined) will start getting excessive for little benefit.

[mattcaswell]

@kroeckx - thoughts on this?

[kroeckx]

I can see your point too.

[mattcaswell]

Looking at #7471, I don't see why this actually fixes the error that was reported. We still malloc(0), which can return NULL, so we can still return malloc failure.

The PR address the actual underlying issue that #7471 was describing - rather than the immediate symptoms of the problem. Really we just don't support setting a zero length group list - but malloc failure isn't a particularly helpful error message. I've added a commit giving a better error.

[mattcaswell]

Pushed. Thanks.