Closed
Bug 1130757
Opened 8 years ago
Closed 7 years ago
OneCRL check should be in NSSCertDBTrustDomain::GetCertTrust, not NSSCertDBTrustDomain::CheckRevocation
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: briansmith, Assigned: mgoodwin)
References
Details
(Keywords: perf)
Attachments
(2 files, 2 obsolete files)
3.51 KB,
patch
|
keeler
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
169.51 KB,
patch
|
mgoodwin
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Path building works like this: 1. GetCertTrust(end-entity certificate). 2. GetCertTrust(1st intermediate certificate). 3. GetCertTrust(2nd intermediate certificate). 4. GetCertTrust(Root) 5. CheckRevocation(2nd intermediate certificate). 6. CheckRevocation(1st intermediate certificate). 7. CheckRevocation(end-entity certificate). There are two details to note: 1. CheckRevocation is NOT called for trust anchors (in particular root certificates). However, I thought a key feature of OneCRL is that it is supposed to improve Mozilla's response time for revoking trust in compromised root certificates. 2. CheckRevocation is the very last thing called for a certificate. The reason we call it last is to avoid making a network fetch for an OCSP response that would be signed by an untrusted issuer. However, since OneCRL is signed by a trusted third party, and since there's no network I/O involved, there's no need to wait to see if the issuer is trusted before making the check. And, indeed, it is better for performance for this check to happen as early as possible, instead of being deferred to the end. In particular, consider this case: +-- Int1 <-- Int2a (Blocked by OneCRL) <- ... / EE <--+ \ +-- Int1 <-- Int2b <-- ... If you can detect that Int2a is blocked by OneCRL in GetCertTrust, then you can potentially save a lot of work by stopping the building of that path much earlier.
Reporter | ||
Comment 1•7 years ago
|
||
Also, we call GetCertTrust for OCSP response signing certificates, but we don't call CheckRevocation for them (to avoid loops, and for performance). Thus, OneCRL currently provide revocation of OCSP response signing certificates either.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mgoodwin
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8569825 -
Flags: review?(dkeeler)
Assignee | ||
Comment 3•7 years ago
|
||
In terms of tests, should I add some to ensure that we can use this to revoke roots, etc?
Comment on attachment 8569825 [details] [diff] [review] Bug1130757.patch Review of attachment 8569825 [details] [diff] [review]: ----------------------------------------------------------------- This looks great. (In reply to Mark Goodwin [:mgoodwin] from comment #3) > In terms of tests, should I add some to ensure that we can use this to > revoke roots, etc? Yes, that sounds good.
Attachment #8569825 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 5•7 years ago
|
||
A test. This patch creates new root, intermediate and ee certificates to allow us to test revoking a root. At some point, it might be nice to rename some of the certificates and files in tlsserver/generate_certs.sh to make things tidier (first-ca, second-ca etc. might be easier to follow than test-ca, other-ca, etc). Though maybe this belongs in Bug 1105753 (since we'll need to do that if we're recreating in any case).
Attachment #8570471 -
Flags: review?(dkeeler)
Comment on attachment 8570471 [details] [diff] [review] Bug1130757-tests.patch Review of attachment 8570471 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but I think we can use otherCA / otherIssuerEE instead of generating new certificates. r=me if that works as expected. ::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh @@ +336,5 @@ > # Make a valid EE using testINT to test OneCRL revocation of testINT > make_EE eeIssuedByIntermediate 'CN=EE issued by intermediate' testINT "localhost" > export_cert eeIssuedByIntermediate test-int-ee.der > > +# Make a chain for testing use of OneCRL for root revocation Can't we just re-use otherCA / otherIssuerEE instead of generating another hierarchy?
Attachment #8570471 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 7•7 years ago
|
||
otherCA / otherIssuerEE works as suggested. Thanks.
Attachment #8570471 -
Attachment is obsolete: true
Attachment #8571292 -
Flags: review+
Assignee | ||
Comment 8•7 years ago
|
||
... this time with a missing certificate file that was causing test failures on try
Attachment #8571292 -
Attachment is obsolete: true
Attachment #8571400 -
Flags: review+
Assignee | ||
Comment 9•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8364822d55ea
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/83c8e3ad6835 https://hg.mozilla.org/integration/mozilla-inbound/rev/b204778f4a7e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/83c8e3ad6835 https://hg.mozilla.org/mozilla-central/rev/b204778f4a7e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Unless I'm mistaken, this affects 37 and 38 as well. I imagine we're not going to fix this for 37 since it's basically already out the door, but we should fix this for 38. Mark, can you take care of preparing a branch patch if necessary?
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8569825 [details] [diff] [review] Bug1130757.patch Approval Request Comment [Feature/regressing bug #]: 1130757 [User impact if declined]: Root revocations will not be possible via OneCRL (meaning chemspill if we need to respond to an incident requiring such) [Describe test coverage new/current, TreeHerder]: covered by existing OneCRL tests for existing functionality, new test added for root revocation case. [Risks and why]: Low risk since OneCRL is checked for most certificates anyway; this simply moves the call site. [String/UUID change made/needed]: No
Flags: needinfo?(mgoodwin)
Attachment #8569825 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8571400 [details] [diff] [review] Bug1130757-tests.patch Approval Request Comment [Feature/regressing bug #]: 1130757 [User impact if declined]: see comment 14 [Describe test coverage new/current, TreeHerder]: see comment 14 [Risks and why]: see comment 14 [String/UUID change made/needed]: no
Attachment #8571400 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #13) > Mark, can you take care of preparing a branch patch > if necessary? It seems a branch patch is not necessary; the patches apply cleanly to aurora (and pass tests) with no changes.
Updated•7 years ago
|
Attachment #8569825 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8571400 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•7 years ago
|
||
We are at half of the 38 cycle, we have some time to fix potential issues.
Comment 18•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/03f55ec25d9a https://hg.mozilla.org/releases/mozilla-aurora/rev/852468558109
Flags: in-testsuite+
Bustage fix for 38: https://hg.mozilla.org/releases/mozilla-aurora/rev/fb59ff1c6cdf
You need to log in
before you can comment on or make changes to this bug.
Description
•