Closed Bug 903885 (CVE-2014-1492) Opened 11 years ago Closed 10 years ago

Hostname matching code violates RFC 6125 for IDNA

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

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

RESOLVED FIXED
Tracking Status
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

People

(Reporter: sites, Assigned: sites, NeedInfo)

Details

(Keywords: csectype-other, sec-moderate, Whiteboard: [adv-main29+])

Attachments

(2 files, 2 obsolete files)

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
I found the issue while I was comparing wildcard matching rules of browsers for Python issue http://bugs.python.org/issue17997
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
Attached patch rfc6125_ace.patch (obsolete) — Splinter Review
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?
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.
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 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-
(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
Attached patch patch v2Splinter Review
Christian, do you agree with this change?
Attachment #8380173 - Flags: review?(sites)
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.
Please request a CVE number for this issue.
(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 on attachment 8380173 [details] [diff] [review]
patch v2

r=kaie on Christian's patch with my little tweak.
Attachment #8380173 - Flags: review?(sites) → review+
https://hg.mozilla.org/projects/nss/rev/15ea62260c21
Status: NEW → RESOLVED
Closed: 10 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
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.
Assignee: nobody → sites
OS: Linux → All
Priority: -- → P1
Hardware: x86_64 → All
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 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+
(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.
(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.
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--'
Attached patch patch v4Splinter Review
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 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+
Attachment #8381010 - Attachment is obsolete: true
(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.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Group: crypto-core-security
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.
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)
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)
NSS was updated to version 3.16.2 across all supported branches in bug 1020695.
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.