699874 - Certificate overrides do not work correctly when using libpkix mode of PSM

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect)

Description

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

The problem is explained in bug 592978 and bug 640892. Basically, libpkix's support for returning error logs (CERTVerifyLog) is incomplete or worse. Instead of fixing libpkix's verify log support, I am going to attempt to change PSM's cert override logic to not require the error log.

Here is the current code:
http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSIOLayer.cpp#3412

Note that CERT_VerifyCertificate and CERT_PKIXVerifyCert can never put SSL_ERROR_BAD_CERT_DOMAIN in the verify log, so that case is dead code. Only CERT_VerifyCertName can return SSL_ERROR_BAD_CERT_DOMAIN, and we already handle that case earlier in the function by setting the ERROR_MISMATCH flag.

Also, we can check the validity time for the EE certificate with CERT_CheckCertValidTimes(cert, PR_Now(), PR_FALSE). This allows us to set the ERROR_TIME flag when necessary.

That means that, if we could tell CERT_PKIXVerifyCert to ignore the validity time of the EE certificate, then we would be able to use the result of PORT_GetError() after CERT_PKIXVerifyCert to determine whether to set the ERROR_UNTRUSTED flag.

So, if we add a new parameter cert_pi_ignoreEndEntityCertTimes parameter to CERT_PKIXVerifyCert, we could avoid needing to use the cert error log at all.

Besides being much simpler to get implemented, this would have positive performance impact, especially if/when we remove the redundant call to CERT_PKIXVerifyCert in our bad cert handler.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: implementaiton v1

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: fix-time-check pkix v2

assumes the psm-chainvalidate patch has been applied.

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: fix-time-check pkix v2.1

assumes the psm callback interface has been applied.

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: fix-time-check pkix v2.2

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: fix-time-check pkix v2.3

Assume the psm chainvalidate interface patch has been applied.

Comment 6

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

Comment on attachment 676291 [details] [diff] [review]
fix-time-check pkix v2.3

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

I'm operating on the assumption that you're going to rebase this on top of bug 813418. Let me know if that's not the case.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +531,5 @@
> +    //workaround to time revocation in addtion of other checks..
> +    if (srv != SECSuccess) {
> +      nsCERTValInParamWrapper pkixInParams2;
> +      PRTime notBefore, notAfter;
> +      srv = CERT_GetCertTimes(cert, &notBefore, &notAfter);

Check return value. You can treat this as a non-recoverable error.

@@ +533,5 @@
> +      nsCERTValInParamWrapper pkixInParams2;
> +      PRTime notBefore, notAfter;
> +      srv = CERT_GetCertTimes(cert, &notBefore, &notAfter);
> +
> +      if (notAfter<PR_Now()) {

Instead of calling PR_Now() again, you should use the value of PR_Now() from the original cert verification.

@@ +541,5 @@
> +        collected_errors |= nsICertOverrideService::ERROR_TIME;
> +        errorCodeExpired = SEC_ERROR_EXPIRED_CERTIFICATE;
> +
> +        pkixParamsToUse = &pkixInParams2;
> +        pkixInParams2.Construct(*survivingParams, &callbackContainer,notAfter-1);

Why notAfter - 1, instead of notAfter? A certificate is still valid at its notAfter time.

What happens with revocation checking when we pass an old time? Presumably, the OCSP responder and/or the NSS OCSP cache is always going to return current information. Are the time checks on those OCSP responses checked against the time passed to CERT_PKIXVerifyCert, or are they compared to PR_Now()?

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: fixing time check via verified log

assumes centralized verification has landed

Comment 8

[Camilo Viecco (:cviecco)]

Since FF 31 libpkix verification is not used by default, starting 33 is is not even used. Because of this this bug wont be fixed.