Closed Bug 982878 Opened 7 years ago Closed 7 years ago

(mozilla::pkix) With mozilla::pkix enabled, some sites throw SEC_ERROR_BAD_DER instead of expired cert error

Categories

(Core :: Security: PSM, defect)

30 Branch
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: mwobensmith, Assigned: keeler)

References

()

Details

Attachments

(2 obsolete files)

Requires turning on insanity::pkix via pref security.use_insanity_verification = true

Two sites:
https://www.involar.com 
https://proportal.system5.jp

In Fx28 and Fx30 default - as well as Chrome 33 - sites do not load, and display an error pertaining to an expired cert.

In Fx30 with insanity::pkix enabled, sites do not load, but we receive SEC_ERROR_BAD_DER instead.
Matt can you share your test setup or code? I can reproduce the expired cert errors but I cannot find a reference to the SEC_ERROR_BAD_DER anywhere. Where did you see that error?
The certificate for https://www.involar.com appears to have an invalid basic constraints extension (it looks like it's just the hex bytes 30 03 02 01 00).
I think what's wrong with it is since the cA bit isn't asserted (by being omitted and defaulting to false) but the pathLenConstraint is included (albeit with a value 0), it's technically incorrect 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.
Looks like https://proportal.system5.jp/ chains to a x509v1 root certificate, so that should be fixed by bug 969188, much like in bug 982879.
Attached patch patch (obsolete) — Splinter Review
This will set a more helpful error when a basic constraints extension can't be decoded.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8390776 - Flags: review?(brian)
Comment on attachment 8390776 [details] [diff] [review]
patch

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

::: security/insanity/lib/pkixcheck.cpp
@@ +163,5 @@
> +        PR_SetError(SEC_ERROR_EXTENSION_VALUE_INVALID, 0);
> +        if (endEntityOrCA == MustBeEndEntity) {
> +          return Fail(FatalError, SEC_ERROR_EXTENSION_VALUE_INVALID);
> +        }
> +      }

Should we return SEC_ERROR_CA_CERT_INVALID instead? One of the design ideas behind insanity::pkix's checking of extensions is that every CheckXXX function returns a distinct and consistent error code, which would be SEC_ERROR_CA_CERT_INVALID in this case. If we return SEC_ERROR_EXTENSION_VALUE_INVALID then it isn't going to be clear *which* extension is invalid.

On the other hand, SEC_ERROR_CA_CERT_INVALID maybe doesn't make sense if isCA=false. On the third hand, I'm not sure that is an overriding concern.
Attachment #8390776 - Flags: review?(brian) → review+
I'm not entirely sure, but I assume there are maybe quite some certificates in use that might have the pathlen set - since this was never an issue (the path length is superfluous, but shouldn't have an influence on the correct functioning of a certificate) with any common browser, I suggest to be careful with this change. There might be many more out there.
WoSign have 1086 valid SSL certificates that have this problem that still valid after June 30, 2014. We realized this problem and we updated our PKI/CA system that corrected this problem since Jan. 1st, 2014. I don't know if 3 months is enough time for us to finish the all related certs replacement.
Can we change the behavior in mozpkix(insanity::pkix) to not throw an exception when pathLenConstraint is included in end-entity certs?

Is this issue worth specifically communicating to CAs and declaring a date when such certs will no longer work?
(In reply to Kathleen Wilson from comment #9)
> Can we change the behavior in mozpkix(insanity::pkix) to not throw an
> exception when pathLenConstraint is included in end-entity certs?

Yes. We should do this in a separate bug though.

> Is this issue worth specifically communicating to CAs and declaring a date
> when such certs will no longer work?

If we make this change then we should file a follow-up bug to undo it. Then we can discuss there the timeline and how to help CAs prepare.
Attached patch patch v1.1 (obsolete) — Splinter Review
I realized the endEntityOrCA check was useless, so I took it out. Carrying over r+.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> Should we return SEC_ERROR_CA_CERT_INVALID instead? One of the design ideas
> behind insanity::pkix's checking of extensions is that every CheckXXX
> function returns a distinct and consistent error code, which would be
> SEC_ERROR_CA_CERT_INVALID in this case.

Understood. However, CheckBasicConstraints already returns SEC_ERROR_CA_CERT_INVALID or SEC_ERROR_PATH_LEN_CONSTRAINT_INVALID, and since this is the only place we're using SEC_ERROR_EXTENSION_VALUE_INVALID, I think this at least makes things no worse (and indeed is more informative than what we have now).
Attachment #8390776 - Attachment is obsolete: true
Attachment #8392989 - Flags: review+
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #10)
> (In reply to Kathleen Wilson from comment #9)
> > Can we change the behavior in mozpkix(insanity::pkix) to not throw an
> > exception when pathLenConstraint is included in end-entity certs?
> 
> Yes. We should do this in a separate bug though.

Filed bug 985021.

> > Is this issue worth specifically communicating to CAs and declaring a date
> > when such certs will no longer work?
> 
> If we make this change then we should file a follow-up bug to undo it. Then
> we can discuss there the timeline and how to help CAs prepare.

Filed bug 985025.
Summary: (insanity::pkix) With insanity::pkix enabled, some sites throw SEC_ERROR_BAD_DER instead of expired cert error → (mozilla::pkix) With mozilla::pkix enabled, some sites throw SEC_ERROR_BAD_DER instead of expired cert error
Comment on attachment 8392989 [details] [diff] [review]
patch v1.1

I just pushed the patch for bug 985021 to inbound, which basically obviates this. I'll close this bug as WONTFIX or INVALID or something when that one sticks.
Attachment #8392989 - Attachment is obsolete: true
The problem with https://www.involar.com was fixed by bug 985021.
The problem with https://proportal.system5.jp was fixed by bug 969188, I believe.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.