1334114 - NSS 3.28 regression in signature scheme flexibility, causes connectivity issue between iOS 8 clients and NSS servers with ECDSA certificates

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Alicja Kario]

NSS since 3.28.0 will use TLSv1.3-draft interpretation of signature algorithms extension. That means that ECDSA P-256 key can be used only to sign using SHA-256 hash and that ECDSA P-384 key can be used only to sign using SHA-384 hash.

If a client advertises support for just ECDSA+SHA256 and P-256 and P-384 curves, and  connects to a server with P-384 certificate installed, the connection will be aborted by the server with NSS 3.28, but will connect fine to a NSS 3.27 server.

In particular iOS 8 clients do advertise P-256 and P-384 curve and ECDSA+SHA256 signature algorithm, but they do not advertise ECDSA+SHA384 or ECDSA+SHA512 signature algorithm.
According to Nick Sullivan (Couldflare), about 3.8% of clients exhibit this kind of behaviour (with a sample of 50k connection attempts).

Comment 1

[Kai Engert [:KaiE:]]

Let me try to put it simpler:

NSS 3.28 is less flexible than it was in the past, which is a regression, and that regression prevents widely deployed clients to connect to NSS servers, if the NSS server upgrades to NSS 3.28

If a server is configured with a P-384 key, only, and a client doesn't offer SHA384 sig alg support, the connection fails with NSS 3.28.

It worked with NSS 3.27

We need a fix that restores the flexibility on NSS trunk for future releases, and a fix that can be backported to NSS 3.28

We need this fix rather quickly, because the March 7 upgrade of Firefox 52 will trigger a wide upgrade to NSS 3.28, and we should avoid the regression for servers that upgrade, too.

Comment 2

[Kai Engert [:KaiE:]]

Hubert, you said you could help us with creating a test client, that we could to use the potential fix?

Comment 3

[Kai Engert [:KaiE:]]

... that we could use to test the potential fix?

Comment 4

[Kai Engert [:KaiE:]]

I think I found a way to test it just with NSS code.

I've built NSS 3.28 with TLS 1.3 disabled.

I've started selfserv, using the certificate "server" directory produced by the NSS test suite, and configured selfserv to have an -ec key only.

In ssl3con.c, I changed the array
  SSLSignatureScheme defaultSignatureSchemes[]
to contain just one entry:
    ssl_sig_ecdsa_secp256r1_sha256

With that change to the code, tstclnt is unable to connect, with error
  SSL_ERROR_NO_CYPHER_OVERLAP

Comment 5

[Kai Engert [:KaiE:]]

Attachment: dont-match-group.patch

I don't know if this patch is good enough.

But at least in my limited connection test, it seems to work?

Comment 6

[Kai Engert [:KaiE:]]

This patch works on NSS trunk (pre-3.29), too.

Does that mean we still have all the code that is required to support this combination, but you simply tried to disable by policy?

Comment 7

[Alicja Kario]

(In reply to Kai Engert (:kaie) from comment #2)
> Hubert, you said you could help us with creating a test client, that we
> could to use the potential fix?

yes, working on it right now

(In reply to Kai Engert (:kaie) from comment #4)
> I think I found a way to test it just with NSS code.
> 
> I've built NSS 3.28 with TLS 1.3 disabled.
> 
> I've started selfserv, using the certificate "server" directory produced by
> the NSS test suite, and configured selfserv to have an -ec key only.
> 
> In ssl3con.c, I changed the array
>   SSLSignatureScheme defaultSignatureSchemes[]
> to contain just one entry:
>     ssl_sig_ecdsa_secp256r1_sha256

wouldn't use of SSL_SignatureSchemePrefSet() be cleaner?

Comment 8

[Kai Engert [:KaiE:]]

(In reply to Hubert Kario from comment #7)
> wouldn't use of SSL_SignatureSchemePrefSet() be cleaner?

I was just hardcoding to be able to test with minimal effort. If we want a real ability to test with a reduced set of schemes, we'll have to write code that dynamically parses a list from the test client command line.

Comment 9

[Alicja Kario]

downstream, I'll depend on tlsfuzzer test case, but I though that a gtest unit test that uses that method could be added upstream...

Comment 10

[Martin Thomson [:mt:]]

Comment on attachment 8830745 [details] [diff] [review]
dont-match-group.patch

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

Almost right.  You should probably ensure that the unit tests continue to work though.  I am fairly sure that there are some that will fail with this change.

::: lib/ssl/ssl3con.c
@@ +6378,5 @@
>          SSLSignatureScheme preferred = ss->ssl3.signatureSchemes[i];
>          PRUint32 policy;
>  
>          if (!ssl_SignatureSchemeValidForKey(!isTLS13 /* allowSha1 */,
> +                                            PR_FALSE /* matchGroup */,

this needs to be isTLS13

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8830745 [details] [diff] [review]
dont-match-group.patch

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

Please see https://nss-review.dev.mozaws.net/D181

Comment 12

[Martin Thomson [:mt:]]

trunk: https://hg.mozilla.org/projects/nss/rev/c5196cda0d0cc
NSS_3_29_BRANCH: https://hg.mozilla.org/projects/nss/rev/7fd9984bde06ba7a9e5

Comment 13

[Alicja Kario]

Verified as fixed.

Thank you!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Is this something we'll want to get in Firefox 52 before release too?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Martin, feel free to chime in as well :)

Comment 16

[Martin Thomson [:mt:]]

Let's do that.  (Leaving n-i in place so that I can send the necessary requests, make the appropriate tags, etc... when I have more time.)

Comment 17

[Martin Thomson [:mt:]]

I think that having Bug 1337580 will do, as long as it gets approved.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Fixed for Fx52 via bug 1337580.