Closed Bug 1097622 Opened 5 years ago Closed 5 years ago

mozilla::pkix returns ERROR_NOT_YET_VALID_CERTIFICATE or ERROR_EXPIRED_CERTIFICATE rather than ERROR_INVALID_TIME when decoding invalid time values

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox33 --- affected
firefox34 --- affected
firefox35 --- affected
firefox36 --- affected
firefox38 --- fixed
firefox-esr31 --- affected

People

(Reporter: decoder, Assigned: Cykesiopka)

References

Details

Attachments

(4 files, 4 obsolete files)

Attached file Malformed certificate
Someone in the german Firefox support channel reported this issue and submitted the certificate attached for inspection. Fx <31 accepted this cert, as well as other browsers. Fx31+ reject the certificate with the following error:

"This certificate is not valid before 11.11.2014 15:20. The current time is 11.11.2014 16:30. (sec_error_expired_certificate)"

Original message is also here: http://www.imagenetz.de/fa8775a6f/firefox.png.html


I inspected the certificate closely and it looks like there is someting fishy going on with the "not after" timestamp:

>  156:d=2  hl=2 l=  28 cons: SEQUENCE          
>  158:d=3  hl=2 l=  13 prim: UTCTIME           :141111122219Z
>  173:d=3  hl=2 l=  11 prim: UTCTIME           :1612311200Z

This is clearly an RFC violation:

> 4.1.2.5.1  UTCTime

>    The universal time type, UTCTime, is a standard ASN.1 type intended
>    for representation of dates and time.  UTCTime specifies the year
>    through the two low order digits and time is specified to the
>    precision of one minute or one second.  UTCTime includes either Z
>    (for Zulu, or Greenwich Mean Time) or a time differential.
> 
>    For the purposes of this profile, UTCTime values MUST be expressed
>    Greenwich Mean Time (Zulu) and MUST include seconds (i.e., times are
>    YYMMDDHHMMSSZ), even where the number of seconds is zero.


We should either throw a DER decoding error here, or make this certificate work if this kind of misencoding is somewhat common.
Flags: needinfo?(dkeeler)
That said it could still be that this standard violation is unrelated to the actual fault. But I was not able to reproduce this with regular timestamps at all, so I suspect it is somehow related.
From https://www.cs.auckland.ac.nz/~pgut001/pubs/x509guide.txt

Validity
--------

Validity ::= SEQUENCE {
    notBefore               UTCTIME,
    notAfter                UTCTIME
    }

This field denotes the time at which you have to pay your CA a renewal fee to
get the certificate reissued.  The IETF originally recommended that all times
be expressed in GMT and seconds not be encoded, giving:

  YYMMDDHHMMZ

as the time encoding.  This provided an unambiguous encoding because a value of
00 seconds was never encoded, which meant that if you read a UTCTime value
generated by an implementation which didn't use seconds and wrote it out again
with an implementation which did, it would have the same encoding because the
00 wouldn't be encoded.

However newer standards (starting with the Defence Messaging System (DMS),
SDN.706), require the format to be:

  YYMMDDHHMMSSZ

even if the seconds are 00.
Yes, it looks like this is the problem. In CheckValidity (in pkixcheck.cpp), we'll return ERROR_EXPIRED_CERTIFICATE if we can't decode the notBefore/notAfter values (which we can't in this case, since not including the seconds makes it an invalid value for UTCTime). Interestingly, internally the error is ERROR_INVALID_TIME, which seems like a more informative error to return to the user in this case. We could change it to return that (NB: if we do, we'd probably have to make that error overridable).
Flags: needinfo?(dkeeler)
Summary: (mozilla::pkix) Weird error message with "not before", possibly due to non-standard expiry date format → mozilla::pkix returns ERROR_EXPIRED_CERTIFICATE (rather than ERROR_INVALID_TIME) when decoding invalid time values
Taking with permission from David over IRC.
Assignee: dkeeler → cykesiopka.bmo
Status: NEW → ASSIGNED
Depends on: 968560
Summary: mozilla::pkix returns ERROR_EXPIRED_CERTIFICATE (rather than ERROR_INVALID_TIME) when decoding invalid time values → mozilla::pkix returns ERROR_NOT_YET_VALID_CERTIFICATE (rather than ERROR_INVALID_TIME) when decoding invalid time values
(Sorry for bugspam, I realised that the title should cover both the pre and post Bug 968560 behaviour to avoid duplicates being filed).
Summary: mozilla::pkix returns ERROR_NOT_YET_VALID_CERTIFICATE (rather than ERROR_INVALID_TIME) when decoding invalid time values → mozilla::pkix returns ERROR_NOT_YET_VALID_CERTIFICATE or ERROR_EXPIRED_CERTIFICATE rather than ERROR_INVALID_TIME when decoding invalid time values
This patch renames the (mE|e)rrorCodeExpired variables to (mE|e)rrorCodeTime.

The variables are used for more than just "expired" errors now, and "Time" is used everywhere else for the same concept.
Main patch for returning ERROR_INVALID_TIME when decoding invalid time values.
Attachment #8562773 - Flags: feedback?(brian)
This patch adds test cases to see if overrides are allowed for certs that have notBefore times earlier than the UNIX epoch.
Comment on attachment 8562773 [details] [diff] [review]
bug1097622_part1_INVALID_TIME-instead-of-EXPIRED_CERTIFICATE-invalid-times_v1.patch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +353,5 @@
>        errorCodeTrust = defaultErrorCodeToReport;
>  
>        SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, now, false);
>        if (validity == secCertTimeUndetermined) {
> +        if (PORT_GetError() != 0) {

I'm making this change with the expectation that this line:
> return(secCertTimeExpired); /*XXX is this the right thing to do here?*/ 
in CERT_CheckCertValidTimes() can be changed to return secCertTimeUndetermined instead.

If that's unlikely, I'll just remove this part of the logic.
Comment on attachment 8562773 [details] [diff] [review]
bug1097622_part1_INVALID_TIME-instead-of-EXPIRED_CERTIFICATE-invalid-times_v1.patch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +351,5 @@
>      {
>        collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED;
>        errorCodeTrust = defaultErrorCodeToReport;
>  
>        SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, now, false);

Let's expose some form of mozilla::pkix::CheckValidity that takes a DER encoding of a certificate and a time and returns Success if the certificate is valid at that time or indicates an invalid encoding, not yet valid, expired, etc.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #11)
> Let's expose some form of mozilla::pkix::CheckValidity that takes a DER
> encoding of a certificate and a time and returns Success if the certificate
> is valid at that time or indicates an invalid encoding, not yet valid,
> expired, etc.

Ok, I'll drop the |validity == secCertTimeUndetermined| changes and rebase on top of Bug 1123671.
(In reply to David Keeler [:keeler] (use needinfo?) from comment #11)
> Let's expose some form of mozilla::pkix::CheckValidity that takes a DER
> encoding of a certificate and a time and returns Success if the certificate
> is valid at that time or indicates an invalid encoding, not yet valid,
> expired, etc.

Alternatively (preferably, IMO), add an /*optional out*/ Result* endEntityTimeErrorCode paraemter to BuildCertChain. The value would be Success if there is no time-related error in the end-entity certificate, or an error code otherwise. If the only error found is the time-related error code, then BuildCertChain should return endEntityTimeErrorCode.
Comment on attachment 8562773 [details] [diff] [review]
bug1097622_part1_INVALID_TIME-instead-of-EXPIRED_CERTIFICATE-invalid-times_v1.patch

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

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +146,5 @@
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE:
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA:
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE:
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE:
> +    case mozilla::pkix::MOZILLA_PKIX_ERROR_ISSUER_INVALID_TIME:

Alphabetical order?

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +312,5 @@
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE: return 13;
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE: return 14;
>      case mozilla::pkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE:
>        return 15;
> +    case SEC_ERROR_INVALID_TIME:                       return 16;

I suggest you don't bother aligning this.

@@ +353,5 @@
>        errorCodeTrust = defaultErrorCodeToReport;
>  
>        SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, now, false);
>        if (validity == secCertTimeUndetermined) {
> +        if (PORT_GetError() != 0) {

I think it's better to just do everything inside mozilla::pkix, instead of relying on changes to NSS.

::: security/pkix/lib/pkixbuild.cpp
@@ +97,5 @@
>      newResult = Result::ERROR_EXPIRED_ISSUER_CERTIFICATE;
>    } else if (newResult == Result::ERROR_NOT_YET_VALID_CERTIFICATE) {
>      newResult = Result::ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE;
> +  } else if (newResult == Result::ERROR_INVALID_DER_TIME) {
> +    newResult = Result::ERROR_ISSUER_INVALID_DER_TIME;

IMO, we don't need to have a separate error code for end-entity vs. issuer for this very rare case.

::: security/pkix/lib/pkixcheck.cpp
@@ +45,1 @@
>    }

Please use the same form as the rest of mozilla::pkix, testing one condition per if statement. The separate statements makes debugging easier, though I realize it is kind of verbose and repetitive.
Attachment #8562773 - Flags: feedback?(brian) → feedback+
+ Rebase on top of Bug 1123671
Attachment #8562770 - Attachment is obsolete: true
Attachment #8565406 - Flags: review?(dkeeler)
- Revert the |validity == secCertTimeUndetermined| changes
- Revert introduction of new error code
+ Address misc issues from Comment 14
Attachment #8562773 - Attachment is obsolete: true
Attachment #8565407 - Flags: review?(dkeeler)
+ Account for changes in Part 1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c389f199e00c
(OSX 10.6 debug looks unrelated)
Attachment #8562775 - Attachment is obsolete: true
Attachment #8565408 - Flags: review?(dkeeler)
Comment on attachment 8565407 [details] [diff] [review]
bug1097622_part1_INVALID_TIME-instead-of-EXPIRED_CERTIFICATE-invalid-times_v2.patch

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

::: security/pkix/lib/pkixcheck.cpp
@@ +46,5 @@
> +  }
> +
> +  if (der::End(validity) != Success) {
> +    return Result::ERROR_INVALID_DER_TIME;
> +  }

tiny nit: I would add a blank line after this one to be consistent
Attachment #8565407 - Flags: review?(dkeeler) → review+
Comment on attachment 8565408 [details] [diff] [review]
bug1097622_part2_pre-epoch-tests_v2.patch

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

Great - r=me.
Attachment #8565408 - Flags: review?(dkeeler) → review+
+ Address style nit
Attachment #8565407 - Attachment is obsolete: true
Attachment #8566303 - Flags: review+
You need to log in before you can comment on or make changes to this bug.