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

RESOLVED FIXED in mozilla31

Status

()

defect
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mwobensmith, Assigned: keeler)

Tracking

30 Branch
mozilla31
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(2 obsolete attachments)

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.
Posted 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.

Comment 8

5 years ago
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.

Comment 9

5 years ago
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.
Posted 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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.