903885 - (CVE-2014-1492) Hostname matching code violates RFC 6125 for IDNA

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Christian Heimes]

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

Comment 1

[Christian Heimes]

I found the issue while I was comparing wildcard matching rules of browsers for Python issue http://bugs.python.org/issue17997

Comment 2

[Daniel Veditz [:dveditz]]

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.

Comment 3

[Christian Heimes]

Attachment: rfc6125_ace.patch

Comment 4

[Christian Heimes]

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?

Comment 5

[Christian Heimes]

What's the status on this issue? Firefox 24 has been released without a fix.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 7

[Christian Heimes]

Thank you very much. I'm not familiar with your release schedule and release policy, hence my question.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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 9

[Daniel Veditz [:dveditz]]

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

This contribution seems to have stalled. Can you review this, Bob?

Comment 10

[Kai Engert [:KaiE:]]

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

Comment 11

[Kai Engert [:KaiE:]]

(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

[Kai Engert [:KaiE:]]

Attachment: patch v2

Christian, do you agree with this change?

Comment 13

[Christian Heimes]

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.

Comment 14

[Christian Heimes]

Please request a CVE number for this issue.

Comment 15

[Kai Engert [:KaiE:]]

(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

[Kai Engert [:KaiE:]]

Comment on attachment 8380173 [details] [diff] [review]
patch v2

r=kaie on Christian's patch with my little tweak.

Comment 17

[Kai Engert [:KaiE:]]

https://hg.mozilla.org/projects/nss/rev/15ea62260c21

Comment 18

[Al Billings [:abillings - ex-MoCo]]

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

Comment 19

[Christian Heimes]

Thanks a lot Kai and Al!

The fix has been pushed to hg by Kai. That is as public as it can get. :)

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Well, no, since no one who uses NSS is using the version with this fix yet.

Comment 21

[Wan-Teh Chang]

Attachment: 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--".

Comment 22

[Wan-Teh Chang]

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

[Kai Engert [:KaiE:]]

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.

Comment 24

[Kai Engert [:KaiE:]]

(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

[Kai Engert [:KaiE:]]

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

Comment 26

[Kai Engert [:KaiE:]]

Attachment: 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.

Comment 27

[Kai Engert [:KaiE:]]

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

Comment 28

[Wan-Teh Chang]

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.

Comment 29

[Kai Engert [:KaiE:]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

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?

Comment 32

[Christian Heimes]

I'll try to review the changes tomorrow. Thanks!

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi Christian - we're getting close to release. If you could review those changes soon, it would be a big help. Thank you.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

NSS was updated to version 3.16.2 across all supported branches in bug 1020695.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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