1361197 - (CVE-2018-5149) CERT_CompareName doesn't compare first RDN

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Matt Mueller]

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.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

https://nss-review.dev.mozaws.net/D301

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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.

Comment 5

[Matt Mueller]

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.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/projects/nss/rev/c7c9b58789a1

Comment 7

[Ryan Sleevi]

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).

Comment 8

[Daniel Veditz [: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?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler]]

(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

[Kai Engert [:KaiE:]]

(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 ?

Comment 11

[Kai Engert [:KaiE:]]

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

[Daniel Veditz [:dveditz]]

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

Comment 13

[Kai Engert [:KaiE:]]

(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?

Comment 14

[Daniel Veditz [: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).

Comment 15

[Kai Engert [: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?

Comment 16

[Mike Hommey [:glandium]]

Debian doesn't use system NSS for Firefox in stable, except in the rare occasion where stable has the right version.

Comment 17

[Kai Engert [:KaiE:]]

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

[Kai Engert [:KaiE:]]

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

[Ryan Sleevi]

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?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

ESR52 is nearly EOL and there are no more scheduled releases planned. This ship has sailed.

Comment 21

[Ryan Sleevi]

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

Comment 22

[Daniel Veditz [:dveditz]]

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