Closed Bug 985021 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.)
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8393116 - Flags: review?(brian)
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 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.
Attachment #8393116 - Flags: review?(brian) → review+
Attached patch patch v1.1 (obsolete) — Splinter Review
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+.
Attachment #8393116 - Attachment is obsolete: true
Attachment #8394386 - Flags: review+
Summary: mozpkix/insanity::pkix: temporarily accept the presence of pathLenConstraint in EE basic constraints extensions → mozilla::pkix: temporarily accept the presence of pathLenConstraint in EE basic constraints extensions
https://hg.mozilla.org/mozilla-central/rev/2b638fb2d8fc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.