Closed Bug 1534600 Opened 1 year ago Closed 1 year ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: florian, Assigned: keeler)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [fxperf:p2][psm-assigned])

Attachments

(1 file)

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.

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?

Flags: needinfo?(dkeeler)

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

Assignee: nobody → dkeeler
Flags: needinfo?(dkeeler)
Priority: -- → P1

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

Assignee: dkeeler → nobody
Priority: P1 → P3

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

Flags: needinfo?(dkeeler)
Whiteboard: [fxperf] → [fxperf:p2]

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.

Flags: needinfo?(dkeeler)
See Also: → 1540645
Assignee: nobody → dkeeler
Component: Normandy Client → Security: PSM
Priority: P3 → P1
Product: Firefox → Core
Whiteboard: [fxperf:p2] → [fxperf:p2][psm-assigned]
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c7a5a7b9cd50
make nsIContentSignatureVerifier asynchronous r=KevinJacobs,mythmon,glasserc
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.