484111 - Must escape DER DNS names when converting to zStrings

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

While working on bug 483437, and reviewing the patch for bug 480509, 
Honza Bombas realized that the problem with unescaped DNS names is 
broader than reported in bug 480509.  It affects DNS names in Subject
Alt Name extensions, as well as in Certificate Subject Common Names. 

Thinking about this, I realized that any time we take any string value
out of a cert and convert it to a NUL-terminate ASCII/UTF8 string 
(a so-called zString), we must escape it.  I think the right way to 
approach this is to start by attempting to find all places where we do
that with dNSNames (part of ASN.1 GeneralName type), and make sure those
are done correctly.  Then when that's done, we can also look into other
types of strings besides dNSNames.  (Yes, that strange capitalization is
the one defined in the standard.)

So, one place to start is with the code in 
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.c#1551

But I'm sure that is only one of many such places. :(

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: test (+ preliminary fix for bug 483437)

You can use test for this bug contained in this patch to reproduce. Certs generated with:
$ certutil -S -n "escapeattack1" -s "CN=www.bank1.com\00www.bad-guy.com" -c "pgo temporary ca" -t "P,," -k rsa -g 1024 -m 545786714 -v 120 -d . 
$ certutil -S -n "escapeattack2" -s "CN=www.bad-guy.com" -8 "www.bank2.com#www.bad-guy.com" -c "pgo temporary ca" -t "P,," -k rsa -g 1024 -m 286817869 -v 120 -d . 

and # replaced with \0 in a debugger (certext.c:731).

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment 1 points out that we also should unescape them on input (from a
zString), as well as escape them on output (to a zString).

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Honza, 
1) what is this build/pgo file?  
2) You asked me in another bug (can't find it now) why some function that is
in an public NSS header file is not exported from NSS.  It looks to me like
you may have duplicated that function in manager/ssl/src/nsNSSCertHelper.cpp .
Is that what you did?  If so, it's trivial for us to export that function
from NSS, and I'd much rather do that than duplicate all that code.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Honza, please attach to this bug a file containing a cert produced by
the steps you gave in comment 1.  Thanks.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: short term fix? v1 (checked in)

This patch adds the missing declaration for CERT_RFC1485_EscapeAndQuote
to cert.h, and uses it in certdb.c to escape DNS names in SANs for host
name comparison purposes.   It's not the ideal solution for several reasons, 
but its quick, and may suffice to mitigate this vulnerability enough for the 
next NSS release.

I want to test this before requesting review, but if you (Honza) want to 
review it and/or play with it, I'd appreciate that.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> Honza, 
> 1) what is this build/pgo file?

I attached a mochitest for these issues. You have to apply it to mozilla-central tree if you want to use it, probably not very suitable for you.

> 2) You asked me in another bug (can't find it now) why some function that is
> in an public NSS header file is not exported from NSS.  It looks to me like
> you may have duplicated that function in manager/ssl/src/nsNSSCertHelper.cpp .
> Is that what you did?  

I did, exactly.

> If so, it's trivial for us to export that function
> from NSS, and I'd much rather do that than duplicate all that code.

Me too, but that function has one (major) disadvantage - it forces the caller to precount length and allocate the dest buffer. It is quit complicated and may lead to disaster when used mistakenly. Perfect would be to create a new function that counts and allocates the buffer, but that's not up to me.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: cert with \0 in subject alt name

+ private key, blank password

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #5)
> Created an attachment (id=368254) [details]
> short term fix? v1 (untested)
> 
> I want to test this before requesting review, but if you (Honza) want to 
> review it and/or play with it, I'd appreciate that.

My test passes with this patch. Problem fixed.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368254 [details] [diff] [review]
short term fix? v1 (checked in)

Please review.

Regarding the need to determine the size of the output buffer before calling CERT_RFC1485_EscapeAndQuote, Given that you know the size of the input, (let's call that size N bytes) you can be sure that the output (including the trailing NUL character) will always fits in a buffer of size 3*(N+1) or equivalently 3*N + 3. This patch uses that formula to allocate storage before calling that function.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 368254 [details] [diff] [review]
short term fix? v1 (checked in)

I can build a final nice patch for bug 483437 upon this change, using the 3*(N+1) math.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368254 [details] [diff] [review]
short term fix? v1 (checked in)

Thanks for the review, Honza.

Committed on trunk.
Checking in cert.h;   new revision: 1.76; previous revision: 1.75
Checking in certdb.c; new revision: 1.99; previous revision: 1.98

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Attachment: fix egregious bug (checked in)

That last patch contained an egregious bug.  I wish C compilers were 
more strict about comparisons of pointers and ints. :(

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 368822 [details] [diff] [review]
fix egregious bug (checked in)

Thanks for the quick review.  Checked in.

lib/certdb/certdb.c new revision: 1.100; previous revision: 1.99

Comment 15

[Mark Cox]

Assigning CVE-2009-2408 for tracking.  (We only need one CVE name for all the patches that correct unescaped names issues).

Comment 16

[Kai Engert (:KaiE:)]

Adding Elio to cc list, he is going to work on the fixed NSS package for RHEL.