578697 - (CVE-2010-3170) Browser Wildcard Certificate Validation Issue

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Not yet certain if this is an NSS issue or PSM issue.  Maybe both?
I'm intentionally giving this bug a higher severity than it probably 
deserves, given that CAs trusted by Mozilla won't issue such certs.

Received an email today from Richard Moore <[e-mail]>.  Text as follows:

> This is an advance notification of an advisory my company will be
> releasing at some point in the future. We aim to work with vendors to
> achieve simultaneous disclosure, so any comments would be appreciated.
> Once I've dug out the various security contacts for the browsers
> concerned we will notify them privately as well. Please treat this
> information as confidential for now.

> Westpoint Security Advisory
> ---------------------------
> 
> Title:        Multiple Browser Wildcard Cerficate Validation Weakness
> Risk Rating:  Low
> Author:       Richard Moore <[e-mail]>
> Test Cases:   Simon Ward <[e-mail]>
> Date:         14 July 2010
> Advisory ID#: wp-10-0001
> URL:          http://www.westpoint.ltd.uk/advisories/wp-10-0001.txt
> CVE:          not yet assigned
> 
> Details
> -------
> 
> RFC 2818 covers the requirements for matching CNs and subjectAltNames in
> order to establish valid SSL connections. It first discusses CNs that are
> for hostnames, and the rules for wildcards in this case. The next paragraph
> in the RFC then discusses CNs that are IP addresses:
> 
> 'In some cases, the URI is specified as an IP address rather than a
> hostname. In this case, the iPAddress subjectAltName must be present
> in the certificate and must exactly match the IP in the URI.'
> 
> The intention of the RFC is clearly that you should not be able to use
> wildcards with IP addresses (in order to avoid the ability to perform
> man-in-the-middle attacks). Unfortunately our testing showed that this
> rule is not adhered to by some browsers.
> 
> We created a certificate with the CN '*.168.3.48' this meets the various
> rules for wildcards in CNs, but should be treated as invalid since it is
> not a hostname. We then observed the errors reported by browsers when
> connecting to an https server using this certificate run on IP address
> 192.168.3.48.
> 
> We imported the test CA used to sign the certifcate in order to perform
> the test.
> 
> The results we saw were as follows:
> 
> IE6     
>         Regarded the IP address as matching the CN (VULNERABLE)
>         
> IE7     
>         Regarded the IP address as matching the CN (VULNERABLE)
> 
> Firefox 3.6.6
>         Regarded the IP address as matching the CN (VULNERABLE)
>         
> Chrome  
>         Regarded the IP address as matching the CN (VULNERABLE)
>         
> Opera   
>         Regarded the IP address did not match the CN (NOT VULNERABLE)
> 
> Mitigating Factors
> ------------------
> 
> Obviously a good CA should refuse to issue a certificate with the CN as
> indicated, however there only need be one CA to issue one in error for
> this issue to result in the user getting no warning at all and being
> vulnerable to MITM.
> 
> The rules for hostname matching mean that only the first octet of the
> IP address can contain a wildcard. This means that you must be able to
> control a server that matches the remainder of the IP address of your
> target which reduces the risk of this attack being used dramatically.
> 
> Impact
> ------
> 
> If exploited then a MITM attack can be performed allowing the guarantees
> SSL provides to be circumvented.
> 
> Timeline
> --------
> 
> 14 July 2010    Limited disclosure to browser developers.

Comment 1

[Kai Engert (:KaiE:)]

After this gets fixed, we should have a brainstorming session about other potential combinations of creative wildcard usage and test our behaviour.

Comment 2

[Kai Engert (:KaiE:)]

I set up a test site on my server.

- create a new firefox profile for testing purposes (firefox -P)
- go to http://kuix.de/ca/nss-test-ca.php
- install and trust the CA for web sites

Now you can perform the actual tests.

- go to https://kuix.de:9447

  cert not accepted. We're fine. Firefox reports:

    kuix.de:9447 uses an invalid security certificate.
    The certificate is only valid for *.106.189.147
    (Error code: ssl_error_bad_cert_domain)

- go to https://87.106.189.147:9447/

  cert is accepted

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

OK.  NSS function CERT_VerifyCertName, which is the function called by 
PSM to see if a host name (or IP address) given by PSM matches a cert, has two major code paths, one for Subject Alt Names, and one for Subject Common Names.

See 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certdb.c&rev=1.104&mark=1851,1856,1877,1881-1883#1851

The path for SubjectAltNames has an explicit test to see if the desired 
"host name" passed in by the caller from the "URI" is really an IP address. 
It calls cert_VerifySubjectAltName, which calls PR_StringToNetAddr to make that determination.  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/certdb.c&rev=1.104&mark=1510-1511,1537#1510
It will not treat the caller's name as a DNS name if this function says it is an IP address.

But when the certificate does not have a subjectAltName, in the path for a Subject Common Name, there is no test to see if the name passed in by the caller is an IP address.  In this case, CERT_VerifyCertName calls CERT_GetCommonName to get the CommonName, then calls cert_TestHostName to test it.  See 
https://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.c#1446
This new function makes no check for IP addresses (v4 or v6).  It should.  I'm adding the co-contributor of that code to the cc list of this bug. 

I can imagine multiple solutions. But I think the simplest and best is this:
cert_TestHostName can detect an IP address, and if found, can call
     PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN); 
     return SECFailure; 
simply refusing to match an IP address against ANY string in a common name. That's what several other browsers do now.  I think it's the right thing.

Comment 4

[Kai Engert (:KaiE:)]

NSS has two separate functions that test the cert's embedded identifier against the active network target:
- cert_TestHostName
- cert_VerifySubjectAltName

cert_VerifySubjectAltName has some logic for looking at IP addresses.

But cert_TestHostName doesn't bother at all.

Shouldn't we have a single hostname verification function, that gets used for both "common names" and "alt subject names"?

Comment 5

[Kai Engert (:KaiE:)]

This is a NSS bug.

The culprit is cert_TestHostName

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

One undesirable characteristic of my proposed solution in comment 3 is that 
it would cause each host name passed in by the caller (PSM) to be checked at 
least twice, and maybe multiple times, when the cert has an SAN with a DNS
name.  This is because cert_VerifySubjectAltName calls cert_TestHostName once 
for each DNS name entry in the SAN, after having done the IP address check 
once itself.  I agree.  That's bad.  

The solution is to NOT check the host name to see if it's an IP address INSIDE
cert_TestHostName, but rather to make it a requirement of the callers of 
cert_TestHostName that they must not call it with an IP address for the host 
name.  cert_VerifySubjectAltName already takes care of this for the SAN path.
It's easy for CERT_VerifyCertName to handle this for the CN path.

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v1

here's a proposed fix

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative patch B, v0.1

Here's the first of several alternatives I will propose -- in addition to 
Kai's alternative(s).

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Alternative patch C, v0.1

Yet another.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

I have not yet looked at Kai's first patch.

My Alternative B is closest in behavior to existing behavior. 
If a cert subject common name bears a string that looks like a dotted 
decimal IP address, and it exactly matches the dotted decimal IP address 
given in the URI,  it will be accepted as a match.  This is least likely 
to break backward compatibility.  It is also losing favor with the IETF 
and with other browsers.  

My alternative C is similar, but it will NOT match any IP address string 
in a URL with an IP address string in a common name in a cert.  It will ONLY
match an IP address in the URI with an IP address in a cert SubjectAltName.
This is what some other browsers do, but is quite different from what NSS 
has done in the past and is MUCH more likely to be viewed as a break of 
backward binary compatibility.  

Both of these patches modify CERT_VerifyCertName.  The reason for this is 
explained in comment 6.  I will read Kai's patch later this evening.

Comment 11

[Wan-Teh Chang]

Comment on attachment 457470 [details] [diff] [review]
Alternative patch C, v0.1

r=wtc.  I support this approach.

>+        PRBool isIPaddr = cert_IsIPAddr(hn);


Nit: the variable name should be isIPAddr, for consistecy
with how "IPAddr" is spelled in the function name.

Comment 12

[Kai Engert (:KaiE:)]

I thinking fixing bugs and dropping features should be done in separate steps, or you make it harder for consumers of NSS to accept new NSS releases as security fixes.

In particular to this functionality, I had recently asked about it on the newsgroup, and Nelson had confirmed the practice of having a cert for an IP address is an allowed practice.
http://groups.google.com/group/mozilla.dev.security.policy/msg/88cc3b45d6b33bda

The discussion about dropping support for IP address in common names is happening in bug 553754 and seems to be open, someone has argued that some users might rely on that feature.


If you decide to really drop support for IP addresses, I propose to announce your decision in bug 553754.

This change will affect stable branches of Mozilla products, who might prefer a bugfix without the feature drop?

Comment 13

[Daniel Veditz [:dveditz]]

If we start rejecting IP addresses as cert matches would it be an error that users (or PSM) could set up a cert override for? Or would it be a hard error? I think SSL_ERROR_BAD_CERT_DOMAIN is the standard name mismatch error and assume it would be overrideable, in which case I'm fine taking this (version B or C) on the stable branches. version C seems best going forward, might as well only change it once.

From Kai's patch
  /* replace all wildcards with zero */

Are multiple wildcards allowed? I know we used to allow all kinds of crazy RegExp stuff but I thought the spec required the wildcard to be the leftmost token.

Comment 14

[Daniel Veditz [:dveditz]]

Taking a wild-assed guess at sg-severity "moderate" since it allows SSL spoofing (normally "high") but only in the case of stupid sites using raw IP addresses as links. And where they're getting the CA that issues those I don't know. Presumably the target site uses a corporate CA but I don't know where the attacker gets a working wildcard cert that the victim will accept.

Comment 15

[Kaspar]

(In reply to comment #3)
> But when the certificate does not have a subjectAltName, in the path for a
> Subject Common Name, there is no test to see if the name passed in by the
> caller is an IP address.  In this case, CERT_VerifyCertName calls
> CERT_GetCommonName to get the CommonName, then calls cert_TestHostName to test
> it.  See 
> https://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.c#1446
> This new function makes no check for IP addresses (v4 or v6).  It should.  I'm
> adding the co-contributor of that code to the cc list of this bug.

For the sake of completeness: cert_TestHostName isn't really a new function, it was already added in 2002 (bug 103752). It was modified last year, though, when we limited the "power" of the wildcard character (bug 159483) so that it would no longer match a dot etc. With the old behavior (which can also be restored through the NSS_USE_SHEXP_IN_CERT_NAME environment variable), things like CN=*, or even CN=*.*.*.* have always been working for IP addresses, too.

(In reply to comment #12)
> I thinking fixing bugs and dropping features should be done in separate steps,
> or you make it harder for consumers of NSS to accept new NSS releases as
> security fixes.

I agree, and would therefore suggest to go with alternative B, for the time being.

Comment 16

[Wan-Teh Chang]

Comment on attachment 457469 [details] [diff] [review]
Alternative patch B, v0.1

r=wtc.

>+static PRBool
>+cert_IsIPAddr(const char *hn)
>+{
>+    PRBool            isIPaddr       = PR_FALSE;
>+    PRNetAddr         netAddr;
>+    isIPaddr = (PR_SUCCESS == PR_StringToNetAddr(hn, &netAddr));
>+    return isIPaddr;
>+}

We don't need the isIPaddr local variable.  We can just do
    return (PR_SUCCESS == PR_StringToNetAddr(hn, &netAddr));

PRBool is a typedef for 'int', so no type casting is necessary.

>+        PRBool isIPaddr = cert_IsIPAddr(hn);

Nit: isIPaddr => isIPAddr

Comment 17

[Wan-Teh Chang]

Comment on attachment 457469 [details] [diff] [review]
Alternative patch B, v0.1

Re: the nit on the variable name isIPaddr:

I see that isIPaddr comes from the existing function
cert_VerifySubjectAltName.  In that case, we should
just use the same name for consistency.

It is a shame that CERT_VerifyCertName may call
PR_StringToNetAddr twice.  But I don't see a clean
way to call PR_StringToNetAddr only once.  Since
Common Name is being deprecated for certificate
name matching, I think adding a second
PR_StringToNetAddr call for Common Name is fine.

Comment 18

[[e-mail]]

(In reply to comment #13)
> Are multiple wildcards allowed? I know we used to allow all kinds of crazy
> RegExp stuff but I thought the spec required the wildcard to be the leftmost
> token.

Multiple wildcards are not allowed, furthermore the wildcard must be part of the first element. This is one of the reasons I regarded the risk as low in my original report. In code that uses the old style shell glob the risk is much higher, but fortunately we are no longer in that situation.

Comment 19

[[e-mail]]

(In reply to comment #14)
> Taking a wild-**** guess at sg-severity "moderate" since it allows SSL
> spoofing (normally "high") but only in the case of stupid sites using raw IP
> addresses as links. And where they're getting the CA that issues those I don't
> know. Presumably the target site uses a corporate CA but I don't know where the
> attacker gets a working wildcard cert that the victim will accept.

In my day job, I do see client sites that use raw IP addresses as links (and use SSL) - note i'm not saying this is sensible. I have little faith that the CA validation is stricter than that in browsers (and we can see the result there), which is one of the reasons I think this issue is important. That said, given that I haven't actually tried to get a CA to sign such a certificate maybe they are better than I fear. As I said in comment #18 this is why I rated the risk as low (though I suspect this might be optimistic).

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

In today's weekly NSS team meeting, we decided not to disallow IP addresses
in cert subject common names at this time, but merely to disallow wildcards
in IP addresses in cert subject common names.  

Rich, At this point, we're waiting for you to tell us when we can commit 
this fix.  We believe that will be effectively a public disclosure.

Comment 21

[Daniel Veditz [:dveditz]]

Marking "needed" for the branches rather than "blocking" since we can't really hold back a release fixing other issues waiting on coordination of disclosure for this. We'll take it when we can (probably through bug 575620, which would be updated to reference nss 3.12.8 rather than 3.12.7)

Comment 22

[Reed Loden [:reed]]

(In reply to comment #20)
> We believe that will be effectively a public disclosure.

You could use a cover bug with some generalized summary that sounds innocuous to commit it so it's less likely to be noticed...

Comment 23

[Reed Loden [:reed]]

Rich, can we get an answer to comment #20? Thanks!

Comment 24

[Kai Engert (:KaiE:)]

(In reply to comment #13)
> From Kai's patch
>   /* replace all wildcards with zero */
> 
> Are multiple wildcards allowed?


I was simply trying to write failsafe code, assuring my code will transform any ip-address-with-wildcards input into a string that represents a correct ip address, allowing me to use nspr's ip address parsing code.

the other attached patches are smarter, because they avoid the need for such a transformation.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #22)
> You could use a cover bug with some generalized summary that sounds innocuous
> to commit it so it's less likely to be noticed...

We used to do that sort of thing.  But fixes got noticed anyway.  
If you're going to do some sort of public disclosure, we'd like to know
when that will be, so we can have a release simultaneously, or just before.

Comment 26

[[e-mail]]

I've just chased microsoft again (previously chased them on Friday). They've already confirmed they can reproduce the issue, but I still have no information from them about when they'll be fixing it. I've suggested that given the issue is relatively minor that we could release a fix for other browsers without them. I'll keep you posted.

Comment 27

[[e-mail]]

I've now got a response from microsoft indicating they'll fix it in a service pack, and am just waiting on an official response to the advisory. I've also written a patch to Qt to address the issue. When is the next firefox release scheduled for since it looks like i can schedule the Qt patch and the advisory release to coordinate with firefox?

Comment 28

[Kai Engert (:KaiE:)]

Rich, what's your opinion about committing a fix to the source repositories? When is it ok to happen?

We need to commit a fix to NSS earlier than releasing an updated browser.

Comment 29

[[e-mail]]

Given that microsoft have said they're fine to release the advisory before they have a fix ready, I don't see any problem. I have a fix ready for Qt and can get that applied any time from Monday, so it's really up to you.

Comment 30

[Kai Engert (:KaiE:)]

Nelson, Wan-Teh, we have two patches with r+

Can one of them be marked as obsolete?

Comment 31

[Kai Engert (:KaiE:)]

(In reply to comment #29)
> Given that microsoft have said they're fine to release the advisory before they
> have a fix ready, I don't see any problem. I have a fix ready for Qt and can
> get that applied any time from Monday, so it's really up to you.

Thanks Rich!

Let's target NSS 3.12.8 and get this patch commited in time.


Thanks to Wan-Teh for reminding us about the agreed-on patch.

Comment 32

[[e-mail]]

Is there a planned date for this to be committed?

Comment 33

[Kai Engert (:KaiE:)]

Rich, we're ready to commit this at any time. We could do it today.

Comment 34

[[e-mail]]

Great, I guess we've both been waiting for each other! Please feel free to commit. I'll release the advisory once the fix is available (presumably that means I should wait for the next firefox release?).

Comment 35

[Kai Engert (:KaiE:)]

Rich, there are two possible courses of action. A very strict one, and a relaxed one.

Given that we seem to agree this issue is low risk, I believe we're fine to avoid the very strict path, and take a more relaxed path.

I'm not the official speaker of the NSS team, nor for Mozilla security, but in my understanding from our meetings, both Mozilla and NSS are basically waiting for you, since it's your discovery, and we want to make sure you're the first to publish the information about this issue.

In our understanding, once we commit the patch to CVS, it's no longer a secret.

I propose we commit the fix to both NSS and KDE as soon as it's convenient, without the need to coordinate the CVS commit.

Then it's basically up to you to publish the issue.

Since we're not doing a firedrill for this fix, it'll probably take anything between 2-5 weeks until this fix ends up in a published stable Firefox security update.

(We'll probably have a NSS 3.12.8 release in about 1-2 weeks, then Firefox needs to pick it up for it's next release.)

Rich, thanks again for making us aware of this issue!

We'll have the weekly NSS meeting in about 30 minutes, where I can tell people about this proposal, and get a final confirmation that this proposal is OK with everyone.

Comment 36

[Kai Engert (:KaiE:)]

Rich, I confirm that comment 35 is OK with Dan Veditz from Mozilla's security team, and the NSS team, as discussed in today's weekly conference call.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Kai and Wan-Teh,
The r+ patch above that not marked obsolete (attachment 457469 [details] [diff] [review])
will disallow wildcards in IP addresses, but will NOT disallow IP addresses 
in cert CNs.

There was another attachment, also r+, attachment 457470 [details] [diff] [review], 
now marked obsolete, that disabled IP addresses entirely in cert CNs. 

I am prepared to commit the patch above (attachment 457469 [details] [diff] [review]) 
because that is what this bug tells me to do.  But I have heard and read 
numerous comments in the last month or so saying that the NSS team now 
wishes to completely stop honoring IP addresses in cert CNs.  

So, I am not sure that attachment 457469 [details] [diff] [review] 
is really the one you want me to commit.  Please confirm it, or refute it,
one more time.  I will commit the patch you choose.

Comment 38

[Kai Engert (:KaiE:)]

Nelson, at this time, we want to keep NSS' current behaviour, which is to still accept IP addresses in CN.

This is intended for backwards compatibility reasons, so that stable product branches can pick up the newer NSS release for security fixes (like this one), but not yet get a change in other behaviour.

Regarding disallowing IP addresses in CN altogether:
The decision was to start slowly. We'll begin by disallowing that at the PSM level, so that Firefox 4 will have the new behaviour (bug 585616), but older Firefox 3.6.x security updates will still accept non-wildcard IPs in CN.

Comment 39

[Kai Engert (:KaiE:)]

(In reply to comment #37)
> The r+ patch above that not marked obsolete (attachment 457469 [details] [diff] [review])
> will disallow wildcards in IP addresses, but will NOT disallow IP addresses 
> in cert CNs.

Correct, that's the approach that has been agreed on in a NSS conference call.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Checking in certdb/certdb.c; new revision: 1.108; previous revision: 1.107

Comment 41

[Wan-Teh Chang]

Checked in on the NSS_3_12_BRANCH (NSS 3.12.8).

Checking in certdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/certdb.c,v  <--  certdb.c
new revision: 1.104.2.1; previous revision: 1.104
done

Comment 42

[Reed Loden [:reed]]

Advisory has been posted:
http://www.westpoint.ltd.uk/advisories/wp-10-0001.txt

Comment 43

[Reed Loden [:reed]]

Can we go ahead and prepare a new NSS release that includes this patch so we can go ahead and get Firefox nightlies using a fixed version?

Comment 44

[Kai Engert (:KaiE:)]

Reed, we intend to release this fix with NSS 3.12.8, which I'd expect in the next 1-2 weeks, maybe sooner.

Comment 45

[Daniel Veditz [:dveditz]]

Want this in the next releases. Presumably we'll just take NSS 3.12.8, but if we run up against code-freeze we might just apply this patch. Time frame shouldn't be a problem though.

Comment 46

[Daniel Veditz [:dveditz]]

Fixed by upgrading to NSS 3.12.8 on both branches (bug 595300)