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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

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.

Comment 1

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

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?

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

This will set a more helpful error when a basic constraints extension can't be decoded.

Comment 6

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

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.

Comment 7

[Eddy Nigg (StartCom)]

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

[Richard Wang]

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

[Kathleen Wilson]

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?

Comment 10

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

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

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v1.1

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

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.