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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 2

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1130757.patch

Comment 3

[Mark Goodwin [:mgoodwin]]

In terms of tests, should I add some to ensure that we can use this to revoke roots, etc?

Comment 4

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

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.

Comment 5

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1130757-tests.patch

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).

Comment 6

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

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?

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1130757-tests.patch

otherCA / otherIssuerEE works as suggested.

Thanks.

Comment 8

[Mark Goodwin [:mgoodwin]]

Attachment: Bug1130757-tests.patch

... this time with a missing certificate file that was causing test failures on try

Comment 9

[Mark Goodwin [:mgoodwin]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8364822d55ea

Comment 10

[Mark Goodwin [:mgoodwin]]

Try is green

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83c8e3ad6835
https://hg.mozilla.org/integration/mozilla-inbound/rev/b204778f4a7e

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83c8e3ad6835
https://hg.mozilla.org/mozilla-central/rev/b204778f4a7e

Comment 13

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

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?

Comment 14

[Mark Goodwin [: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

Comment 15

[Mark Goodwin [:mgoodwin]]

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

Comment 16

[Mark Goodwin [:mgoodwin]]

(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.

Comment 17

[Sylvestre Ledru [:Sylvestre]]

We are at half of the 38 cycle, we have some time to fix potential issues.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/03f55ec25d9a
https://hg.mozilla.org/releases/mozilla-aurora/rev/852468558109

Comment 19

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

Bustage fix for 38: https://hg.mozilla.org/releases/mozilla-aurora/rev/fb59ff1c6cdf