1534600 - fetchSignedObjects is janky due to NSS doing main thread I/O

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[Florian Quèze [:florian]]

Visible in this startup profile (this was a startup with a fresh profile; nothing in the user profile directory yet) https://perfht.ml/2EYqEUz

Also visible in https://perfht.ml/2EYCZYZ where it was less janky but still did a lot of main thread I/O.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey Dana, I'm not really familiar with nsIContentSignatureVerifier, but I see your name in the log - out of curiosity, how hard would it be to make verifyContentSignature async, and have the SQLite query that it performs occur off of the main thread?

Comment 2

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

This wouldn't be that hard, I don't think.

Comment 3

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

I don't actually have time to do this right now.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Hey Dana, could you post a brief description of what would need to be done here? As you mentioned it wouldn't be that hard to do, maybe someone else can work on it

Comment 5

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

The easiest fix would be to avoid decoding the certificates with NSS. This would actually be fairly easy:

• Return an array of arrays of bytes from ReadChainIntoCertList
• CSTrustDomain would take that array and blindly loop over it in FindIssuer rather than trying to match issuer/subject names (mozilla::pkix makes this fast by doing that check itself)
• CSTrustDomain::GetCertTrust would have to use mozilla::pkix::BackCert to get a handle on the certificate's subject/issuer/serial/public key for revocation checking.
• To see if a given cert is the content signing root, we could modify nsINSSComponent::IsCertContentSigningRoot to take either a precomputed hash or the bytes of the cert to be hashed and compared.
• Back in ContentSignatureVerifier::CreateContextInternal, BackCert can be used to again get the cert's public key.

Unfortunately this doesn't really solve the problem of doing a certificate verification on the main thread. To address that issue, we would have to put this logic on some background thread and either return a promise or notify via an xpcom callback type. Further unfortunately, this API is used both from js and ContentVerifier, where it seems we don't have any kind of js context ( https://searchfox.org/mozilla-central/rev/201450283cddc9e409cec707acb65ba6cf6037b1/dom/security/ContentVerifier.cpp#46 ), so maybe promises wouldn't work here. Also, ContentVerifier imposes some constraints on how nsIContentSignatureVerifier is used because it expects that it will make a network request to fetch the certificate chain that signed the data. This request can only be started and consumed from the main thread, so we're looking at going back and forth between many different threads. To be blunt, I'd like to get rid of this aspect of the API since it's not even actually used yet, but I'm told that "we've talked about [using] it in a number of places". I'm a bit skeptical, though, since we landed this code almost 3 years ago and this part of it still isn't being used. It might be worth splitting this functionality into pieces that can be a bit more separate from each other and yet make use of common utility functions or something.

Comment 6

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

Attachment: bug 1534600 - make nsIContentSignatureVerifier asynchronous r?KevinJacobs,mythmon,glasserc

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c7a5a7b9cd50
make nsIContentSignatureVerifier asynchronous r=KevinJacobs,mythmon,glasserc

Comment 8

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/c7a5a7b9cd50