1057123 - mozilla::pkix: allow end-entity certificates to assert keyCertSign in some cases

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Similarly to bug 1034124, when an end-entity certificate has a key usage extension that asserts keyCertSign, it is technically invalid. However, for compatibility reasons, this error should be overridable.

Do do this, we should add a new error (MOZILLA_PKIX_ERROR_END_ENTITY_ASSERTS_KEY_CERT_SIGN or something) and use it in the appropriate place in pkixcheck.cpp (see the patch in bug 1040446). Then, we should make that error overridable by modifying NSSErrorsService.cpp (see the patch in bug 1034124). To test, generate a certificate with this problem in generate_certs.sh and add a test to test_cert_overrides.js.

Comment 1

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

I suggest that we just replace the check that an end-entity certificate's KU doesn't contain keyCertSign with a comment saying that we allow end-entity certificates (certificates without basicConstraints.cA set to true) to claim keyCertSign for compatibility reasons, but it doesn't matter because we only trust certificates with basicConstraints.cA set to true as CAs.

Comment 2

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

Attachment: patch

Ok - I turned the check into a comment.
(It looks like none of the key usage tests we had exercised this situation, so I added some tests. Unfortunately, because the generator scripts don't support an add-only mode (i.e. don't regenerate existing certificates), this patch is much larger than it needs to be.)

Comment 3

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

David, don't you need to update the gtest pkixcheck_CheckKeyUsage_tests.cpp too?

Comment 4

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

Whoops - forgot about that. Thanks.

Comment 5

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

Attachment: patch v2

Comment 6

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

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

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

::: security/pkix/lib/pkixcheck.cpp
@@ +154,5 @@
> +  // extension (Section 4.2.1.9) MUST also be asserted."
> +  // However, we allow end-entity certificates (i.e. certificates without
> +  // basicConstraints.cA set to TRUE) to claim keyCertSign for compatibility
> +  // reasons. This does not compromise security because we only allow
> +  // certificates with basicConstraints.cA set to TRUE to act as CAs.

I suggest that if requiredKeyUsageIfPresent == keyCertSign and endEntityOrCA == MustBeEndEntity, then we should fail with ERR_INADEQUATE_KEY_USAGE, i.e.:


// your comment
if (requiredKeyUsage == KeyUsage::keyCertSign &&
    endEntityOrCA != EndEntityOrCA::MustBeCA) {
  return Result::ERROR_INADEQUATE_KEY_USAGE;
}

::: security/pkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp
@@ -199,5 @@
> -TEST_F(pkixcheck_CheckKeyUsage, keyCertSign)
> -{
> -  NAMED_SIMPLE_KU(good, 2, 0x04);
> -  ASSERT_BAD(CheckKeyUsage(EndEntityOrCA::MustBeEndEntity, &good,
> -                           KeyUsage::keyCertSign));

I think that this result should still be the same. The only thing that should be different is the result of CheckKeyUsage(EndEntityOrCA::MusteEndEntity, &good, Keyusage::someOtherKeyUsage) for an encoded key usage that includes someOtherKeyUsage AND keyCertSign.

Comment 7

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

(Sorry my suggestion in comment 1 was not well thought out, as you did exactly what I said in comment 1, instead of what I meant. Let me know if you disagree.)

Comment 8

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

Attachment: patch v3

I think what you outlined in comment 6 sounds like a good solution.

Comment 9

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

Thanks for the review. Updating summary to more accurately describe the goal of this bug.
https://tbpl.mozilla.org/?tree=Try&rev=9166806dc1d3

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2829560211c8

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/2829560211c8

Comment 13

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

Attachment: patch for beta

Approval Request Comment
[Feature/regressing bug #]: mozilla::pkix (enabled by default in 31, not disable-able in 33)
[User impact if declined]: users won't be able to access HP CommandView servers (see duplicate bugs)
[Describe test coverage new/current, TBPL]: has tests
[Risks and why]: low
[String/UUID change made/needed]: none

Comment 14

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

[Tracking Requested - why for this release]: mozilla::pkix compatibility bug (see comment 13)

Comment 15

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

Comment on attachment 8483641 [details] [diff] [review]
patch v3

Approval Request Comment
see comment 13

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/40f37e556c95
https://hg.mozilla.org/releases/mozilla-beta/rev/599ae9ec1b9c