fix curves findings in TLS1.2 and prior versions

[Odinmylord]

Describe your changes

If a server uses a certificate with secp256r1 as its private key testssl.sh only finds the "secp256r1" while the server also offers secp384r1 and secp521r1. This is caused by TLS versions prior to 1.3 closing the connection with the error "no shared cipher" if the client does not send the curve used in the certificate in the supported_groups extension.
The easiest solution (which is the one implemented here) is to move the curves found to the end of the supported_groups extension instead of removing them, once testssl.sh finds the same curve a second time it moves forward meaning that only an additional handshake is performed. An alternative would be to first find the private key of the certificate and, if a curve is used, add that curve as the last one instead of removing it from the ClientHello.

What is your pull request about?

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Typo fix
• Documentation update
• Update of other files

If it's a code change please check the boxes which are applicable

• For the main program: My edits contain no tabs and the indentation is five spaces
• I've read CONTRIBUTING.md and Coding_Convention.md
• I have tested this fix against >=2 hosts and I couldn't spot a problem
• I have tested this new feature against >=2 hosts which show this feature and >=2 host which does not (in order to avoid side effects) . I couldn't spot a problem
• For the new feature I have made corresponding changes to the documentation and / or to help()
• If it's a bigger change: I added myself to CREDITS.md (alphabetical order) and the change to CHANGELOG.md

[dcooper16]

Hi @Odinmylord,

Thanks for reporting this and for providing a PR. I was able to reproduce the issue. I thought that the supported_groups extension was only about key exchange, but RFC 8422 does seem to indicate that it is also about the server's certificate.

I think it hadn't been noticed before since server's that have ECC certificates tend to also have RSA certificates, in which case the connection is still successful using the RSA certificate.

I haven't had a chance to test your PR, but I have a couple of comments.

I believe that the change you made for testing with OpenSSL also needs to be made for testing with tls_sockets(). The current change may be sufficient if OpenSSL supports one of the server's cipher suites and all of the server's supported curves, but the testing with tls_sockets() that comes next is there to catch things that OpenSSL does not support.

I am concerned that this change will not work with all servers. When doing testing of #2617 I noticed that most servers selected signature algorithms based on their own preference order rather than the client's specified preference order. When I tested your proposed fix against an OpenSSL server (i.e., openssl s_server), moving P-256 to the end of the list of supported curves in the openssl s_client command resulted in the server choosing a curve that appeared earlier in the client's list. However, what if there are servers that (like with the list of cipher suites or supported signature algorithms) ignore the client's preference order? It seems that this change would result in the server just choosing the same curve again and other supported curves not being found.

So, while I don't know of a server to test against that ignores the client's preference order for supported_groups, my guess is that a slightly more complicated solution is needed -- run the tests as they are currently done, and then when that is done run the tests as you propose. This would add more lines of code, but would only result in one additional test. In most cases the second set of tests could be skipped. If "RSA" appears in $ecdhe_cipher_list, then that should indicate that the second set of tests aren't needed, as the server could choose an RSA certificate based cipher suite when the supported_groups extension doesn't include the curve in the ECC certificate.

[Odinmylord]

Hi, @dcooper16 I fixed the code as you proposed and tested it both with system openssl and sockets.

[dcooper16]

Hi @Odinmylord,

Sorry it took me so long to look at this code. I haven't had a chance to try running the code, but I read through it and have just a few minor comments. When I get some more time, I'll try testing it out.

[dcooper16]

The "|| break" part of this line is very confusing to me. I would suggest one of two alternatives:

• Enclose the entire "while true" loop in an "if" statement that checks whether the second run is needed.
• Remove the "if" from this line (and the corresponding "fi"). If the condition is not satisfied, then the execution will break out of the loop, so the subsequent code does not need to be within an "if"/"then" clause.

[dcooper16]

This "if" statement should be removed. As noted in the comment where curves_deprecated is defined, a curve is deprecated if it is one that MUST NOT be used with TLS 1.3. Here, you want to try all of the curves, even the deprecated ones.

[dcooper16]

This "if" statement should be removed for the same reason as the previous check for deprecated curves.

[dcooper16]

I believe this line should be modified similarly to the one on line 10880, so that there is not an "if" statement with an "|| break" that is part of it.

[dcooper16]

This "if" statement should be deleted. As noted above, deprecated curves should only be excluded from TLS 1.3 ClientHello messages. Also, there is no need to check that "$proto$ == 03, as that is done earlier.

[dcooper16]

As with the "if" statement on line 10957, this isn't needed.