Closed
Bug 910438
(CVE-2013-5606)
Opened 10 years ago
Closed 10 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•10 years ago
|
||
(this is against mozilla central)
Assignee | ||
Comment 2•10 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•10 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•10 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•10 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•10 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•10 years ago
|
Attachment #797073 -
Flags: review?(emaldona) → review?(brian)
Comment 7•10 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•10 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•10 years ago
|
Assignee: nobody → cviecco
Status: NEW → ASSIGNED
Comment 9•10 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•10 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•10 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•10 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•10 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•10 years ago
|
||
http://hg.mozilla.org/projects/nss/rev/d29898e0981c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Comment 14•10 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•10 years ago
|
Updated•10 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
•