1133566 - Add function to get negotiated key exchange algorithms during certificate verification

Status: RESOLVED DUPLICATE of bug 1084669

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Add-SSL_GetNegotiatedKeyExchangeAlgorithms.patch

Every TLS key exchange method (RSA, ECDHE-RSA) maps to an X.509 KeyUsage extension value. RSA key exchange is keyEncipherment and the other common ones are digitalSignature. NSS's libssl doesn't enforce that a connection that chose RSA key exchange is using a certificate with keyEncipherment set.

There is some compatibility risk with adding this check to NSS. At the same time, there is a large benefit to preventing RSA keys with only the digitalSignature bit set (and not the keyEncipherment bit set) from being used for RSA key exchange: these certificates would be guaranteed to always use ephemeral key exchange, which reduces liability in the case of a disclosure of the private key for the certificate, and which reduces the chance of downgrade attacks.

In order to investigate this compatibility risk, and also to facilitate the eventual addition of such matching to Firefox and libssl's default certificate auth hook, a new function is needed that allows the certificate auth hook to get the key exchange algorithms negotiated for the connection. This information is available from SSL_GetChannelInfo, but SSL_GetChannelInfo cannot be called in the auth certificate hook.

This patch adds that function.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In bug 1133562, there is a patch for Gecko that shows how this function can be used for the measurements. In bug 970760, we hope to enforce the matching, at least for RSA key exchange.

Comment 2

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

In the NSS meeting, the question was raised, "Why can't SSL_GetChannelInfo be called from the auth certificate hook? If we can fix that, wouldn't that make this API redundant?" Apparently Martin has been working on making SSL_GetChannelInfo callable earlier in the handshake - how is that going? If that's doable, it sounds like that's the preferred route. Otherwise, it sounds like we'll go ahead with adding this.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The auth certificate hook is called before we receive the ServerKeyExchange message. But, SSL_GetChannelInfo returns information that isn't available until the ServerKeyExchange message is received. For example, see this line:

 	inf.keaKeyBits       = ss->sec.keaKeyBits;

keaKeyBits is the size of the key from the ServerKeyExchange message.

It would be confusing and error-prone to have SSL_GetChannelInfo callable before it has all the information it needs to fill in all the fields in its return value.

It would be a performance regression to call the auth certificate hook later after the ServerKeyExchange and the rest of the stuff needed by SSL_GetChannelInfo is available.

Comment 4

[Martin Thomson [:mt:]]

In this case, Brian is probably right, though what I've proposed in bug 1084669 is that there be additional information populated in SSL_GetChannelInfo.  The existing info is zeroed prior to the handshake being "complete enough", but creating additional "preliminary" properties to the structure would be OK.  My plan was to add fields to the info struct for what was interesting (selected protocol, cipher suite, etc...) and add a state variable that would indicate whether only preliminary info is provided.  Adding kea parameters would be easy.

Comment 5

[Kai Engert [:KaiE:]]

Did you make a decision, if the proposed API is the right approach, or if a different approach should be used?

We should make a decision quickly, because we should finalize the NSS 3.18 release.

Once you tell me the API makes sense, I can do the review of the patch for correctness.

Comment 6

[Eric Rescorla (:ekr)]

Kai, my goal was to have mt and bsmith discuss this, so I think they should
resolve it. I'm happy to weigh in if more opinions are needed, but I don't think
we're there yet.

Comment 7

[Martin Thomson [:mt:]]

Brian said that he was unable to look at it properly for a couple of weeks.  If this is more urgent than that (I think that :keeler has some time pressure), we might need to resolve it sooner than that.

Comment 8

[Kai Engert [:KaiE:]]

We'd like to postpone this bug, because more discussion and investigation appears to be necessary, but we'd like to wrap up the 3.18 release.

Changing target milestone to 3.18.1

Comment 9

[Kai Engert [:KaiE:]]

Comment on attachment 8565128 [details] [diff] [review]
Add-SSL_GetNegotiatedKeyExchangeAlgorithms.patch

clearing review request, please re-request when ready.

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

1. The work in bug 1084669 subsumes this work.

2. When looking at the patch for bug 1084669, I realized that the patch I attached to this bug is hard to review because it doesn't document (via assertions or unit tests) that the (correct) information is available in the auth certificate hook.