1552254 - internal_error alert on Certificate Request with sha1+ecdsa in TLS 1.3

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Hubert Kario]

When Certificate Request in TLS 1.3 includes only SHA1+ECDSA pair, NSS client rejects it with internal_error alert instead of the expected handshake_failure.

Tested with nss-3.43.0

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:jcj, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Eric Rescorla (:ekr)]

Without a complete repro I can't be sure, but here's what I think is going on:

• The server asks for ECDSA/SHA-1
• The cert selection callback ignores this and returns an ECDSA P-256 cert
• https://searchfox.org/nss/source/lib/ssl/ssl3con.c#6127 calls ssl_SignatureSchemeFromSpki
  https://searchfox.org/nss/source/lib/ssl/ssl3con.c#4202 which returns ECDSA-P256-SHA256
• The check for ssl_CanUseSignatureScheme fails because the peer didn't advertise it, thus causing ssl_CompleteHandleCertificateRequest to fail
• https://searchfox.org/nss/source/lib/ssl/tls13con.c#4196 catches the failure and sends internal_error.

It's debatable whose mistake this is, because really the callback shouldn't supply an invalid cert, but that would require it knowing that ECDSA-SHA1 was illegal in TLS 1.3

Comment 3

[Eric Rescorla (:ekr)]

Marking Security until MT takes a look.

Comment 4

[Eric Rescorla (:ekr)]

MT, I think this is fine, but I haven't thought through the interactions with Firefox.

Comment 5

[Martin Thomson [:mt:]]

Yes, this looks right. We don't currently provide a callback with enough information to make a good choice. We pass (void*, PRFileDesc*, CERTDistNames*, CERTCertificate**, SECKEYPrivateKey**). We could maybe provide a new function that allowed the application code at the client to query for the valid signature schemes.

In this case, internal_error is valid in the sense that the GetClientAuthDataHook callback (which is internal) returned a bad answer. It should have returned SECFailure. But it's also the case that this isn't a great situation.

As for the impact on Firefox, the same sort of bug is highly likely. The code in nsNSSIOLayer.cpp is completely ignorant of any signing requirements from the server. It might even provide an RSA cert, even if that weren't listed.

That doesn't make this a security bug, just an annoying one. We refuse to sign with the bad/old signature scheme and fail the connection. This bug observes that we generate an inappropriate error (which is half true), but a real fix would require greater awareness of the constraints on certificate selection.

If we wanted to fix this properly, we'd want to have that code aware of this requirement. We might provide a new query function that would allow the application to do the filtering, but there are a few things to consider in doing that:

• we would need to ensure that we filtered out signature schemes that are invalid; and
• signature_schemes and signature_schemes_cert would need to be considered.

The alternative is to define a new configuration method. That method might take multiple certificates so that NSS can do the filtering itself. If that were to use the server certificate configuration code we have, we could reuse the certificate selection and maybe avoid creating new bugs as we do so. I'd want to fix that code (see bug 1547617) before doing this though.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Going through the code, I agree with Martin's analysis -- including that this is not a security bug. Unless someone says otherwise, I will open this back up tomorrow.

We know we have a lot of work to do regarding client cert auth -- Firefox and NSS both. It remains fairly low priority, though we'd be happy to see patches. Marking this P3.

Comment 7

[Hubert Kario]

(In reply to J.C. Jones [:jcj] (he/him) from comment #6)

including that this is not a security bug. Unless someone says otherwise

no disagreement here

We know we have a lot of work to do regarding client cert auth -- Firefox and NSS both. It remains fairly low priority, though we'd be happy to see patches. Marking this P3.

For us (Red Hat) client cert auth is a fairly important use-case, so if you are not confident in this code, we most likely will need to work on it.

Any particular areas you consider especially "hairy"? (if you consider that information sensitive, we can also discuss it through email in a closed group - please include all the CCd people then, or at least Bob and Daiki)

Also, sorry for not providing reproducer for this issue, but the only one I have is very ugly and requires patching multiple libraries to make it work so it is quite fragile.

Comment 8

[J.C. Jones [:jcj] (he/they)]

Let me pull in Dana who has the best handle on our client auth situation. She can decide whether to keep it in this bug, update the client auth wiki page, or start a separate thread.

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

I don't have a strong opinion here. Whatever we do, it seems like we just need a way for the application to know if a certificate will be acceptable for the connection in question.

Comment 10

[Hubert Kario]

(In reply to Dana Keeler (she/her) (use needinfo) (:keeler for reviews) from comment #9)
and what about client certificate code in general? regarding stuff from comment #7 ?

I don't have a strong opinion here. Whatever we do, it seems like we just need a way for the application to know if a certificate will be acceptable for the connection in question.

for the selection of key used for connection, I don't think it can be moved to a callback or left to a user application: code like that has a very high probability of not being forward compatible with future protocol versions. In TLS 1.2 it needs to take into account the negotiated ciphersute, in TLS 1.3 it doesn't but then some sigalgs cannot be used any more (rsa pkcs1 v1.5), others can be used with old keys (rsa-pss-rsae) while others still have new limitations (like P-256 key being allowed only with SHA-256 hash)... that's a lot of complex and error-prone code to put into an application.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler]]

One thing we could do is provide a function that takes a TLS socket and a certificate that returns whether or not that certificate can be used as a client certificate for that connection. Applications could call this repeatedly in the client auth data callback with candidate certificates until they find one that is acceptable. That way the complexity is contained in NSS.

Comment 12

[Robert Relyea]

I'm looking at Dana's suggestion. I think this is the right way. This will allow more constraints on the SSL side in the future, and still allow the client to build a list of acceptable certs to present to a human (if that's the client UI).

bob

(BTW I'm taking on the new callback and the changes to NSS tools, once that's done, the Firefox equivalent of the old PSM code will need to be updated as well).

Comment 13

[Robert Relyea]

Attachment: Bug 1552254 internal_error alert on Certificate Request with sha1+ecdsa in TLS 1.3

We need to be able to select Client certificates based on the schemes sent to us from the server. Rather than changing the callback function, this patch adds those schemes to the ssl socket info as suggested by Dana. In addition, two helpful functions have been added to aid User applications in properly selecting the Certificate:
PRBool SSL_CertIsUsable(PRFileDesc *fd, CERTCertificate *cert) - returns true if the given cert matches the schemes of the server, the schemes configured on the socket, capability of the token the private key resides on, and the current policy. For future SSL protocol, additional restrictions may be parsed.
SSL_FilterCertListBySocket(PRFileDesc *fd, CERTCertList *certlist) - removes the certs from the cert list that doesn't pass the SSL_CertIsUsable() call.

In addition the built in cert selection function (NSS_GetClientAuthData) uses the above functions to filter the list. In order to support the NSS_GetClientAuthData three new functions have been added:
SECStatus CERT_FilterCertListByNickname(CERTCertList *certList, char *nickname, void *pwarg) -- removes the certs that don't match the 'nickname'.
SECStatus CERT_FilterCertListByCertList(CERTCertlist *certList, const CERTCertlist *filterList ) -- removes all the certs on the first cert list that isn't on the second.
PRBool CERT_IsInList(CERTCertificate *, const CERTCertList *certList) -- returns true if cert is on certList.

In addition

• PK11_FindObjectForCert() is exported so the token the cert lives on can be accessed.
• the ssle ssl_PickClientSignatureScheme() function (along with several supporing functions) have been modified so it can be used by SSL_CertIsUsable()

Comment 14

[Robert Relyea]

Patch is checked in, closing the bug.