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]

Hi @odinmylord,

Thanks for making the changes. The revised PR looks good to me. I made suggestions for minor changes to a couple of lines of code, to make then more efficient, but I think the PR is acceptable as is.