985021 - mozilla::pkix: temporarily accept the presence of pathLenConstraint in EE basic constraints extensions

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

According to rfc 5280 section 4.2.1.9:

   CAs MUST NOT include the pathLenConstraint field unless the cA
   boolean is asserted and the key usage extension asserts the
   keyCertSign bit.

However, there are some unexpired/unrevoked certificates in the wild that do exactly this. So, the plan is to temporarily accept the presence of the pathLenConstraint field in end-entity certificate basic constraints extensions. (This will be un-done in a later bug when we are confident there will be minimal compatibility issues.)

Comment 1

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

Attachment: patch

This applies on top of the patch in bug 982878, but that change seems superfluous now, so I'll probably WONTFIX that, or DUP it to this one.

Comment 2

[Richard Wang]

I appreciate this decision that it is hard to persuade all subscribers to reinstall new replacement cert since most website is the big traffic website in China. And the good thing is we corrected this problem for new issued cert from Jan. 1st, 2014.

Comment 3

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

Comment on attachment 8393116 [details] [diff] [review]
patch

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

::: security/insanity/lib/pkixcheck.cpp
@@ +184,5 @@
> +      PR_SetError(SEC_ERROR_EXTENSION_VALUE_INVALID, 0);
> +      return der::Failure;
> +    }
> +    basicConstraints.isCA = true;
> +  }

Nit: It would be better to factor this out into mozilla::pkix::der::OptionalBoolean so that we can unit test it separately and so we can reuse it in other places.

@@ +193,5 @@
> +      return der::Failure;
> +    }
> +    PR_SetError(0, 0);
> +    int32_t pathLenConstraint = DER_GetInteger(&pathLenConstraintEncoded);
> +    if (PR_GetError() != 0) {

DER_GetInteger returns long, not int32_t. CERTBasicConstraintsStr::pathLenConstraint has type int, not int32_t. Also, there is a better way to do the error checking:

unsigned long pathLenConstraint = DER_GetUInteger(&pathLenConstraintEncoded);
if (pathLenConstraint == ULONG_MAX) ||
    pathLenConstraint > numeric_limits<int>::max()) {
  return der::Fail(SEC_ERROR_EXTENSION_VALUE_INVALID);
}

@@ +200,5 @@
> +    if (pathLenConstraint < 0) {
> +      PR_SetError(SEC_ERROR_EXTENSION_VALUE_INVALID, 0);
> +      return der::Failure;
> +    }
> +    basicConstraints.pathLenConstraint = pathLenConstraint;

basicConstraints.pathLenConstraint = static_cast<int>(pathLenConstraint);

@@ +223,5 @@
>    CERTBasicConstraints basicConstraints;
>    if (cert.encodedBasicConstraints) {
> +    if (DecodeBasicConstraints(cert.encodedBasicConstraints,
> +                               basicConstraints) != der::Success) {
> +      return RecoverableError;

Sometimes this will result in SEC_ERROR_BAD_DER. You'll want to translate SEC_ERROR_BAD_DER into a better error code.

Comment 4

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

Attachment: patch v1.1

Thanks, Brian. I added der::OptionalBoolean and filed bug 986150 to use it in more places and add it to our test coverage.
Addressed comments and carrying over r+.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/de535cd27ee7

Comment 6

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

broke the build, backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/407a3ea120b0

Comment 7

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

Attachment: patch v1.2

Some signed/unsigned issues - looks like this fixed it: https://tbpl.mozilla.org/?tree=Try&rev=afc038f513b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d622f4ec6e9

Comment 8

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

Not doing so well, here: https://hg.mozilla.org/integration/mozilla-inbound/rev/10f1951a1f1f

Comment 9

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

Attachment: patch v1.3

https://tbpl.mozilla.org/?tree=Try&rev=7d117d38e5ec
OK - third time's the charm: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b638fb2d8fc

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b638fb2d8fc