1079658 - (CVE-2014-8642) Critical id-pkix-ocsp-nocheck extension in an OCSP delegated responder certificate causes OCSP response verification to fail

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

[Tracking Requested - why for this release]:

We currently treat all delegated OCSP responder certificates as if they had id-pkix-ocsp-nocheck, but we don't recognize the id-pkix-ocsp-nocheck extension. This causes problems if the id-pkix-ocsp-nocheck extension is present in the delegated response signer certificate.

This could be a security issue because we'll throw away any Revoked/Unknown response that was  signed by such a certificate.

This should be easy to fix: Just add the logic for the extension to pkixcert.cpp's BackCert::RememberExtension. We actually don't need to remember its value because we don't do anything with it.

Comment 1

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

When we fix this bug, we'll accept more Revoked/Unknown OCSP responses than before. This is a good thing, but it will technically reduce site compatibility.

Comment 2

[Sylvestre Ledru [:Sylvestre]]

This might be too late for 33 since we should be releasing next Tuesday.
Anyway, tracking in case we make a build2

Comment 3

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

Brian, are you intending to work on this? If not, I can.

Comment 4

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

David, it would be great if you could take this.

One thing to consider is whether the presence of this extension in a non-OCSP context (when requiredEKUIfPrresent != id_kp_OCSPSigning || endEntityOrCA != EndEntityOrCA::EndEntity) should be considered an error.

Comment 5

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

I imagine so, but I bet that will lead to some compatibility issues. Maybe it would be best to do this in two pieces: handle the presence of id-pkix-ocsp-nocheck, and then restrict it to OCSP contexts.

Comment 6

[Daniel Veditz [:dveditz]]

Maybe sec-moderate? Although given Chrome's opinion of OCSP (they dropped support) maybe actual value is sec-low.

Comment 7

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

Attachment: part 1/2: handle id-pkix-ocsp-nocheck extension

Let me know if you think the certificate extension gtest refactoring should be in another patch.

Comment 8

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

Attachment: part 2/2: disallow id-pkix-ocsp-nocheck in non-ocsp contexts

And then here's part 2. Thanks!

Comment 9

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

Comment on attachment 8505106 [details] [diff] [review]
part 1/2: handle id-pkix-ocsp-nocheck extension

Review of attachment 8505106 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixcert.cpp
@@ +267,5 @@
>    } else if (extnID.MatchRest(id_ce_inhibitAnyPolicy)) {
>      out = &inhibitAnyPolicy;
>    } else if (extnID.MatchRest(id_pe_authorityInfoAccess)) {
>      out = &authorityInfoAccess;
> +  } else if (extnID.MatchRest(id_pkix_ocsp_nocheck)) {

Please make a comment about why we are doing this: we need to make sure we don't reject delegated OCSP response signing certificates that contain a critical nocheck extension when validating OCSP responses. Otherwise, the application may ignore the valid OCSP response. In particular, without this, an application that implements soft-fail OCSP to ignore valid Revoked and/or Unknown responses, and an application that implements hard-fail OCSP may fail to connect to a server even with a Good response.

::: security/pkix/lib/pkixutil.h
@@ +132,5 @@
>    Input extKeyUsage;
>    Input inhibitAnyPolicy;
>    Input keyUsage;
>    Input nameConstraints;
> +  Input ocspNocheck;

Can't this just be a local variable in RememberExtension?

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +191,5 @@
> +  // Tests that a critical extension not in the id-ce or id-pe arcs (which is
> +  // thus unknown to us) is detected and that verification fails with the
> +  // appropriate error.
> +  EXTENSION_TESTCASE(unknownCriticalExtensionBytes,
> +                     Result::ERROR_UNKNOWN_CRITICAL_EXTENSION),

If we're going to do this now, which I think is fine to do, why not do it this way:

EXTENSION_TESTCASE(
    TLV(der::SEQUENCE,
        tlv_unknownCriticalExtensionOID + 
        Boolean(true) +
        TLV(der::OCTET_STRING, ByteString()),
    Result::ERROR_UNKNOWN_CRITICAL_EXTENSION),
...

I guess this would require the pkixtestutil changes from bug 1077859, but those should be easy to uplift.

Doing that would make these changes much easier to review.

Comment 10

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

Comment on attachment 8505108 [details] [diff] [review]
part 2/2: disallow id-pkix-ocsp-nocheck in non-ocsp contexts

Review of attachment 8505108 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/lib/pkixcert.cpp
@@ +186,5 @@
> +  if (ocspNocheck.GetLength() > 0 &&
> +      (requiredEKUIfPresent != id_kp_OCSPSigning ||
> +       endEntityOrCA != EndEntityOrCA::MustBeEndEntity)) {
> +    return Result::ERROR_EXTENSION_VALUE_INVALID;
> +  }

Is there a reason this is here instead of CheckIssuerIndependentProperties? In general,

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +123,5 @@
> +  { \
> +    endEntityOrCA, \
> +    requiredEKUIfPresent, \
> +    expectedResult \
> +  }

Is this macro necessary? It seems like it would be clearer to just use the raw syntax, since we're not transforming the parameters in any way.

@@ +249,5 @@
> +  // Tests that a certificate with the id-pkix-ocsp-nocheck extension (marked
> +  // critical) in yet another non-OCSP context does not verify successfully.
> +  OCSP_EXTENSION_TESTCASE(EndEntityOrCA::MustBeCA,
> +                          KeyPurposeId::anyExtendedKeyUsage,
> +                          Result::ERROR_EXTENSION_VALUE_INVALID),

What about the cases where it is not marked critical?

I can understand rejecting non-OCSP-signer certificates when it is critical.
But, should we really reject a non-OCSP-signer certificate when it has a non-critical nocheck extension? Does a specification say we should do that?

@@ +301,5 @@
> +        0x04, 0x0C, // OCTET STRING (length = 12)
> +          0x30, 0x0A, //SEQUENCE (length = 10)
> +            0x06, 0x08, // OID (length = 8)
> +              // 1.3.6.5.5.7.3.9 (id_kp_OCSPSigning)
> +              0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x03, 0x09

Again, since we already did the work to make TLV and Boolean and other things work in a reasonable way in tests, we might as well use them instead of hand-coding the test data like this.

Comment 11

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

Comment on attachment 8505106 [details] [diff] [review]
part 1/2: handle id-pkix-ocsp-nocheck extension

Review of attachment 8505106 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +182,5 @@
> +      // 1.3.6.1.5.5.7.48.1.5
> +      0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01, 0x05,
> +    0x01, 0x01, 0xff, // BOOLEAN (length = 1) TRUE
> +    0x04, 0x02, // OCTET STRING (length = 2)
> +      0x05, 0x00, // NULL

Please cite RFC 6960:

ext-ocsp-nocheck EXTENSION ::= { SYNTAX NULL IDENTIFIED
                                 BY id-pkix-ocsp-nocheck }

Also, please see http://comments.gmane.org/gmane.ietf.x509/30947. We should test the other encodings of this extension too.

Comment 12

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

Attachment: patch v2

Thanks for the reviews, Brian. I think I've addressed all of your comments for the part 1 patch. Also, I think I'm going to finish part 2 in a separate follow-up bug, since there may be compatibility concerns and it's probably not a good idea to uplift in that case.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

sec-moderate + no patch ready yet. I am going to skip that for 33.1.

Comment 14

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

Comment on attachment 8513782 [details] [diff] [review]
patch v2

Review of attachment 8513782 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/gtest/pkixcert_extension_tests.cpp
@@ +115,5 @@
> +  0x85, 0x1a, 0x01, 0x83, 0x74, 0x09, 0x03
> +};
> +
> +// python DottedOIDToCode.py --tlv wrongExtensionOID 1.3.6.6.1.5.5.7.1.1
> +// (there is an extra "6" that shouldn't be in this OID)

Nit: put this value after tlv_id_pe_authorityInformationAccess, because the "wrongness" is easier to understand that way.

@@ +125,5 @@
> +static const uint8_t tlv_id_pe_authorityInformationAccess[] = {
> +  0x06, 0x08, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x07, 0x01, 0x01
> +};
> +
> +// python DottedOIDToCode.py --tlv id-ce-unknown 2.5.29.55

Please add a comment that this is a made-up OID for testing that "id-ce"-prefixed OIDs we don't understand.

@@ +162,5 @@
> +  { TLV(der::SEQUENCE,
> +        BytesToByteString(tlv_unknownExtensionOID) +
> +        Boolean(true) +
> +        TLV(der::OCTET_STRING, ByteString())),
> +    Result::ERROR_UNKNOWN_CRITICAL_EXTENSION },

In bug 1063281 and bug 970542, we put the "}," on its own line. Which one of us is doing things the "correct" way? Let's be consistent. (Perhaps unsurprisingly, I think I'm doing it the "Mozilla Standard" way, but maybe I'm misunderstanding.)

@@ +165,5 @@
> +        TLV(der::OCTET_STRING, ByteString())),
> +    Result::ERROR_UNKNOWN_CRITICAL_EXTENSION },
> +
> +  // Tests that a non-critical extension not in the id-ce or id-pe arcs (which
> +  // is thus unknown to us) verifies successfully.

NIT: add "even if empty" since extensions we know about aren't normally allowed to be empty. Also, please put this ahead of the previous cases, so that it is clear that the empty value for an unknown extension is allowed for that case too (or, explicitly say so in its comment).

@@ +173,5 @@
> +    Success },
> +
> +  // Tests that an incorrect OID for id-pe-authorityInformationAccess
> +  // (when marked critical) is detected and that verification fails.
> +  // (Until bug 1020993 was fixed, the code checked for this OID.)

s/the code checked for this OID/this wrong value was used for id-pe-authorityInformationAccess/.

@@ +188,5 @@
> +  { TLV(der::SEQUENCE,
> +        BytesToByteString(tlv_id_pe_authorityInformationAccess) +
> +        Boolean(true) +
> +        TLV(der::OCTET_STRING, TLV(der::SEQUENCE, ByteString()))),
> +    Success },

NIT: Put this ahead of the previous test case.

Comment 15

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #12)
> Also, I think I'm going to finish part 2 in a separate
> follow-up bug, since there may be compatibility concerns and it's probably
> not a good idea to uplift in that case.

Perhaps we should just file that bug as a TODO, and not even bother with the updated patch for now.

Comment 16

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

Attachment: patch v2.1

Thanks for the reviews, Brian. I believe I've addressed all of your review comments (in particular, I think you're right about the placement of the close-curly-braces).

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a4aa0b6ee40d
https://hg.mozilla.org/integration/mozilla-inbound/rev/acef7c16e3f1

Comment 17

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

Apparently that try wasn't good enough, because I still broke the tree:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b70e15bc452

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/acef7c16e3f1
https://hg.mozilla.org/mozilla-central/rev/6b70e15bc452

Comment 19

[Lawrence Mandel [:lmandel] (use needinfo)]

This isn't a very large patch (mostly tests) but given that we've shipped this before, that the bug is rated sec-moderate, and that we're already at beta8, unless there is a significant user impact with this bug I think we should wait to take it in 35.

keeler - Do you want to make the case today for taking this fix in 34?

Comment 20

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

I don't think we have any indication that there is significant user impact with this bug at this time (and furthermore there isn't a branch patch prepared). I agree we should take it for 35 and not 34.

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a nomination here for uplift?  Would be good to get this into Mon Dec 22 beta.

Comment 22

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

Attachment: patch for beta

Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix
[User impact if declined]: relevant certificate revocation information might not be heeded in rare cases
[Describe test coverage new/current, TBPL]: has tests
[Risks and why]: low - this has been riding the trees for a while now
[String/UUID change made/needed]: none

I had to cherry-pick some trivial testing utilities (basically DNSName from bug 1063281) in the testing code, but otherwise this applied and appears to work fine. Carrying over r+.

Comment 23

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d88622dd6622