Closed
Bug 910438
(CVE-2013-5606)
Opened 11 years ago
Closed 11 years ago
CERT_VerifyCert returns SECSuccess (saying certificate is good) even for bad certificates, when the CERTVerifyLog log parameter is given
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(firefox-esr17 wontfix, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 affected)
RESOLVED
FIXED
3.15.3
People
(Reporter: cviecco, Assigned: cviecco)
References
Details
(Keywords: sec-high, Whiteboard: Doesn't affect Firefox, but maybe other NSS-using products)
Attachments
(1 file, 1 obsolete file)
724 bytes,
patch
|
cviecco
:
review+
briansmith
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
(this is against mozilla central)
Assignee | ||
Comment 2•11 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•11 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•11 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•11 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•11 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•11 years ago
|
Attachment #797073 -
Flags: review?(emaldona) → review?(brian)
Comment 7•11 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•11 years ago
|
||
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+
Updated•11 years ago
|
Assignee: nobody → cviecco
Status: NEW → ASSIGNED
Comment 9•11 years ago
|
||
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+
Comment 10•11 years ago
|
||
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
Updated•11 years ago
|
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
Comment 11•11 years ago
|
||
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•11 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)
Comment 13•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 14•11 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•11 years ago
|
Updated•11 years ago
|
status-firefox-esr17:
--- → wontfix
Flags: needinfo?(dveditz)
Whiteboard: [needs CVE?] → Doesn't affect Firefox, but maybe other NSS-using products
You need to log in
before you can comment on or make changes to this bug.
Description
•