Closed Bug 699874 Opened 11 years ago Closed 8 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla10

People

(Reporter: briansmith, Assigned: cviecco)

References

Details

Attachments

(2 files, 4 obsolete files)

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.
No longer depends on: 699884
Attached patch implementaiton v1 (obsolete) — Splinter Review
Attachment #665101 - Flags: feedback?(bsmith)
Attached patch fix-time-check pkix v2 (obsolete) — Splinter Review
assumes the psm-chainvalidate patch has been applied.
Attachment #665101 - Attachment is obsolete: true
Attachment #665101 - Flags: feedback?(bsmith)
Attached patch fix-time-check pkix v2.1 (obsolete) — Splinter Review
assumes the psm callback interface has been applied.
Attachment #676284 - Attachment is obsolete: true
Attached patch fix-time-check pkix v2.2 (obsolete) — Splinter Review
Attachment #676286 - Attachment is obsolete: true
Assume the psm chainvalidate interface patch has been applied.
Attachment #676288 - Attachment is obsolete: true
Attachment #676291 - Flags: review?(bsmith)
Assignee: bsmith → cviecco
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()?
Attachment #676291 - Flags: review?(bsmith)
assumes centralized verification has landed
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.