Closed Bug 1361197 (CVE-2018-5149) Opened 3 years ago Closed 3 years ago

CERT_CompareName doesn't compare first RDN

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox-esr52 wontfix, firefox-esr60 unaffected)

RESOLVED FIXED
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.
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)
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)
(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)
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)
https://hg.mozilla.org/projects/nss/rev/c7c9b58789a1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.32
Group: crypto-core-security → core-security-release
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)
(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.
(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)
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.
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)
(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)
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).
Flags: needinfo?(dveditz) → needinfo?(kaie)
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)
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)
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.
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.
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)
ESR52 is nearly EOL and there are no more scheduled releases planned. This ship has sailed.
Flags: needinfo?(kaie)

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

Looks like we just need to unhide this, right? If so, jc, ping me on IRC

Flags: needinfo?(dveditz)
Group: core-security-release
Flags: needinfo?(jjones)
You need to log in before you can comment on or make changes to this bug.