1049110 - save the verified certificate chain from TLS connections on TransportSecurityInfo (or SSLStatus or something)

Status: RESOLVED DUPLICATE of bug 1406856

(Core :: Security: PSM, defect, P3)

Description

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

For a number of reasons, when we successfully validate a certificate for a TLS connection, we should save the certificate chain built. This means we never have to attempt to re-create it, which can be problematic in a number of ways.

Comment 1

[Sami Jaktholm]

Attachment: sslstatus-store-verified-chain.patch

Here's a patch that makes SSLStatus to store the verified certificate chain. This is the first time I'm working on that code so feedback is more than welcome.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d9d183aa73d

Comment 2

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

Comment on attachment 8566408 [details] [diff] [review]
sslstatus-store-verified-chain.patch

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

This is a good start, but there are a few issues that need to be dealt with. First, whenever the data that comprises a TransportSecurityInfo changes, TRANSPORTSECURITYINFOMAGIC needs to change (see https://hg.mozilla.org/mozilla-central/annotate/1b4c5daa7b7a/security/manager/ssl/src/TransportSecurityInfo.cpp#l300 ). Since TransportSecurityInfo contains an nsSSLStatus, it would need to change in this case.
Next, AuthCertificate isn't always called, so the code that sets the verified chain won't always run. This can happen with session resumption, for example. To solve this, we would either need to re-verify the chain (which costs some time, which we're trying to avoid by doing resumption) or we need a verification cache of sorts (which would save time here and in other places, but would cost memory and would be complicated and difficult to get right). Basically, anywhere where we create a new nsSSLStatus we would need to potentially handle setting the verified chain.

::: security/manager/ssl/src/nsSSLStatus.cpp
@@ +170,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    mVerifiedCertChain = do_QueryInterface(chain);
> +    if (!mVerifiedCertChain) {
> +      return NS_NOINTERFACE;

I realize existing code does this, but I don't think NS_NOINTERFACE is an appropriate error code to use here. How about NS_ERROR_FAILURE?

Comment 3

[Sami Jaktholm]

Thanks for the feedback. I'll look into that when I got more time.

Comment 4

[Sami Jaktholm]

Attachment: sslstatus-store-verified-chain.patch

Finally found time to look into this.

I added a VerifiedCertChainCache (with structure similar to RememberCertErrorsTable) that caches the chains to a hashtable:
* Chains for successfully verified certificates are saved in AuthCertificate (SSLServerCertVerification.cpp) with the certificate SHA1 fingerprint as the key. In addition, the chain and the certificate is added to the SSLStatus object.
* If SSLStatus does not have a certificate when HandshakeCallback is called, VerifiedCertChainCache is queried for a chain (which should exist as the certificate should have been verified at least once) and it's added to the SSLStatus object.

In the end the SSLStatus object should always have a chain if the server certificate is available (excluding cases of failed verification).

However, there's some issues that need to be resolved:
1) The cached chains are never removed. Every certificate with a different SHA1 fingerprint will cause a chain of certificates to be added to the cache hogging up increasing amounts memory. The right time to remove a chain would be when it's guaranteed that the next time a certificate is seen, a verification will be performed (making the chain available again). However, I have no idea if it's possible to detect or know for sure when the chain is no longer needed...

The second solution would be to put a limit on the amount of chains stored and start throwing them out when the limit is reached. However, this could cause the chain to be occasionally unavailable which is bad.

2) I'm not really sure about the SHA1 fingerprint as a key to identify certificates. Does it work or is there something better available?

So, basically I have three questions:
1) Does any of this make any sense or am I completely off the tracks?
2) If this does make sense, which property of a certificate should be used as a key to identify them?
3) When should the chains be removed from the cache and how that moment can be detected?

Thanks in advance.

Comment 5

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

Comment on attachment 8574433 [details] [diff] [review]
sslstatus-store-verified-chain.patch

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

(In reply to Sami Jaktholm from comment #4)
> So, basically I have three questions:
> 1) Does any of this make any sense or am I completely off the tracks?

I think this could work, but I'm concerned about the unbounded memory usage issue (i.e. if we never remove anything from the cache, it will tend to grow indefinitely, and performance will degrade considerably over time).

> 2) If this does make sense, which property of a certificate should be used
> as a key to identify them?

SHA-1 probably shouldn't be used - the SHA-256 fingerprint of the certificate should be sufficient. As a backup, you could compare the entire raw encoding of the certificates (although if we find a SHA-256 collision, that would be quite incredible).

> 3) When should the chains be removed from the cache and how that moment can
> be detected?

Ideally NSS would be able to tell us when something has been ejected from its cache (and thus it wouldn't ever do resumption without negotiating a full handshake for that host/cert/whatever the unit of atomicity is in that case). I'm not aware of a way of doing this, and in any case arguably the better approach would be to fix bug 731478 and have NSS make available the entire peer cert chain even in the resumption case. Then, in HandshakeCallback, we can just re-verify the chain (similarly to this: https://hg.mozilla.org/mozilla-central/annotate/eab4a81e4457/security/manager/ssl/src/nsNSSIOLayer.cpp#l419 ). (We can probably make this work in 99% of cases even without fixing bug 731478 since we cache intermediates from successful handshake verifications, but we'd actually like to stop doing that. Also, I think it would be useful to expose the certificates the peer sent for other diagnostic purposes.)

If we fixed bug 1023621, we could even push the work of re-verifying the chain to exactly where the information is going to be used while not blocking the main thread. This saves both time and memory, since most connections won't ever inspect the full verified certificate chain (this would also be beneficial for things like removing nsIX509Cert.getChain() - bug 867473).

::: security/manager/ssl/src/nsSSLStatus.h
@@ +84,5 @@
> +    sInstance = new VerifiedCertChainCache();
> +    return NS_OK;
> +  }
> +
> +  static VerifiedCertChainCache & GetInstance()

nit: "static VerifiedCertChainCache& GetInstance()"

@@ +98,5 @@
> +  }
> +private:
> +  mozilla::Mutex mMutex;
> +
> +  static VerifiedCertChainCache * sInstance;

nit: "static VerifiedCertChainCache* sInstance;"

Comment 6

[Sami Jaktholm]

Sorry for the long delay. The method described in bug 731478 seems a whole lot better than what I'm doing in my patch. I think it's better to wait for bug 731478 to be fixed and do what you described.