Bug 903885 (CVE-2014-1492)

Hostname matching code violates RFC 6125 for IDNA

RESOLVED FIXED in Firefox 29, Firefox OS v1.3

Status

NSS
Libraries
P1
normal
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Christian Heimes, Assigned: Christian Heimes, NeedInfo)

Tracking

({csectype-other, sec-moderate})

trunk
3.16
csectype-other, sec-moderate

Firefox Tracking Flags

(firefox27 wontfix, firefox28 wontfix, firefox29+ fixed, firefox30+ fixed, firefox31 fixed, firefox-esr24- fixed, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 wontfix, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [adv-main29+])

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

4 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130803192641

Steps to reproduce:

The implementation of cert_TestHostName() at http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/security/nss/lib/certdb/certdb.c#l1384 does not handle IDNA domain prefixes according to RFC 6125, section 6.4.3 "Checking of Wildcard Certificates". http://tools.ietf.org/html/rfc6125#section-6.4.3 


Actual results:

A CN or subjectAltName DNS wildcard such as 'x*.example.org' should not match a IDNA A-label like 'xn--www-una.example.org'.


Expected results:

cert_TestHostName() should not use wildcard matching if the first fragment of a hostname is an IDNA A-label. Chromium has such a special case in http://src.chromium.org/viewvc/chrome/trunk/src/net/cert/x509_certificate.cc?revision=212341#l640
(Assignee)

Comment 1

4 years ago
I found the issue while I was comparing wildcard matching rules of browsers for Python issue http://bugs.python.org/issue17997
(Assignee)

Updated

4 years ago
Summary: Hostname matching code violate RFC 6125 for IDNA → Hostname matching code violates RFC 6125 for IDNA
Assignee: nobody → nobody
Component: Security → Libraries
Product: Core → NSS
Version: unspecified → trunk
Confirming, this is indeed what the RFC says. Thankfully people generally don't get prefix wildcards like x*, they go ahead and get the full * if they're going to pay for it. In practice this may not affect that many real certs or domains.

In current NSS code x*, xn*, xn-*, and xn--* will match all IDNA labels.

Spec-compliant behavior would mean that you could _never_ have a wildcard cert match an IDNA label unless it was simply "*". Seems a bit unfair to folks with non-roman alphabets but ACE-encoding doesn't really make for workable prefixes and suffixes I guess.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csec-other, sec-moderate
(Assignee)

Comment 3

4 years ago
Created attachment 794018 [details] [diff] [review]
rfc6125_ace.patch
(Assignee)

Comment 4

4 years ago
Thanks for your confirmation, Daniel.

I agree with you. It is unlikely that many sites will be affected by the issue. In fact I haven't seen any partial wildcards like 'x*' in the wild, yet. The attached patch should take care of the issue. Do you have some sort of unit tests for hostname matching in NSS, too?
(Assignee)

Comment 5

4 years ago
What's the status on this issue? Firefox 24 has been released without a fix.
The status is that it is "new," which means it is unfixed as of yet. It is currently not assigned to a developer. Given that it was reported when Firefox 24 was well under development, it was always unlikely to be fixed there. We still need to decide how we want to address this.
(Assignee)

Comment 7

4 years ago
Thank you very much. I'm not familiar with your release schedule and release policy, hence my question.
We have a six week cycle and a "train" model of shipping. Bugs are fixed on trunk. Once every six weeks, trunk is ported to the Aurora branch. After another six weeks, the Aurora branch becomes the Beta branch. After another six weeks, the Beta branch is released as the release branch. See https://wiki.mozilla.org/Release_Management/Release_Process

For serious security issues, we'll fix it on trunk (when the bug will be resolved as "fixed") and then port it to the other branches. This is a sec-moderate issue so it is unlikely to be ported once it is a fixed without a special need.
Comment on attachment 794018 [details] [diff] [review]
rfc6125_ace.patch

This contribution seems to have stalled. Can you review this, Bob?
Attachment #794018 - Flags: review?(rrelyea)
Group: crypto-core-security

Comment 10

4 years ago
Comment on attachment 794018 [details] [diff] [review]
rfc6125_ace.patch

Christian, thank you for the patch, however, the patch needs a modification.

>+	/* RFC 6125 IDN matching */
>+	int firstace      = PORT_Strcasecmp('xn--', cn);

The result of this call will never be zero, because the common name will never be equal to 'xn--'.
If you want to limit the comparison to the first 4 characters, it should be

    int firstace      = PORT_Strncasecmp('xn--', cn, 4);

Besides that, your approach looks correct to me.
Because the other comparisons already use '!', it might be more consistent to write

    && !firstace
Attachment #794018 - Flags: review?(rrelyea) → review-

Comment 11

4 years ago
(In reply to Kai Engert (:kaie) from comment #10)
> Because the other comparisons already use '!', it might be more consistent
> to write
> 
>     && !firstace

forget that, my mistake

Comment 12

4 years ago
Created attachment 8380173 [details] [diff] [review]
patch v2

Christian, do you agree with this change?
Attachment #8380173 - Flags: review?(sites)
(Assignee)

Comment 13

4 years ago
Comment on attachment 8380173 [details] [diff] [review]
patch v2

Yes, your patch looks fine.

I'm a bit embarrassed that I didn't get strcmp() / strncmp() right in the first place.
(Assignee)

Comment 14

4 years ago
Please request a CVE number for this issue.

Comment 15

4 years ago
(In reply to Christian Heimes from comment #14)
> Please request a CVE number for this issue.

Dan, can you please help with getting a CVE number?

Comment 16

4 years ago
Comment on attachment 8380173 [details] [diff] [review]
patch v2

r=kaie on Christian's patch with my little tweak.
Attachment #8380173 - Flags: review?(sites) → review+

Comment 17

4 years ago
https://hg.mozilla.org/projects/nss/rev/15ea62260c21
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.16
(In reply to Christian Heimes from comment #14)
> Please request a CVE number for this issue.

Normally we assign a CVE number when an issue is about to be publicly disclosed in an advisory or otherwise.

I'm assigning CVE-2014-1492 to it.
Alias: CVE-2014-1492
(Assignee)

Comment 19

4 years ago
Thanks a lot Kai and Al!

The fix has been pushed to hg by Kai. That is as public as it can get. :)
Well, no, since no one who uses NSS is using the version with this fix yet.

Updated

4 years ago
Assignee: nobody → sites
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All

Comment 21

4 years ago
Created attachment 8381010 [details] [diff] [review]
v3: Use C string literal "xn--" instead of Python string literal 'xn--'

Patch v2 has two bugs.

1. It uses the Python string literal 'xn--'.

2. It still allows the pattern "x*.python.org" to match the hostname
"xn--tda.python.org".

This patch fixes the first problem by using the C string literal "xn--".
Attachment #794018 - Attachment is obsolete: true
Attachment #8381010 - Flags: review?(kaie)

Updated

4 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 22

4 years ago
Comment on attachment 8381010 [details] [diff] [review]
v3: Use C string literal "xn--" instead of Python string literal 'xn--'

Review of attachment 8381010 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certdb/certdb.c
@@ +1398,5 @@
>  	    && secondcndot - firstcndot > 1
>  	    && PORT_Strrchr(cn, '*') == wildcard
>  	    && !PORT_Strncasecmp(cn, hn, wildcard - cn)
>  	    && !PORT_Strcasecmp(firstcndot, firsthndot)
> +	    && PORT_Strncasecmp(cn, "xn--", 4)) {

To match the Chromium code that Christian cited in comment 0, I think
we can replace the last line with this:

            && (PORT_Strncasecmp(hn, "xn--", 4) || wildcard == cn)) {

The firstcndot - wildcard == 1 test above ensures that '*' is followed
by the first dot in |cn|, so the wildcard == cn test implies that
there is nothing before '*' in |cn|.

If you think this makes sense, I'll add this change to my patch. Note
that this will allow the pattern "*.python.org" to match the hostname
"xn--tda.python.org".

Comment 23

4 years ago
Comment on attachment 8381010 [details] [diff] [review]
v3: Use C string literal "xn--" instead of Python string literal 'xn--'

r=kaie
Thank you for catching my oversight and the clarifications/simplifications.
Attachment #8381010 - Flags: review?(kaie) → review+

Comment 24

4 years ago
(In reply to Wan-Teh Chang from comment #21)
> 
> 2. It still allows the pattern "x*.python.org" to match the hostname
> "xn--tda.python.org".

You're right, because the comparison is currently made against the common name, but the comparison must be done against the actual hostname...


(In reply to Wan-Teh Chang from comment #22)
>            && (PORT_Strncasecmp(hn, "xn--", 4) || wildcard == cn)) {
>
> The firstcndot - wildcard == 1 test above ensures that '*' is followed
> by the first dot in |cn|, so the wildcard == cn test implies that
> there is nothing before '*' in |cn|.
>
> If you think this makes sense, I'll add this change to my patch. 

I need to translate it into human language to be certain about it.

This means:
- if the hostname doesn't start with "xn--" (not equal) the comparison returns != 0,
  meaning the comparison results in a "true" value,
  this sub-expression will be true (second part ignored),
  and the overall "if statement expression" will allowed to become true, meaning
  a valid match.

- if the hostname starts with "xn--" (equal) the comparison returns 0,
  meaning the comparison results in a "false" value,
  meaning the hostname is IDNA,
  and the second part (after "||") will decide about the result of the expression.
  If the wildcard is found at the beginning of the common name (cn),
  then we allow the overall if-statement-expression to become true meaning a valid match.
  However, if the wildcard position differs from the beginning of the common name,
  there are characters in front of the wildcard, and we don't allow that,
  the expression will be false, and will cause the overall if-statement-expression
  to be false, and rejected as not matching.

I agree your patch makes sense.

Please feel free to add my explanation as a comment to the patch.


I might also be helpful to add some additional comments:
-	    && firstcndot  - wildcard  == 1
-	    && secondcndot - firstcndot > 1
-	    && PORT_Strrchr(cn, '*') == wildcard
+	    && firstcndot  - wildcard  == 1 /* no chars between * and . */
+	    && secondcndot - firstcndot > 1 /* not .. */
+	    && PORT_Strrchr(cn, '*') == wildcard /* only one wildcard in cn */


> Note
> that this will allow the pattern "*.python.org" to match the hostname
> "xn--tda.python.org".

Yes, and I understand Dan already agreed this makes sense, when he said in comment 2:
> Spec-compliant behavior would mean that you could _never_ have a wildcard cert match 
> an IDNA label unless it was simply "*".

r=kaie on comment 22 and a new patch with those changes.

Comment 25

4 years ago
(In reply to Kai Engert (:kaie) from comment #24)
> I need to translate it into human language to be certain about it.
> ...
> I agree your patch makes sense.
> Please feel free to add my explanation as a comment to the patch.

A shorter version of my longer explanation, suitable for a code comment:

  /* If hn starts with xn--, then cn must start with wildcard */

Since our builds are currently broken, I'm going to check your changes in.

Updated

4 years ago
Attachment #8381010 - Attachment description: Use C string literal "xn--" instead of Python string literal 'xn--' → v3: Use C string literal "xn--" instead of Python string literal 'xn--'

Comment 26

4 years ago
Created attachment 8381492 [details] [diff] [review]
patch v4

This is Wan-Teh's patch v3, an incremental patch on top of v2.

It contains the additional change that Wan-Teh proposed in comment 22.

My changes on top of those are limited to comments.
Attachment #8381492 - Flags: review+

Comment 27

4 years ago
https://hg.mozilla.org/projects/nss/rev/2ffa40a3ff55

Comment 28

4 years ago
Comment on attachment 8381492 [details] [diff] [review]
patch v4

Review of attachment 8381492 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc.

::: lib/certdb/certdb.c
@@ +1395,3 @@
>  	 */
>  	if (wildcard && secondcndot && secondcndot[1] && firsthndot 
> +	    && firstcndot  - wildcard  == 1 /* no chars between * and . */

Nit: it may be clearer to say "wildcard is the last char in the first component". You can omit "the" to shorten this comment.

@@ +1395,4 @@
>  	 */
>  	if (wildcard && secondcndot && secondcndot[1] && firsthndot 
> +	    && firstcndot  - wildcard  == 1 /* no chars between * and . */
> +	    && secondcndot - firstcndot > 1 /* not .. */

This comment should say "the second component is non-empty".

By the way, this test and the secondcndot[1] test (which
ensures the third component is non-empty) don't seem necessary
to me.
Attachment #8381492 - Flags: review+

Updated

4 years ago
Attachment #8381010 - Attachment is obsolete: true
status-b2g18: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
status-b2g-v1.2: --- → wontfix
status-b2g-v1.3: --- → affected
status-b2g-v1.4: --- → affected
status-firefox27: --- → wontfix
status-firefox28: --- → wontfix
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox-esr24: --- → affected
tracking-firefox29: --- → +
tracking-firefox30: --- → +
tracking-firefox-esr24: --- → -

Comment 29

4 years ago
(In reply to Wan-Teh Chang from comment #28)

I've landed the comment changes you have requested:
https://hg.mozilla.org/projects/nss/rev/709d4e597979


> By the way, this test and the secondcndot[1] test (which
> ensures the third component is non-empty) don't seem necessary
> to me.

Sorry, I don't have time to think about this. If it's important, let's file a follow-up bug, or I'd appreciate if someone else can ponder and confirm.

Updated

4 years ago
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Group: crypto-core-security
status-b2g-v1.3T: --- → affected
NSS 3.16 is on 29+. Not sure if this bug warrants updating b2g28 (3.15.5) or esr24 (3.15.4) to version 3.l6 as well.
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: --- → fixed
status-firefox29: affected → fixed
status-firefox30: affected → fixed
status-firefox31: --- → fixed
status-firefox-esr24: affected → wontfix
Christian, have you had an opportunity to review the code changes? I'd like to know what your confidence is. Do you feel that we've addressed your concerns?
Flags: needinfo?(sites)
(Assignee)

Comment 32

4 years ago
I'll try to review the changes tomorrow. Thanks!
Flags: needinfo?(sites)
Whiteboard: [adv-main29+]
Hi Christian - we're getting close to release. If you could review those changes soon, it would be a big help. Thank you.
Flags: needinfo?(sites)
status-b2g-v1.2: wontfix → affected
status-b2g-v1.2: affected → wontfix
NSS was updated to version 3.16.2 across all supported branches in bug 1020695.
status-b2g-v1.3: affected → fixed
status-b2g-v1.3T: affected → fixed
status-firefox-esr24: wontfix → fixed
Is it OK to open up this bug? It is sec-moderate and it is already publicly-disclosed (e.g. https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2014-1492).
Flags: needinfo?(dveditz)
Group: core-security
Flags: needinfo?(dveditz)
You need to log in before you can comment on or make changes to this bug.