103752 - add support for multiple hostnames in an SSL server certificate

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[bill]

Stephane - it's called SubjectAltName, and CMS does support the extension [;-)]

However, NSS doesn't support using this extension as an alternative for the
common name check, it only supports either one common name, or the magic,
proprietry extension SEC_OID_NS_CERT_EXT_SSL_SERVER_NAME (which is no longer n
common usage).

There are several models we could follow to represent the common name:
- use a single CN with regular expression (does not work in IE)
- use multiple instances of CN's (NSS will only read the first
- use a combination of 1 & 2 (I haven't tested this)
- use multiple subjectaltnames (doesn't work with NSS)


See section 4.2.1.7 in RFC 2459 for more info on RFC 2459
   http://www.ietf.org/rfc/rfc2459.txt?number=2459

My recommendation is to add code to NSS to support:
- reading multiple instances of CN
- reading dnsName from subjectaltname

The code is here:
http://lxr.mozilla.org/mozilla/source/security/nss/lib/certdb/certdb.c#1293

Comment 1

[Wan-Teh Chang]

Bob, could you take a look at this request?  Thanks.

Comment 2

[Stephane Saux]

add myself and stevep/thomask to the cc list.

Comment 3

[Wan-Teh Chang]

Target 4.0, priority P1.

Comment 4

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 5

[Terry Hayes]

RFC 2818 also discusses using SubjectAltName for DNS (host) names.

Comment 6

[Michael Bell]

RFC 2818 describes the way how to interpret a servercertificate too (RFC
describes HTTP over TLS). Mozilla is not compliant to this standard but
Microsofts IE is it but there is a solution for this problem.

1. Microsoft is standard compliant so it checks the subject alternative name
first.

2. Mozilla (and Netscape too) checks only the (first) common name.

So what to do?

Set the servernames in the common name like a regular expression. Set the
subject alternative name like described in RFC 2818 and don't forget the IPs. IE
will check the subject alternative name first and ignores the ugly common name
in the distinguished name. Mozilla will check only your common name and ignores
the subject alternative name.

This way work but please take in mind that this is a hack and Mozilla is not
standard comliant to RFC 2818.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

This is a deficiency in the NSS library function CERT_VerifyCertName().
It just hasn't kept up to date with the standards.  

If that function is brought up to date (so that it also considers 
SubjectAltName in addition to the Subject CommonName, then this problem 
will be fixed without any changes to the SSL library being needed.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 138792 has been marked as a duplicate of this bug. ***

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

I disagree with the target fix milestone of NSS 4.0.
I believe this deficiency in CERT_VerifyCertName can be fixed without
depending on any of the other "stan" changes.  
This is becoming a hotter issue daily.  
CERT_VerifyCertName needs to catch up with the times, changing from
5 year old code to only 1-2 year old code :)

Comment 10

[Wan-Teh Chang]

*** Bug 142897 has been marked as a duplicate of this bug. ***

Comment 11

[Wan-Teh Chang]

Assigned the bug to Nelson.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch implements Subject Alt name test for SSL servers

This patch should work for Subject Alternative Names that are either 
DNSNames or IPAddresses.  I've tested it with DNSNames, but haven't yet
found any server certs with IPAddresses in the Subject Alt Name extension.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

I have checked in the above patch on the trunk.  
The ability to match on IP addresses in the subjectAltName extension is
not yet tested because I have not found a cert that contains an IPAddress
in the subjectAltName extension.  

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Attachment: revised patch, works when subjectAltName extension is absent

This patch is now checked in.

Comment 15

[Michael Bell]

Attachment: cert of server with IP address in subjectAltName

We have a server which uses an IP address in the subjectAltName.
https://www.hu-berlin.de

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Thanks for the URL of the server.  I see that my code for handling 
IP addresses is wrong, so I'm reopening this bug.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 93406 [details]
cert of server with IP address in subjectAltName

Changed MIME type of the above attachment.

Comment 18

[Wan-Teh Chang]

Comment on attachment 93347 [details]
revised patch, works when subjectAltName extension is absent

These are the review comments on the second patch.

1. I didn't find any errors.

2. Nit: declare cert_TestHostName and cert_VerifySubjectAltName
as static.

3. Nit: in cert_TestHostName, the type of nameLen should be
ptrdiff_t.

>+	if ((cndomain = PORT_Strchr(cn, '.')) != NULL &&
>+	    (nameLen = cndomain - cn) < sizeof cnbuf  &&
>+	    nameLen > 0) {

The comparison nameLen > 0 must be true (unless PORT_Strchr
is broken) and can be deleted.

>+	    cn = (const char *)cnbuf;

The (const char *) cast in front of cnbuf is not necessary.

4. In cert_TestHostName, you can declare the cn argument
as char *, without the const.  Then, you can chop off the
domain in the cn string by
	*cndomain = '\0';
like the original code does.  This avoids the copy.

5. In cert_VerifySubjectAltName, the comment "so much copy
it" should be "so must copy it".  Delete the comment
"Then downshift it."

6. If we are more likely to have a match with the common
name, we should compare the hn string with it first.  If
they compare equal, we skip the cert_VerifySujectAltName
call altogether.

7. It's kind of strange to see certdb set an SSL error
code (SSL_ERROR_BAD_CERT_DOMAIN).

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

reponses to above review comments, by number:

2. The lower case cert_ prefix is intended to signify that the function
is only for NSS internal use.  I didn't make these functions static because
I thought some other part of NSS might want to use them sometime.

3. if the first character of cn is '.', then namelen == 0, right?

4-5. Good suggestions.  I did them.

6. RFC 2818 says that if a subjectAltName with a DNSNmae is present, it must be 
used as the cert's identity, otherwise the subject common name may be used.  
So, I have changed this test so that it only examines the Common Name if the 
subjectAltName call returns SEC_ERROR_EXTENSION_NOT_FOUND.  

7. True.  Perhaps these functions actually belong in libSSL.  But changing that 
would be an ABI change, not backwards binary compatible.

Comment 20

[Wan-Teh Chang]

Nelson wrote:
> 3. if the first character of cn is '.', then namelen == 0,
> right?

Ah, you are right.  I thought the intent was to detect a
negative nameLen, which is impossible.  I didn't realize
the intent was to detect a zero nameLen.

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incremental patch, addresses issues listed above

Review invited.

Comment 22

[Wan-Teh Chang]

Comment on attachment 93497 [details] [diff] [review]
incremental patch, addresses issues listed above

Comments on the incremental patch.

1. When verifying an IP address, you handle three cases.
Seems like for completeness, the fourth case (
current->name.other.len == 4 &&
netAddr.ipv6.family == PR_AF_INET6 ) should also be
handled.

2. I suggest using replacing
    PRStatus prs;
    ...
    prs = PR_StringToNetAddr(hn, &netAddr);

by something like
    PRBool hnIsIPAddr;
    ...
    hnIsIPAddr = (PR_StringToNetAddr(hn, &netAddr) == PR_SUCCESS);

It'll be easier to read and make the comments unnecessary.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Attachment: incremental patch, addresses review comments above

Review invited.

Comment 24

[Wan-Teh Chang]

Comment on attachment 93617 [details] [diff] [review]
incremental patch, addresses review comments above

>+		    PRUint32 ipv4 = (current->name.other.data[0] << 24) |
>+		                    (current->name.other.data[1] << 16) |
>+				    (current->name.other.data[2] <<  8) |
>+				     current->name.other.data[3];
>+		    /* ipv4 must be in Network Byte Order on input. */
>+		    PR_ConvertIPv4AddrToIPv6(PR_htonl(ipv4), &v6Addr);
>+		    match = !memcmp(&netAddr.ipv6.ip, &v6Addr, 16);

You can also just say:

    PRUint32 ipv4;
    memcpy(&ipv4, current->name.other.data, 4);
    PR_ConvertIPv4AddrToIPv6(ipv4, &v6addr);
    match = !memcmp(&netAddr.ipv6.ip, &v6Addr, 16);

or alternatively:

    if (PR_IsNetAddrType(&netAddr, PR_IpAddrV4Mapped) {
	match = !memcmp(&netAddr.ipv6.ip.pr_s6_addr[12],
			current->name.other.data, 4);
    } 

>-    PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
>+    if ((!isIPaddr && !DNSextCount) || (isIPaddr && !IPextCount)) {
>+	/* no relevant value in the extension was found. */
>+	PORT_SetError(SEC_ERROR_EXTENSION_NOT_FOUND);
>+    } else {
>+	PORT_SetError(SSL_ERROR_BAD_CERT_DOMAIN);
>+    }

The (isIPaddr && !IPextCount) test makes your name verification
more lenient than what RFC 2818 reads to me.  That is, if the
server cert has the subject alt name extension but the extension
contains no IP addresses (IPextCount is 0), RFC 2818 suggests that
the name verification should fail, but your patch will go on to
test the common name.  The relevant paragraph in RFC 2818 is
quoted here:

   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.

It is fine by me to be more lenient.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Elsewhere in RFC 2818, it says 

   If a subjectAltName extension of type dNSName is present, that MUST
   be used as the identity. Otherwise, the (most specific) Common Name
   field in the Subject field of the certificate MUST be used. 

Notice that this section is not conditioned upon the URI being a hostname
and not an IP address.  So this statement conflicts with the one cited 
above.  

I implemented this policy:

    if the URI contains a host name
        if the subject alt name is present and has one or more DNS names
            use the DNS names in that extension as the server identity
        else
            use the subject common name as the server identity
    else if the URI contains an IP address
        if the subject alt name is present and has one or more IP addresses
            use the IP addresses in that extension as the server identity
        else
            compare the URI IP address string with the subject common name.

Perhaps there is another issue here.  NSS always gets and examines the
first Common Name attribute.  It is apparently possible for the subject 
name to cotnain more than one Common Name attribute.  We make no attempt
to find the "most specific" one.  I don't know how that's defined.

The above patch is checked in.  I'm marking this resolved.

Comment 26

[Terry Hayes]

My understanding of the phrase "most specific" is that the last attribute in 
the RDN sequence that matches the tag (CN) should be used. As you point out, 
NSS does not do this.  However, since the use of CN for host name is supported 
only for compatibility, and we haven't had any problems to this point, there is 
no need to fix this.

Comment 27

[Wan-Teh Chang]

Comment on attachment 93617 [details] [diff] [review]
incremental patch, addresses review comments above

r=wtc.

Comment 28

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 1.1

Comment 29

[Wan-Teh Chang]

I checked in Nelson's patches into the NSS_3_5_BRANCH and
NSS_CLIENT_TAG.  This fix will be in Mozilla 1.1 and NSS
3.5.1.

Comment 30

[Wan-Teh Chang]

This broke the Mac build:

Error   : illegal implicit conversion from 'const char *' to
'char *'
certdb.c line 1266   int match = PORT_RegExpCaseSearch(hn, cn);

We may need to do what the original code did: duplicate hn

    hostname = PORT_Strdup(hn);

and pass the copy (hostname) to PORT_RegExpCaseSearch.

I backed out the patch from NSS_CLIENT_TAG.

Comment 31

[Wan-Teh Chang]

I found that Nelson already fixed this problem the
right way on the tip.  PORT_RegExpCaseSearch and the
other functions declared in portreg.h should take
const char * arguments.  I merged Nelson's changes to
portreg.{h,c} into the NSS_3_5_BRANCH and NSS_CLIENT_TAG
and put the fix for this bug back in NSS_CLIENT_TAG.

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

*** Bug 183732 has been marked as a duplicate of this bug. ***

Comment 33

[Ivan Dolezal]

If you look at https://uzivatel.vsb.cz/ , the browser will not display a
spoofing warning, but on the other hand it will display "Bad Padlock". The
certificate has 4 different SubjectAltName DNS records. Should Mozilla either
display warning or don't display the "Bad Padlock" at all... it looks
inconsistent. Not really sure if this is a bug or a feature, so I am not
reopening this bug. Personally I am not really sure if the presented kind of
certificate complies RFC 2818 and I will be grateful for your opinion.

I am using Mozilla 1.2.1 release on Win98 with its version of NSS. The CA
Certificate can be downloaded at
http://www.vsb.cz/cgi-bin/CA/CAload.pl/001/cacert.crt .

Comment 34

[Wan-Teh Chang]

I think the bad padlock means the secure page contains.
references to non-secure (http, not https) URLs.

If you replace all the href="http://..." by href="https://..."
in that page, the bad padlock should be gone.

Comment 35

[Michael Bell]

The four different subjectAltNames are correct. This is not spoofing. The
different alternative names are for virtual hosts like ISPs. We have servers
with up to six or seven different names and they have only one certificate. I
checked the page and only some http://... avoid Mozilla from running without
warnings.

Comment 36

[Ivan Dolezal]

Thank you guys for your effort and the lesson. I didn't make the webpage itself
and somehow did a silent implication that the experienced author knew what he
was doing. Sorry for your time and thank you once again.