Bug 910438 (CVE-2013-5606)

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

RESOLVED FIXED in 3.15.3

Status

P1
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cviecco, Assigned: cviecco)

Tracking

({sec-high})

trunk
3.15.3
sec-high
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr17 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 affected)

Details

(Whiteboard: Doesn't affect Firefox, but maybe other NSS-using products)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
Created attachment 797073 [details] [diff] [review]
Return fail if error log is not empty

(this is against mozilla central)
(Assignee)

Comment 2

5 years ago
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.
Attachment #797073 - Flags: review?(emaldona)

Comment 3

5 years ago
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.
Attachment #797073 - Flags: review?(rrelyea)
(Assignee)

Comment 4

5 years ago
(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
(Assignee)

Comment 5

5 years ago
(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
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Updated

5 years ago
Attachment #797073 - Flags: review?(emaldona) → review?(brian)
(Assignee)

Updated

5 years ago
Blocks: 912155

Comment 7

5 years ago
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
Attachment #797073 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 8

5 years ago
Created attachment 800219 [details] [diff] [review]
fix-Cert_verifycert

Thanks for the review robert. Here is the updated patch, keeping the r+ from previous review.
Attachment #797073 - Attachment is obsolete: true
Attachment #797073 - Flags: review?(brian)
Attachment #800219 - Flags: review+
Assignee: nobody → cviecco
Status: NEW → ASSIGNED
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.
Attachment #800219 - Flags: review+
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.
Flags: needinfo?(dveditz)
Keywords: sec-high
Priority: -- → P1
Whiteboard: [needs CVE?]
Target Milestone: --- → 3.15.3
Summary: CERT_VerifyCert return value depends on verifylog existence on keyusage failure → CERT_VerifyCert returns SECSuccess (saying certificate is good) even for bad certificates, when the CERTVerifyLog log parameter is given
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.
Flags: needinfo?(cviecco)
(Assignee)

Comment 12

5 years ago
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.
Flags: needinfo?(cviecco)
http://hg.mozilla.org/projects/nss/rev/d29898e0981c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 14

5 years ago
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".

Updated

5 years ago
Blocks: 935959
status-b2g18: --- → affected
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: --- → affected
status-b2g18: affected → wontfix
status-b2g-v1.1hd: affected → wontfix
status-firefox-esr17: --- → wontfix
Flags: needinfo?(dveditz)
Whiteboard: [needs CVE?] → Doesn't affect Firefox, but maybe other NSS-using products
CVE-2013-5606
Alias: CVE-2013-5606
You need to log in before you can comment on or make changes to this bug.