1295060 - Don't pretend signature_algorithms wasn't present when there are no common algorithms

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

In handling signature algorithms, we decide that when there are no supported signature algorithms that this must mean that there was no extension.  That's daft because what really happened is that we didn't have any signature schemes in common.

This is server only, or I would suggest that we need some compatibility testing on the browser end.

Comment 1

[Martin Thomson [:mt:]]

Bob, if I were to fail the handshake if there were no common signature algorithms (as opposed to assuming the defaults), would this cause Redhat any issues?

Comment 2

[Eric Rescorla (:ekr)]

Shouldn't we be testing whether the signature algorithms extension was negotiated?

Comment 3

[Martin Thomson [:mt:]]

Probably, yes.  We currently check the number of signature schemes that we have in common before deciding to use the common ones.  The idea of this patch would be to change the defaults check to be based on whether the extension was present.

Comment 4

[Martin Thomson [:mt:]]

Attachment: bug1295060-1.patch

If signature_algorithms is present, but we don't support any of the schemes that were provided by the client, don't fall back to the defaults.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8782238 [details] [diff] [review]
bug1295060-1.patch

Review of attachment 8782238 [details] [diff] [review]:
-----------------------------------------------------------------

Martin, 

Can you please produce a test that demonstrates that if SigAlgs is omitted, then you cannot negotiate ECDSA?

::: lib/ssl/ssl3con.c
@@ +7055,5 @@
>      SECStatus rv;
>  
> +    if (!ssl3_ExtensionNegotiated(ss, ssl_signature_algorithms_xtn)) {
> +        /* The signature_algorithms extension is mandatory if the client expects
> +         * the server to authenticate with a signature. */

an you fix this comment and put it inside the 1.3block?

Comment 6

[Martin Thomson [:mt:]]

Statute of limitations has expired.  ekr, see https://nss-dev.phacility.com/D18

I realize that this probably messes with your stuff, so I'm happy to rebase on top of the negotiation patch if needed, but the test seems to be valuable.  I'm ambivalent regarding the error code.

Comment 7

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/502c53690edf

Comment 8

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/449fbc61acf9