Closed
Bug 1361197
(CVE-2018-5149)
Opened 7 years ago
Closed 6 years ago
CERT_CompareName doesn't compare first RDN
Categories
(NSS :: Libraries, defect)
NSS
Libraries
Tracking
(firefox-esr52 wontfix, firefox-esr60 unaffected)
RESOLVED
FIXED
3.32
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | wontfix |
firefox-esr60 | --- | unaffected |
People
(Reporter: mattm, Assigned: ttaubert)
Details
(Keywords: sec-moderate)
CERT_CompareName does not compare the first RDNs in a CERTName, starting with: https://hg.mozilla.org/projects/nss/rev/25bbb8bdd460 The change: + if (!ardns++ || !brdns++) { + break; + } causes the algorithm to always skip the first element before doing any comparison.
Assignee | ||
Comment 1•7 years ago
|
||
Thanks for reporting! The function itself isn't used in NSS, although we export it. Firefox uses it here: http://searchfox.org/mozilla-central/rev/ae8c2e2354db652950fe0ec16983360c21857f2a/security/manager/ssl/nsNSSCertificateDB.cpp#1204 It's used when the user downloads a client cert and imports it. Looks like it could cause cert name collisions with smartcards? David, wdyt? Is this something we need to worry about?
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Flags: needinfo?(dkeeler)
Assignee | ||
Comment 2•7 years ago
|
||
Matt, can you tell us how exactly you found this issue? Did it cause any trouble for you because you're using the exported function somehow?
Flags: needinfo?(mattm)
Assignee | ||
Comment 3•7 years ago
|
||
https://nss-review.dev.mozaws.net/D301
(In reply to Tim Taubert [:ttaubert] from comment #1) > Thanks for reporting! > > The function itself isn't used in NSS, although we export it. Firefox uses > it here: > > http://searchfox.org/mozilla-central/rev/ > ae8c2e2354db652950fe0ec16983360c21857f2a/security/manager/ssl/ > nsNSSCertificateDB.cpp#1204 > > It's used when the user downloads a client cert and imports it. Looks like > it could cause cert name collisions with smartcards? David, wdyt? Is this > something we need to worry about? It's unclear what the code that calls CERT_CompareName is trying to accomplish. From my reading it's trying to avoid a nickname collision except if it finds a certificate with the same subject (which may not be the same certificate, so it's not clear why this would be useful or desirable). This seems wrong already, regardless of the correctness of CERT_CompareName. In any case, I don't think this is a security concern - we don't generally use the nickname-based APIs except sometimes for display purposes.
Flags: needinfo?(dkeeler)
Reporter | ||
Comment 5•7 years ago
|
||
A unittest on Chromium buildbots started failing, https://bugs.chromium.org/p/chromium/issues/detail?id=716594, and debugging that led me to it. CERT_CompareName is used directly in a few places in Chromium(on Linux & Chromeos): https://cs.chromium.org/search/?q=CERT_CompareName&sq=package:chromium&type=cs Also Chromium(on Linux & Chromeos) still uses the old CERT_PKIXVerifyCert which I believe uses CERT_CompareName internally.
Flags: needinfo?(mattm)
Updated•7 years ago
|
Keywords: sec-moderate
Assignee | ||
Comment 6•6 years ago
|
||
https://hg.mozilla.org/projects/nss/rev/c7c9b58789a1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.32
Updated•6 years ago
|
Group: crypto-core-security → core-security-release
Comment 7•6 years ago
|
||
Dan: 1) With NSS 3.32 out, can/should this be opened? 2) From a process point, should NSS 3.32 have assigned a CVE to this and included it in the security release notes, given comment #5 (I believe so) re: Comment #4 - The purpose of CERT_CompareName is to exercise the RFC3280/5280 name comparison algorithm - including string normalization. That is, whitespace folding, case normalization, etc. As Comment #5 mentions, this is used in the libpkix algorithm, namely PKIX_PL_X500Name_Match ( https://dxr.mozilla.org/nss/source/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c#456 ), which is used internally by pkix_pl_X5009Name_Equals ( https://dxr.mozilla.org/nss/source/nss/lib/libpkix/pkix_pl_nss/pki/pkix_pl_x500name.c#120 ) This is used in the algorithm for libpkix, namely pkix_CertSelector_DefaultMatch - https://dxr.mozilla.org/nss/source/nss/lib/libpkix/pkix/certsel/pkix_certselector.c#1097 - which calls PKIX_PL_X500Name_Match to match subject (an X500Name) against a target cert. It's also used in the reverse chain validator, pkix_NameChainingChecker_Check - https://dxr.mozilla.org/nss/source/nss/lib/libpkix/pkix/checker/pkix_namechainingchecker.c#21 It doesn't allow violating nameConstraints, thankfully (libpkix uses CERT_CheckNameSpace for that).
Flags: needinfo?(dveditz)
Comment 8•6 years ago
|
||
(In reply to Ryan Sleevi from comment #7) > 1) With NSS 3.32 out, can/should this be opened? Yes. Since it doesn't have status flags for which Firefox releases are fixed it escaped notice of our usual post-release clean-up. > 2) From a process point, should NSS 3.32 have assigned a CVE to this and > included it in the security release notes, given comment #5 (I believe so) Yes. We are out of our CVE-2017- allocation so the NSS team should request one from Mitre. If a CVE-2018- number would be OK then we could assign one. Kai: can you update the NSS 3.32 release notes with this? Or is someone else doing that these days?
Flags: needinfo?(dveditz) → needinfo?(kaie)
(In reply to Ryan Sleevi from comment #7) > re: Comment #4 - The purpose of CERT_CompareName is to exercise the > RFC3280/5280 name comparison algorithm - including string normalization. Right - I was saying it's not clear why nsNSSCertificateDB::get_default_nickname needs to use that algorithm, and that I don't think Firefox was vulnerable as a result of that specific call.
Comment 10•6 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #8) > > Kai: can you update the NSS 3.32 release notes with this? Or is someone else > doing that these days? I can do that. It seems I need to wait until we have a CVE assigned? Who can make the decision if we require a CVE-2017 or CVE-2018 ?
Flags: needinfo?(kaie) → needinfo?(dveditz)
Comment 11•6 years ago
|
||
If we edit them post release, would you agree that I send a follow up to the old 3.32 release announcement to the dev-tech-crypto list? Suggested text for the release notes? "A regression bug was fixed, which caused function CERT_CompareName to ignore the first RDN in the comparison." It would be good to mention which NSS releases had been affected by the regression. Unfortunately the commit from comment 0 and the associated bug don't have a target NSS milestone assigned, so we'd have to research which NSS release was the first to contain it.
Comment 12•6 years ago
|
||
We don't have any more 2017 CVEs in our pool: it will have to be a 2018 CVE. That's not a problem. CVE-2018-5149
Alias: CVE-2018-5149
Flags: needinfo?(dveditz)
Comment 13•6 years ago
|
||
(In reply to Kai Engert (:kaie:) from comment #11) > It would be good to mention which NSS releases had been affected by the > regression. Unfortunately the commit from comment 0 and the associated bug > don't have a target NSS milestone assigned, so we'd have to research which > NSS release was the first to contain it. The commit was made 2016-10-17. The first release from trunk after that was NSS 3.28 on 2016-12-22. This means that the Firefox 52 ESR branch has this bug. Should we get this fixed and released on the ESR 52.x branch, prior to publishing this bug?
Flags: needinfo?(dveditz)
Comment 14•6 years ago
|
||
From what David said earlier it's not a bad bug for Firefox. Do Linux distros which ship ESR-52 ship the ESR branch system NSS such that other applications might be affected, and therefore want/need this patch? If so we can take it but otherwise I wouldn't (assuming I'm understanding David correctly).
status-firefox-esr52:
--- → affected
status-firefox-esr60:
--- → unaffected
Flags: needinfo?(dveditz) → needinfo?(kaie)
Comment 15•5 years ago
|
||
Dan, thanks for your quick reply, but I had failed to see it earlier, sorry. There might be distributions which might have stayed with older versions of NSS. I see that Debian is still on FF 52.x and NSS 3.26, probably with backports. Debian ships NSS as a system library, and thus other applications could be using the same library. If we publish this fix for 3.28.x, I don't know if e.g. Debian will backport it to the 3.26.x version. Mike, as a potential consumer for such a fix, would you be interested in an official 3.28.x containing this fix, or would announcing the fix on dev-tech-crypto be sufficient?
Flags: needinfo?(kaie) → needinfo?(mh+mozilla)
Comment 16•5 years ago
|
||
Debian doesn't use system NSS for Firefox in stable, except in the rare occasion where stable has the right version.
Flags: needinfo?(mh+mozilla)
Comment 17•5 years ago
|
||
Ok. So updating the NSS version bundled with ESR 52.x won't fix the issue for apps other than Firefox on Debian. It seems that publishing the issue might be the best course of action, as it allows users of the old NSS branch to backport the fix, where necessary.
Comment 18•5 years ago
|
||
I've added a sentence to the "bugx fixed" section. https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.32_release_notes#Bugs_fixed_in_NSS_3.32 (In reply to Kai Engert (:kaie:) from comment #11) > If we edit them post release, would you agree that I send a follow up to the > old 3.32 release announcement to the dev-tech-crypto list? I think nobody has commented on this question yet.
Comment 19•5 years ago
|
||
Kai: It's unclear if Comment #11 was directed at dveditz, but it may be worth directly following-up if you're not getting a reply. It's definitely unfortunate that the process ball was dropped, but it seems like we should push to fix?
Flags: needinfo?(kaie)
Comment 20•5 years ago
|
||
ESR52 is nearly EOL and there are no more scheduled releases planned. This ship has sailed.
Flags: needinfo?(kaie)
Comment 21•5 years ago
|
||
Dan, JC: Not sure who owns triage for these, but not sure next steps to close / open the bug here. It looks like things stalled out after Comment #8
Flags: needinfo?(jjones)
Flags: needinfo?(dveditz)
QA Contact: jjones
Comment 22•5 years ago
|
||
Looks like we just need to unhide this, right? If so, jc, ping me on IRC
Flags: needinfo?(dveditz)
Updated•5 years ago
|
Group: core-security-release
Flags: needinfo?(jjones)
You need to log in
before you can comment on or make changes to this bug.
Description
•