910438 - (CVE-2013-5606) CERT_VerifyCert returns SECSuccess (saying certificate is good) even for bad certificates, when the CERTVerifyLog log parameter is given

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Camilo Viecco (:cviecco)]

Calling Cert_VerifyCert on a trusted certificate with incompatible keyusages for the usage returns success if the verifylog parameter is not null and failure if the verifylog parameter is not null.

I would prefer to fail always on bad key usages. The issue is on:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/certvfy.c#1314
notice that if the certificate is trusted it will always exit... but
on a bad key usage with no log, the function would have terminated earlier with fail on:
http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/certvfy.c#1303

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: Return fail if error log is not empty

(this is against mozilla central)

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 797073 [details] [diff] [review]
Return fail if error log is not empty

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

Found this one while trying some psm layer testing. This is the least intrusive change that keeps the same error sematics (I think).

To reproduce the bad behavior: call CERT_VerifyCert with a cert with an invalid key usage for the usage requested and a null log. The call will correctly return SECError. Now call it again with the same input parameters, except for a valid CERTVerifyLog, the call will now return SECSuccess.

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 797073 [details] [diff] [review]
Return fail if error log is not empty

Bob might know more about the details and what to do.

Comment 4

[Camilo Viecco (:cviecco)]

(In reply to Camilo Viecco (:cviecco) from comment #0)
> Calling Cert_VerifyCert on a trusted certificate with incompatible keyusages
> for the usage returns success if the verifylog parameter is not null and
> failure if the verifylog parameter is not null.
This should have been:
It returns success when the verifylog parameter is not null and success when the verifylog
parameter is null.

> 
> I would prefer to fail always on bad key usages. The issue is on:
> http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> certvfy.c#1314
> notice that if the certificate is trusted it will always exit... but
> on a bad key usage with no log, the function would have terminated earlier
> with fail on:
> http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> certvfy.c#1303

Comment 5

[Camilo Viecco (:cviecco)]

(In reply to Camilo Viecco (:cviecco) from comment #4)
> (In reply to Camilo Viecco (:cviecco) from comment #0)
> > Calling Cert_VerifyCert on a trusted certificate with incompatible keyusages
> > for the usage returns success if the verifylog parameter is not null and
> > failure if the verifylog parameter is not null.
> This should have been:
> It returns sucess when the verifylog parameter is not null and failure when
> the verifylog
> parameter is null.


> 
> > 
> > I would prefer to fail always on bad key usages. The issue is on:
> > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> > certvfy.c#1314
> > notice that if the certificate is trusted it will always exit... but
> > on a bad key usage with no log, the function would have terminated earlier
> > with fail on:
> > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> > certvfy.c#1303

Comment 6

[Camilo Viecco (:cviecco)]

(In reply to Camilo Viecco (:cviecco) from comment #4)
> (In reply to Camilo Viecco (:cviecco) from comment #0)
> > Calling Cert_VerifyCert on a trusted certificate with incompatible keyusages
> > for the usage returns success if the verifylog parameter is not null and
> > failure if the verifylog parameter is not null.
> This should have been:
> It returns success when the verifylog parameter is not null and success when
> the verifylog
> parameter is null.


I am still unable to express myself correctly.
null verifylog on bad cert-> return fail
non null verify log on bad cert-> return success.


> 
> > 
> > I would prefer to fail always on bad key usages. The issue is on:
> > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> > certvfy.c#1314
> > notice that if the certificate is trusted it will always exit... but
> > on a bad key usage with no log, the function would have terminated earlier
> > with fail on:
> > http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/certhigh/
> > certvfy.c#1303

Comment 7

[Robert Relyea]

Comment on attachment 797073 [details] [diff] [review]
Return fail if error log is not empty

r- for the following block:

-    } else if (trusted) {
-	goto winner;
+    } else {
+      if (trusted) {
+        goto winner;
+      }
     }

The two code fragments are semantically identical and the former conforms to the style within the file.

r+ for the following block:

 winner:
+    if (log && log->head) {
+      return SECFailure;
+    }

VerifyCertificate() handles the given case by always checking all usages no matter whether we are loggin or not.

bob

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: fix-Cert_verifycert

Thanks for the review robert. Here is the updated patch, keeping the r+ from previous review.

Comment 9

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

Comment on attachment 800219 [details] [diff] [review]
fix-Cert_verifycert

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

r+ with requested change made.

This bug is more serious than I originally realized.

I would expect, and I think that other people would expect, that the log parameter should not change the error code returned from PR_GetError() when the function returns SECFailure. Hoewver, with the current implementation, the error code returned from PR_GetError() will be the last error encountered when the log parameter is provided, while PR_GetError() will return the error code for the first error encountered when the log parameter is not provided. Please file a follow-up bug to correct that. I think the solution may be as simple as calling PORT_SetError(log->head->error). Until that bug is fixed and verified, Gecko must never never pass the log parameter to CERT_VerifyLog, to ensure that we get consistent results; the *only* place where we should pass in a verifyLog is in the existing cert error override code, which has been using CERT_VerifyCertificate with an error log since forever.

::: security/nss/lib/certhigh/certvfy.c
@@ +1339,5 @@
>  	    }
>  	}
>      }
>  
>  winner:

Please rename this label to "done:" as "winner" implies success, and (as this bug shows), it isn't necessarily the case that we've succeeded by this point.

Comment 10

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

dveditz, I think this is a CVE-worthy bug. AFAICT, it doesn't affect current or recent versions of Firefox but it potentially has a big impact on other products using this CERT_VerifyCert function.

Comment 11

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

Camilo, please double-check that we never call CERT_VerifyCert with a verify log in any of our products, including nightly, aurora, beta, thunderbird, and b2g.

Comment 12

[Camilo Viecco (:cviecco)]

We actually call it, but we assume that the return value is failure (we dont use the return value), this is inside the cert-override code.

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#492

The other 9 places where Certverifier is called (which calls CERT_VerifyCert) is is called with a null verifylog.

Comment 13

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

http://hg.mozilla.org/projects/nss/rev/d29898e0981c

Comment 14

[Wan-Teh Chang]

Comment on attachment 800219 [details] [diff] [review]
fix-Cert_verifycert

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

::: security/nss/lib/certhigh/certvfy.c
@@ +1339,5 @@
>  	    }
>  	}
>      }
>  
>  winner:

The "winner" label is clearly the opposite of the "loser" label below. Ideally the two labels should be merged into a single "done" label, and we set |rv| to the appropriate value before "goto done".

Comment 15

[Daniel Veditz [:dveditz]]

CVE-2013-5606