Closed Bug 1130757 Opened 5 years ago Closed 5 years ago

OneCRL check should be in NSSCertDBTrustDomain::GetCertTrust, not NSSCertDBTrustDomain::CheckRevocation

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: briansmith, Assigned: mgoodwin)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 2 obsolete files)

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.
Blocks: 1105731
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: nobody → mgoodwin
Attached patch Bug1130757.patchSplinter Review
Attachment #8569825 - Flags: review?(dkeeler)
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+
Attached patch Bug1130757-tests.patch (obsolete) — Splinter Review
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+
Attached patch Bug1130757-tests.patch (obsolete) — Splinter Review
otherCA / otherIssuerEE works as suggested.

Thanks.
Attachment #8570471 - Attachment is obsolete: true
Attachment #8571292 - Flags: review+
... this time with a missing certificate file that was causing test failures on try
Attachment #8571292 - Attachment is obsolete: true
Attachment #8571400 - Flags: review+
Try is green
Keywords: checkin-needed
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?
Flags: needinfo?(mgoodwin)
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?
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?
(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.
Attachment #8569825 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8571400 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
We are at half of the 38 cycle, we have some time to fix potential issues.
You need to log in before you can comment on or make changes to this bug.