Last Comment Bug 480509 - Improper character escaping and unescaping in alg1485.c & secname.c
: Improper character escaping and unescaping in alg1485.c & secname.c
Status: RESOLVED FIXED
[sg:high]
: fixed1.9.0.13, fixed1.9.0.14, fixed1.9.1
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.0
: x86 All
: P1 critical (vote)
: 3.12.3
Assigned To: Nelson Bolyard (seldom reads bugmail)
:
Mentors:
: 504449 (view as bug list)
Depends on: 500495
Blocks: 483437 483440
  Show dependency treegraph
 
Reported: 2009-02-27 00:49 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2009-08-07 01:27 PDT (History)
20 users (show)
mbeltzner: blocking1.9.1+
dveditz: wanted1.9.1.x+
dveditz: blocking1.9.0.14+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Work in Progress (19.09 KB, patch)
2009-02-27 15:37 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
Work in Progress - v2 (21.27 KB, patch)
2009-02-27 16:43 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
Work in Progress - v3 - handle OID components up to 64 bits (22.14 KB, patch)
2009-02-28 23:11 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
patch v4 - for review (22.20 KB, patch)
2009-03-02 19:46 PST, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
Certificate with UTF-8 CN (749 bytes, text/plain)
2009-03-03 09:13 PST, Honza Bambas (:mayhemer)
no flags Details
Patch v5 - addressed WTC's comments (25.60 KB, patch)
2009-03-09 17:41 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review-
Details | Diff | Splinter Review
Patch v5 - again (maybe bugzilla will like this one) (25.65 KB, patch)
2009-03-09 17:48 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
interdiff of v4 and v5 (16.41 KB, patch)
2009-03-09 18:39 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
sample PEM encoded PKCS10 certificate request attack (734 bytes, text/plain)
2009-03-09 19:54 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details
patch v6 - attempt to address Bob's comments (26.27 KB, patch)
2009-03-12 22:50 PDT, Nelson Bolyard (seldom reads bugmail)
no flags Details | Diff | Splinter Review
patch v7 - aargh indeed! (checked in) (26.78 KB, patch)
2009-03-13 21:49 PDT, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
Details | Diff | Splinter Review
patch - remove duplicate line (checked in) (550 bytes, patch)
2009-03-18 13:56 PDT, Nelson Bolyard (seldom reads bugmail)
honzab.moz: review+
Details | Diff | Splinter Review
patch, part 3, v1 - don't assume last byte has a zero continuation bit (checked in) (1.28 KB, patch)
2009-04-11 13:09 PDT, Nelson Bolyard (seldom reads bugmail)
honzab.moz: review+
Details | Diff | Splinter Review
patch part 4, v1 - fix bug in previous patch (525 bytes, patch)
2009-04-11 22:41 PDT, Nelson Bolyard (seldom reads bugmail)
alvolkov.bgs: review+
Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2009-02-27 00:49:42 PST
NSS's numerous functions for converting strings from ASCII/UTF8 to DER,
and from DER to ASCII/UTF8, including functions that return string values
from cert name attributes, which live in files alg1485.c and secname.c,
do not escape all characters that they should, and do not unescape 
hex character pairs at all.   This has grave security implications.

I'm afraid this is going to require a pretty big overhaul to some of that
code.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2009-02-27 15:37:08 PST
Created attachment 364604 [details] [diff] [review]
Work in Progress

Not for review.  This is almost right I think.  However,
it may escape the last byte of a multi-byte UTF8 character,
it may put quotation marks around a returned element.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2009-02-27 16:43:56 PST
Created attachment 364614 [details] [diff] [review]
Work in Progress - v2

This seems to solve the unwanted quoting problem.  
Still must tackle quoting of UTF8 char fragments.
Need more test certs.
Comment 3 Nelson Bolyard (seldom reads bugmail) 2009-02-28 23:11:30 PST
Created attachment 364741 [details] [diff] [review]
Work in Progress - v3 - handle OID components up to 64 bits

Getting closer.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2009-03-02 19:41:35 PST
Adding reviewers and testers to CC list.  
If you're CC'ed but you've not seen this bug before, please read it.
Contact me privately (not in bugzilla) for details.
New patch forthcoming momentarily.
Comment 5 Nelson Bolyard (seldom reads bugmail) 2009-03-02 19:46:48 PST
Created attachment 365118 [details] [diff] [review]
patch v4 - for review

This patch causes the code to display "UNSUPPORTED" rather than "INVALID"
for OID components that exceed 64-bits or are not correctly BER/DER encoded.

I was mistaken in thinking that the escaping code would present a problem
with UTF8 characters.  I now believe it will not, because it does not 
escape bytes with the high order bit set.  In UTF8, ALL bytes in a multi-byte
character have the high bit set.  I was mistakenly thinking that all but the
last have the high bit set.
Comment 6 Nelson Bolyard (seldom reads bugmail) 2009-03-02 19:50:57 PST
I'd appreciate it if one or more PSM folks would test with this patch.
I'm looking for things that no longer work right, such as improper cert 
name mismatches or incorrectly displayed cert name attributes.  

I'm running this code in my browser, and it seems OK to me so far, but 
I rarely encounter certs with non-ASCII characters in the names.  
In fact, a recent attempt to find servers with such certs came up empty. :(
So, I'm hoping others will encounter them more frequently than I have.
Comment 7 Honza Bambas (:mayhemer) 2009-03-03 08:09:56 PST
I generated a cert with openssl like this:

openssl.cnf:
[req]
string_mask = utf8only
distinguished_name = MY1
[MY1]
commonName = ñeco ñýáěšžěšžřěčíě+ščýáíéřáíÚ

openssl genrsa -out user.key 1024
openssl req -key user.key -new -x509 -out user.crt -config openssl.cnf -utf8

and imported to softoken as a user trusted CA. Name is correctly displayed everywhere in the cert manager. Exported certificate displayed with windows XP cert manager also contains correct name.

Certificate imported while patch not applied is correctly displayed with the patch. Import w/ the patch and display w/o the patch also correct.
Comment 8 Nelson Bolyard (seldom reads bugmail) 2009-03-03 09:11:30 PST
Honza, Thanks.  Please attach that cert to this bug.  It seems useful as a 
test case.  Thanks.
Comment 9 Honza Bambas (:mayhemer) 2009-03-03 09:13:44 PST
Created attachment 365221 [details]
Certificate with UTF-8 CN
Comment 10 Wan-Teh Chang 2009-03-09 13:10:56 PDT
Comment on attachment 365118 [details] [diff] [review]
patch v4 - for review

I did my first reading of the patch last weekend.  I only
read the patch, not the original files, so I didn't fully
understand the code.  The comments below are therefore mostly
coding style nits.

1. nss/cmd/lib/secutil.c

Why we don't need to print those two quotation marks in
secu_PrintRawString?  Is it because the input string will
contain the quotation marks with your other changes?

2. nss/lib/certdb/alg1485.c

Since you moved x2b to the beginning of the file, far
from where x2b is used, it would be nice to add a comment
to document what x2b is.  For example,
    Maps a hex digit '0'-'9', 'a'-'f', 'A'-'F' to its value
    0-15.  Maps other characters to -1.
Alternatively, if you can move the definition of the IS_HEX
macro up, right below the x2b array, then the code will be
somewhat self-documenting.

>@@ -259,7 +292,12 @@ scanVal(char **pbp, char *endptr, char *
> 	    if (bp >= endptr) {
> 		/* escape charater must appear with paired char */
> 		*pbp = bp;
>-		return SECFailure;
>+		return 0;
>+	    }
>+	    c = *bp;
>+	    if (IS_HEX(c) && (endptr - bp) > 0 && IS_HEX(bp[1])) {
>+		bp++;
>+		c = (char)((x2b[(PRUint8)c] << 4) | x2b[(PRUint8)*bp]); 
> 	    }

The (endptr - bp) > 0 test in the second if statement should
be removed.  That test is guaranteed to be true by the first
if statement.

>@@ -274,27 +312,28 @@ scanVal(char **pbp, char *endptr, char *
[...]    
>-    /* stip trailing spaces from unquoted values */
>+    /* strip trailing spaces from unquoted values */
>     if (!isQuoted) {
>-	if (valBufp > valBuf) {
>-	    valBufp--;
>-	    while ((valBufp > valBuf) && OPTIONAL_SPACE(*valBufp)) {
>-		valBufp--;
>-	    }
>-	    valBufp++;
>+	while (valBufp > valBuf) {
>+	    char c = valBufp[-1];
>+	    if (! OPTIONAL_SPACE(c))
>+	        break;
>+	    --valBufp;
> 	}
>+	vallen = valBufp - valBuf;
>     }

Is the new code simply a rewrite of the original code, or does
it fix a bug in the original code?  There is a subtle difference
between the original code and new code.  I'm not sure if that's
intentional.

Original code: will NOT call OPTIONAL_SPACE(*valBufp) for
valBufp == valBuf because of the (valBufp > valBuf) test before
OPTIONAL_SPACE(*valBufp).

New code: will call OPTIONAL_SPACE(*valBufp) for valBufp == valBuf.
 
>@@ -591,19 +605,23 @@ cert_RFC1485_GetRequiredLen(const char *
[...]
>+	    if (pNeedsQuoting)          /* quoting is allowed */
>+		needsQuoting = PR_TRUE; /* entirety will need quoting */
>+	    else
>+	    	reqLen++;               /* will escape this character */

In ert_RFC1485_GetRequiredLen, you added three instances of the
if (pNeedsQuoting) test like the above.  Why are you using
pNeedsQuoting as a boolean ("quoting is allowed")?  Isn't
pNeedsQuoting an output parameter?

In CERT_RFC1485_EscapeAndQuote, you added the following block of code
inside the for loop:

>     for (i = 0; i < srclen; i++) {
> 	char c = src[i];
>-	if (c == C_DOUBLE_QUOTE || c == C_BACKSLASH) {
>-	    /* escape it */
>+	if (NEEDS_HEX_ESCAPE(c)) {
> 	    *d++ = C_BACKSLASH;
>+	    *d++ = hexChars[ (c >> 4) & 0x0f ];
>+	    *d++ = hexChars[  c       & 0x0f ];
>+	} else {
>+	    if (NEEDS_ESCAPE(c)) { /* escape it */
>+		*d++ = C_BACKSLASH;
>+	    }
>+	    *d++ = c;
>+	}
>+    }

The resulting for loop is the same as the for loop in the new
cert_RFC1485_HexEscape function you added:

>+    for (i = 0; i < srclen; i++) {
>+	char c = src[i];
>+	if (NEEDS_HEX_ESCAPE(c)) {
>+	    *d++ = C_BACKSLASH;
>+	    *d++ = hexChars[ (c >> 4) & 0x0f ];
>+	    *d++ = hexChars[  c       & 0x0f ];
>+	} else {
>+	    if (NEEDS_ESCAPE(c)) { /* escape it */
>+		*d++ = C_BACKSLASH;
>+	    }
>+	    *d++ = c;
> 	}
>-	*d++ = c;
>     }

(Actually the code following the for loop is also the same.)
Perhaps you can create a function for the common code.

>+	/* Here, d points at first byte, e points at last */
>+	len = (unsigned int)(e - d);
>+	if (len <= 3U) {        /* 0-28 bit number */

Just curious: how does len=3 become 28 bits?  28 = 7 * (3 + 1)?

>+#define CASE(i, m) \
>+	    case i:                  \
>+		c  = e[-i] & m;      \
>+		n |= c << (7 * i);   \
>+		if (!n) goto unsupported \
>+		/* fall-through */
>+
>+	    switch (len) {
>+	    CASE(3, 0x7f);
>+	    CASE(2, 0x7f);
>+	    CASE(1, 0x7f);
>+	    case 0: n |= e[0]; /* most significant bit is known to be zero. */
> 	    }

Nit: since the CASE(i, m) macro is a code snippet generator,
we don't need to make it look like a function macro.  So I
suggest that we move the semicolon into the macro definition,
after the "goto unsupported" statement.

Nit: put "case 0:" and "n |= e[0];" on separate lines.

>+	} else if (len <= 9U) { /* 29-64 bit number */
>+	    PRUint64 n = 0;
>+	    PRUint64 c;
>+
>+	    switch (len) {
>+	    CASE(9, 0x01);
>+	    CASE(8, 0x7f);
>+	    CASE(7, 0x7f);
>+	    CASE(6, 0x7f);
>+	    CASE(5, 0x7f);
>+	    CASE(4, 0x7f);
>+	    CASE(3, 0x7f);
>+	    CASE(2, 0x7f);
>+	    CASE(1, 0x7f);
>+	    case 0: n |= e[0]; /* most significant bit is known to be zero. */
>+	    }

Since len is > 3U, we should not need the cases for 3, 2, 1, 0, right?

>+	} else {
>+	    /* More than a 64-bit number, or not minimal encoding. */
>+unsupported:
>+	    if (!a)
>+		a = PR_smprintf("OID.UNSUPPORTED");
>+	    else {
>+		b = a;
>+		a = PR_smprintf("%s.UNSUPPORTED", b);
>+	    }
>+	}

Nit: I never liked goto statements whose targets are in a different
block.  I know you used them a lot in lib/ssl.

In AppendAVA, you have:

>+    PRBool       needsQuoting;
...
>+		cert_RFC1485_GetRequiredLen(avaValue->data, avaValue->len, 
>+					    &needsQuoting));

You never use the value returned by cert_RFC1485_GetRequiredLen in
needsQuoting.  This is related to my question above that you're using
pNeedsQuoting as a boolean in cert_RFC1485_GetRequiredLen.

In CERT_GetNameElement, you have:

>+    char *    buf = 0;

Since you changed 0 to NULL elsewhere, you may want to change 0 to
NULL here, too.

At the end of CERT_GetNameElement, you have:

>+found:
>+    avaValue = CERT_DecodeAVAValue(&ava->value);
>+    if(!avaValue) {
>+	return buf;
>+    }
>+    valueLen = cert_RFC1485_GetRequiredLen(avaValue->data, avaValue->len, 
>+					   NULL) + 1;
>+    if (arena) {
>+	buf = (char *)PORT_ArenaZAlloc(arena, valueLen);
>+    } else {
>+	buf = (char *)PORT_ZAlloc(valueLen);
>+    }
>+    if (buf) {
>+	SECStatus rv;
>+	rv = cert_RFC1485_HexEscape(buf, valueLen, 
>+				    (char *)avaValue->data, avaValue->len);
>+	if (rv != SECSuccess) {
>+	    PORT_Free(buf);
>+	    buf = NULL;
>+	}
>+    }
>+    SECITEM_FreeItem(avaValue, PR_TRUE);
>     return buf;
> }

The PORT_Free(buf) call should only be made if arena is NULL.

This block of code is very similar to the code in CERT_GetLastNameElement.
You may want to create a function for the common code.

In CERT_GetLastNameElement, 

>+	valueLen = cert_RFC1485_GetRequiredLen(avaValue->data, avaValue->len, 
>+	                                       NULL) + 1;
> 	if (arena) {
>-	    buf = (char *)PORT_ArenaZAlloc(arena,decodeItem->len + 1);
>+	    buf = (char *)PORT_ArenaZAlloc(arena, valueLen);
> 	} else {
>-	    buf = (char *)PORT_ZAlloc(decodeItem->len + 1);
>+	    buf = (char *)PORT_ZAlloc(valueLen);
> 	}
>-	if ( buf ) {
>-	    PORT_Memcpy(buf, decodeItem->data, decodeItem->len);
>-	    buf[decodeItem->len] = 0;
>+	if (buf) {
>+	    SECStatus rv;
>+	    rv = cert_RFC1485_HexEscape(buf, valueLen, 
>+		    		        (char *)avaValue->data, avaValue->len);
>+	    if (rv != SECSuccess) {
>+	    	PORT_Free(buf);
>+		buf = NULL;
>+	    }
> 	}
>-	SECITEM_FreeItem(decodeItem, PR_TRUE);
>+	SECITEM_FreeItem(avaValue, PR_TRUE);

This is the block of code that's similar.  Therefore, its PORT_Free(buf)
call needs the same fix.

3. nss/lib/certdb/secname.c
 
> static SECStatus
>-SetupAVAValue(PRArenaPool *arena, int valueType, char *value, SECItem *it,
>+SetupAVAValue(PRArenaPool *arena, int valueType, SECItem *item, SECItem *it,
> 	      unsigned maxLen)

There are two SECItem* parameters named 'item' and 'it', which is confusing.
I would keep the 'value' parameter and change its type fo unsigned char*,
and make "unsigned valueLen" an input parameter (rather than a local variable).

You may want to make the same change to CreateAVAFromSECItem (and rename
the function).
Comment 11 Nelson Bolyard (seldom reads bugmail) 2009-03-09 17:41:54 PDT
Created attachment 366443 [details] [diff] [review]
Patch v5 - addressed WTC's comments

(In reply to comment #10)

Wan-Teh, thanks for your valuable comments.  I have addressed all of them
(but one) in the new patch.  Here are answers to your questions.

> 1. nss/cmd/lib/secutil.c
> 
> Why we don't need to print those two quotation marks in
> secu_PrintRawString?  

Those quote marks were always confusing and unnecessary.  
Given tha the input will often be a string such as 
    C="A.B.C. company"
the extra quotation marks created confusing nonsense like:
    "C="A.B.C. company""
So, I eliminated them.

> 2. nss/lib/certdb/alg1485.c
> 
> Since you moved x2b to the beginning of the file, [...] if you can move 
> the definition of the IS_HEX macro up, right below the x2b array, 

Done.

> The (endptr - bp) > 0 test in the second if statement should be removed.  

Actualy, it was wrong.  It should have been > 1  or  >= 2.  I fixed it.

> In scanVal
> Is the new code simply a rewrite of the original code, or does
> it fix a bug in the original code?  There is a subtle difference
> between the original code and new code.  

Yes, that subtle difference is the reason  for the change.  Plus, I think
the new code is clearer than the old.  I think it's easier to convince 
one's self that the new code is correct.

> In cert_RFC1485_GetRequiredLen, you added three instances of the
> if (pNeedsQuoting) test like the above.  Why are you using
> pNeedsQuoting as a boolean ("quoting is allowed")?  Isn't
> pNeedsQuoting an output parameter?

I have added the following block comment to that function to explain:

+/* Some characters must be escaped as a hex string, e.g. c -> \nn .
+ * Others must be escaped by preceding with a '\',  e.g. c -> \c , but
+ * there are certain "special characters" that may be handled by either
+ * escaping them, or by enclosing the entire attribute value in quotes.
+ * Some callers will do quoting when needed, others will not.
+ * Argument pNeedsQuoting is set to NULL by the caller if the caller
+ * will not do quoting (and therefore spacing for the escaping must be
+ * allocated).  A caller that will do quoting when necessary points
+ * pNeedsQuoting to a variable where this function will tell the caller
+ * whether or not it should do quoting.
+ */

I eliminated a lot of duplicated code by refactoring some functions.

> >+	/* Here, d points at first byte, e points at last */
> >+	len = (unsigned int)(e - d);
> >+	if (len <= 3U) {        /* 0-28 bit number */
> 
> Just curious: how does len=3 become 28 bits?  28 = 7 * (3 + 1)?

I replaced all those horrid single-character variable names with longer
names and comments.  It should be self documenting now.  Yes, the explanation is  28 = 7 * (3 + 1)

> Nit: since the CASE(i, m) macro is a code snippet generator,
> we don't need to make it look like a function macro.  

Yes, but some tools get confused if it doesn't, so I prefer that it does.

> Since len is > 3U, we should not need the cases for 3, 2, 1, 0, right?

We don't need the case labels, but we do need the code that goes with them.
All these cases fall through.  I refactored the macro to clarify.

> In CERT_GetNameElement, you have:
> 
> >+    char *    buf = 0;

fixed.

> The PORT_Free(buf) call should only be made if arena is NULL.

Thanks for spotting that.  I fixed it.  The code is now in a common function so it's not duplicated any more.


> 3. nss/lib/certdb/secname.c

> There are two SECItem* parameters named 'item' and 'it', which is confusing.

I renamed those variables to "in" and "out".  I think it helps.
I also made "in" a pointer to a const SECItem.
Comment 12 Nelson Bolyard (seldom reads bugmail) 2009-03-09 17:48:58 PDT
Created attachment 366447 [details] [diff] [review]
Patch v5 - again (maybe bugzilla will like this one)

Bugzilla fails to diff v5 (above) with v4 of this patch. 
Maybe it will be able to properly diff this v5 against v4.
Comment 13 Nelson Bolyard (seldom reads bugmail) 2009-03-09 17:55:49 PDT
Comment on attachment 366447 [details] [diff] [review]
Patch v5 - again (maybe bugzilla will like this one)

No.  Bugzilla (actually, Bonsai) just cannot correctly diff v4 and v5.
It leaves out large parts of the changes in v5, so it will be necessary to review v5 "from scratch".  
@#$%^&*()!   Sorry.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2009-03-09 18:39:53 PDT
Created attachment 366468 [details] [diff] [review]
interdiff of v4 and v5

If this works, I think Wan-Teh will find this will be easier to review 
than a complete from-scratch review of v5.
Comment 15 Nelson Bolyard (seldom reads bugmail) 2009-03-09 19:54:44 PDT
Created attachment 366477 [details]
sample PEM encoded PKCS10 certificate request attack

If you want to see some of the real bugs this patch fixes, download this 
PEM file (P10certreq.pem) and then view it with the NSS utility command: 
  pp -t certificate-request -a -i P10certreq.pem

Do that pp command at least twice: 
- once with pp and shared libraries from any recent release of NSS 
  (without this patch)
- once with pp and shared libs from a build with this patch applied.

I wonder how other tools display it.
Comment 16 Nelson Bolyard (seldom reads bugmail) 2009-03-09 20:39:02 PDT
PP from NSS 3.12.2 displays the Subject name in the above CSR as

CN=*,OID.2.5.4.3=www.paypal.com,CN=www.mydomain.org,C=US

With patch v5, it is displayed as:

CN=*\00.www.mydomain.org,OID.2.5.4.UNSUPPORTED=www.paypal.com,\
CN=www.mydomain.org,C=US
Comment 17 Nelson Bolyard (seldom reads bugmail) 2009-03-10 14:24:38 PDT
Seeking patch review from any NSS peer
Comment 18 Robert Relyea 2009-03-11 12:13:02 PDT
Comment on attachment 366443 [details] [diff] [review]
Patch v5 - addressed WTC's comments

r- rrelyea

The patch in general looks good. I only found two very obscure bug, but it needs to be fixed.

The first bug is SPECIAL_CHAR(c) is a super set of NEEDS_ESCAPE(c).

This means the reqLen++ at line 630 is never tripped. If pNeedsQuoting is not set to NULL, then our reqLen will be short by the number of imbedded quotes and backslashes.

The second bug is in escapeAndQuote. It occurs if pNeedsQuoting is set to NULL and we encounter SPECIAL_CHARS that are not NEEDS_ESCAPE, those characters will not be escaped.

The solution to problem 1 may be as simple as removing quote and backslash from SPECIAL_CHAR (assuming you aren't required to quote if you have \" or \\).

The solution to problem 2 is probably add || (pNeedsQuoting == NULL && SPECIAL_CHAR(c)) to your if at 673.

bob
Comment 19 Nelson Bolyard (seldom reads bugmail) 2009-03-12 00:59:59 PDT
(In reply to comment #18)

> The first bug is SPECIAL_CHAR(c) is a super set of NEEDS_ESCAPE(c).

Good catch.  Definitely a bug.  Order of tests is wrong. Will fix.

> The second bug is in escapeAndQuote. It occurs if pNeedsQuoting is set to 
> NULL and we encounter SPECIAL_CHARS that are not NEEDS_ESCAPE, those 
> characters will not be escaped.

That's true.  It's the desired behavior though, not a bug.  I guess I
need to add more comments to explain that.  Here's some explanation.

The cases were pNeedsQuoting is NULL are cases where the NSS API returns
the value of a single attribute, not the cases where it returns a full
DN as a set of RDNs, or even a complete AVA.  That is, the functions that
use that feature are not functions that return
   NAME=VALUE,NAME=VALUE,NAME=VALUE
or even a simple
   NAME=VALUE
Instead, they're functions that only return 
   VALUE
They're primarily (perhaps exclusively) used for two purposes:
- To extract the CN value for host name matching
- To get single attribute values to display in a GUI, one attribute per line,
as PSM does.  

In both of those cases, we don't want the quotes, and we only want escapes
for the characters that might intefere with the display or name comparison.
There are some characters that must always be escaped, whether in a quoted
string or not, and they are the ones that we want to escape when we're not
quoting.  We never actually escape those characters that are "required" to 
be escaped when not in quotes, because that is only relevant when producing
a full RDN string, and we always use quoting for those.  

Perhaps I could be convinced that there are 3 cases here:
- use quoting
- use no quoting, but still do NOT escape characters that "require" it 
  in unquoted full DNs
- use no quoting, and DO escape characters that require it in unquoted DNs.

We have no immediate use for the third case, but perhaps the code should be 
prepared to provide it if it is ever wanted.

I'll produce another patch.
Comment 20 Nelson Bolyard (seldom reads bugmail) 2009-03-12 22:50:59 PDT
Created attachment 367178 [details] [diff] [review]
patch v6 - attempt to address Bob's comments

In this patch, I replaced the boolean needsQuoting (and pointers to it) 
with a new enumerated type.  Hopefully, this is clearer.  
I will check that bonsai can diff this patch against the previous version 
before requesting review.
Comment 21 Robert Relyea 2009-03-13 18:25:29 PDT
Argh... line 1000 still has the PRBool needsQuoting parameter.

Everything else about the patch looks good to me.

bob
Comment 22 Nelson Bolyard (seldom reads bugmail) 2009-03-13 21:49:53 PDT
Created attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)

What's amazing is: my compiler didn't even emit a warning about that gaffe!
Let's try this again.
Comment 23 Nelson Bolyard (seldom reads bugmail) 2009-03-14 12:51:40 PDT
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)

Third time's a charm?
Comment 24 Nelson Bolyard (seldom reads bugmail) 2009-03-14 12:59:09 PDT
I'm running the latest attached patch in my browser as I write this.  
It's working as intended, but unfortunately, that does not fix the problems 
in the cert manager's cert display.  Evidently PSM uses its own code for 
that display, and that code has some of the same problems.  So, I will file 
separate bugs against PSM, and mark this bug as blocking them.  Unfortunately, 
I will need to "spell out" the attacks in more detail in those PSM bugs.
Comment 25 Robert Relyea 2009-03-16 16:22:29 PDT
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)

r+ good to go...

bob
Comment 26 Nelson Bolyard (seldom reads bugmail) 2009-03-17 00:32:37 PDT
cmd/lib/secutil.c;    new revision: 1.94; previous revision: 1.93
lib/certdb/alg1485.c; new revision: 1.32; previous revision: 1.31
lib/certdb/certi.h;   new revision: 1.28; previous revision: 1.27
lib/certdb/secname.c; new revision: 1.25; previous revision: 1.24

Thanks for the reviews, Bob.
Comment 27 Nelson Bolyard (seldom reads bugmail) 2009-03-17 00:56:57 PDT
I wonder if I should change certutil, while I'm at it, to refuse to generate
a cert from a CSR with an invalid OID encoding.  It might be just a few lines
of code.  I'll look into it.
Comment 28 Nelson Bolyard (seldom reads bugmail) 2009-03-17 01:48:25 PDT
I backed out the tiny change that removed the quotes from around subject
names in the output of certutil and pp.  The test scripts require those
quotes to be present.  :(

Checking in secutil.c; new revision: 1.95; previous revision: 1.94
Comment 29 Honza Bambas (:mayhemer) 2009-03-17 14:26:52 PDT
Why is not CERT_RFC1485_EscapeAndQuote in any header while it's being exported?
Comment 30 Nelson Bolyard (seldom reads bugmail) 2009-03-17 21:37:06 PDT
(In reply to comment #29)
> Why is not CERT_RFC1485_EscapeAndQuote in any header while it's being exported?

It's a bug, I'd say.  Please file a separate bug report on this.
The question is: what is the right fix? 
put it in a public header file?
or remove it from nss.def?
Comment 31 Honza Bambas (:mayhemer) 2009-03-18 10:27:22 PDT
+	} else {
+	    if (NEEDS_ESCAPE(c) || (SPECIAL_CHAR(c) && mode == fullEscape)) {
+		*dst++ = C_BACKSLASH;
+		*dst++ = c;
+	    }
+	    *dst++ = c;
 	}

Won't this code add c to dst twice in case it needs escaping?
Comment 32 Nelson Bolyard (seldom reads bugmail) 2009-03-18 13:53:13 PDT
Honza, thanks for reviewing that code.  You're right.  
I had originally coded that as:

+    } else if (NEEDS_ESCAPE(c) || (SPECIAL_CHAR(c) && mode == fullEscape)) {
+        *dst++ = C_BACKSLASH;
+        *dst++ = c;
+    } else {
+        *dst++ = c;
     }

And I decided to refactor it, but I forgot to remove the extra line.  
Sorry.  :(  I'll attach a patch.
Comment 33 Nelson Bolyard (seldom reads bugmail) 2009-03-18 13:56:36 PDT
Created attachment 368099 [details] [diff] [review]
patch - remove duplicate line (checked in)

Honza, please review.

As an aside, I think we need a test tool that allows testing this code 
without having to generate certs and/or CSRs to do it.
Comment 34 Honza Bambas (:mayhemer) 2009-03-18 14:04:23 PDT
Comment on attachment 368099 [details] [diff] [review]
patch - remove duplicate line (checked in)

Some test for this would be great. Code for the required length with modes and quoiting is not very clear to do in-head-debug review of it.
Comment 35 Honza Bambas (:mayhemer) 2009-03-18 14:57:40 PDT
I just wrote a test checking there is a away to create a certificate signed by a trusted CA faking any secure site when used with a DNS attack. W/o this bug and bug 483437 fixed (patch comes soon) it is absolutely invisible to the user that he is under an attack.

Btw, it seems that IE6 is also vulnerable to this attack.
Comment 36 Nelson Bolyard (seldom reads bugmail) 2009-03-18 16:17:42 PDT
Checking in certdb/alg1485.c; new revision: 1.33; previous revision: 1.32

I tested it with this command (as typed into the shell window)

certutil -R -d DB -a -o req -s 'CN=" foo, bar;  baz\+bag\\\\\\"\00 xyzabc "'
Comment 37 Honza Bambas (:mayhemer) 2009-03-18 16:19:21 PDT
Should we also escape this way extensions? I managed to create a valid server certificate with dns alt name "www.bank.com\0www.bad-guy.com", connection to www.bank.com succeeds because we do PORT_Strcasecmp on the extension data, unescaped. None of the patches for this bug, bug 483440 and bug 483437 affects it.
Comment 38 Nelson Bolyard (seldom reads bugmail) 2009-03-18 16:24:39 PDT
Honza, 
Where do we do this "PORT_Strcasecmp on the extension data, unescaped" ?
Comment 39 Nelson Bolyard (seldom reads bugmail) 2009-03-18 16:30:07 PDT
The answer to the question in comment 37 is that we should always 
escape DNS names any time we take them out of a certificate and turn
them into a nul-terminated string.  That includes DNS names in 
subjectAltNames and in GeneralNames wherever they occur.
Comment 40 Nelson Bolyard (seldom reads bugmail) 2009-03-18 16:37:13 PDT
I see that the code at 
http://mxr.mozilla.org/security/source/security/nss/lib/certdb/certdb.c#1551
needs to be changed to use escaping.  But that is a different bug, in a 
different source file, so let's file another bug on that, and work on it 
there.
Comment 41 Nelson Bolyard (seldom reads bugmail) 2009-04-11 13:00:37 PDT
While reviewing Bug 483440, I realized that the NSS patch committed for THIS
bug makes an incorrect assumption, and is still exploitable.  I will shortly
attach a patch to fix it.
Comment 42 Nelson Bolyard (seldom reads bugmail) 2009-04-11 13:09:04 PDT
Created attachment 372238 [details] [diff] [review]
patch, part 3, v1 - don't assume last byte has a zero continuation bit (checked in)

The previous patch assumed that the last byte of the OID string always 
has a zero most-significant bit.  That's a false assumption. 
This patch corrects it.  Honza, please review.
Comment 43 Honza Bambas (:mayhemer) 2009-04-11 14:28:42 PDT
Comment on attachment 372238 [details] [diff] [review]
patch, part 3, v1 - don't assume last byte has a zero continuation bit (checked in)

I am studding the CERT_GetOidString code. What happens when the first byte has bit 0x80 set? It seems we instead of treating it as a single byte and decomposing this single byte by /40 and %40 we treat also it as an arbitrary integer. I compare with PSM code of GetDefaultOIDFormat function.

Patch looks good.
Comment 44 Nelson Bolyard (seldom reads bugmail) 2009-04-11 14:42:08 PDT
Thanks for the review. 

Rules for the encoding of the the first component are the same for any of the 
other components.  The high order bit is a "more bytes follow" bit.  After 
collecting 7 least signficant bits from each of the component's bytes, then 
the %40 and /40 logic should come to play.  In practice, it is rare to see 
the first byte with an continuation bit, but the code should also correctly 
handle it, since the point of this excersize is to handle the unusual cases.
Comment 45 Nelson Bolyard (seldom reads bugmail) 2009-04-11 15:59:38 PDT
Comment on attachment 372238 [details] [diff] [review]
patch, part 3, v1 - don't assume last byte has a zero continuation bit (checked in)

Thanks for the review.
Checking in certdb/alg1485.c; new revision: 1.34; previous revision: 1.33
Comment 46 Nelson Bolyard (seldom reads bugmail) 2009-04-11 22:41:04 PDT
Created attachment 372266 [details] [diff] [review]
patch part 4, v1 - fix bug in previous patch

The previous patch used a macro that lacks the case label.  
This patch puts the case label back explicitly.
Comment 47 Alexei Volkov 2009-04-12 00:15:16 PDT
Comment on attachment 372266 [details] [diff] [review]
patch part 4, v1 - fix bug in previous patch

r=alexei
Comment 48 Honza Bambas (:mayhemer) 2009-04-12 02:42:52 PDT
Grrr... I'm blind.
Comment 49 Nelson Bolyard (seldom reads bugmail) 2009-04-12 03:36:14 PDT
Declaring victory (again :).
Comment 50 Johnathan Nightingale [:johnath] 2009-04-16 12:23:38 PDT
Given the recent churn, is it safe to assume that this bug is not (properly) fixed in the NSS_3_12_3_RC0 tag pulled onto the 1.9.1 branch?
Comment 51 Nelson Bolyard (seldom reads bugmail) 2009-04-16 13:25:35 PDT
Johnathan,
This bug covers a number of related issues, some of which are security 
vulnerabilities and some are not.  Most of them (all but one) were 
fixed in 3.12.3, and one was not.  The one that was not is not (IMO) a 
security vulnerability, because NSS makes no security decisions based 
on its results.  So, it is true that this bug was not 100% fixed in 3.12.3
but the security vulnerabilities (caused by this code) were.
Does that help?
Comment 52 Johnathan Nightingale [:johnath] 2009-04-16 13:36:28 PDT
(In reply to comment #51)
> Johnathan,
> This bug covers a number of related issues, some of which are security 
> vulnerabilities and some are not.  Most of them (all but one) were 
> fixed in 3.12.3, and one was not.  The one that was not is not (IMO) a 
> security vulnerability, because NSS makes no security decisions based 
> on its results.  So, it is true that this bug was not 100% fixed in 3.12.3
> but the security vulnerabilities (caused by this code) were.

> Does that help?

Very much so, thanks Nelson.  Based on this, I don't believe the post-NSS_3_12_3_RC0 portion of this bug blocks 1.9.1. It's not quite right to mark this fixed1.9.1 since not all of it has been fixed on 1.9.1, but I'm going to put it back in the blocker-nomination queue, to give drivers a chance to remove its blocking status.
Comment 53 Mike Beltzner [:beltzner, not reading bugmail] 2009-04-16 13:39:56 PDT
I think the right course here is to:

 - morph this to reflect the actual security part of the bug that is now fixed1.9.1 by virtue of us having taken the NSS_3_1_12_3_RC0 tag

 - mark this bug as fixed1.9.1

 - file a follow-up bug for the post work if we think it's necessary to track it in our Bugzilla, or just leave it to be something we'll pick up when we move to the next version of the NSS tag
Comment 54 Nelson Bolyard (seldom reads bugmail) 2009-04-16 13:48:11 PDT
The one issue that was not fixed for 3.12.3 was fixed a few days later.
The fix is already in the NSS tree, which is why this bug is resolved fixed.
We could file a separate bug for the one piece that was fixed after 3.12.3
RC0 (which is now also RTM, IINM), and mark it resolved fixed immediately.
Would that help with Firefox record keeping?  If so, let me know and I will.
Comment 55 Johnathan Nightingale [:johnath] 2009-04-16 13:59:30 PDT
I really appreciate the offer, but I think that this discussion should be adequate for that, if anyone ever wants to know what happened. I think filing a bug we all already know is fixed because it makes a process happy is giving too much credit to the process.

Marking this fixed1.9.1 for the parts that we considered important enough to block on in the first place.  As Mike says, we can pick up the follow-up fix in the next tag.

Thanks for your help understanding it, Nelson.
Comment 56 Nelson Bolyard (seldom reads bugmail) 2009-07-15 16:24:00 PDT
*** Bug 504449 has been marked as a duplicate of this bug. ***
Comment 57 Mark Cox 2009-07-27 02:47:51 PDT
Assigned CVE-2009-2408 (for this and bz#484111 together)
Comment 58 Kai Engert (:kaie) 2009-07-30 15:48:43 PDT
Adding Elio to cc list, he is going to work on the fixed NSS package for RHEL.
Comment 59 Jeff Walden [:Waldo] (remove +bmo to email) 2009-08-06 17:56:15 PDT
Honza, we should add a mochitest for this, shouldn't we?  As far as I can tell from comments here we haven't, and we're just relying on whatever testing NSS has, which at a glance at the patches here (didn't read all of them, just looked at likely candidates for containing one) it seems we don't have right now.
Comment 60 Honza Bambas (:mayhemer) 2009-08-07 01:27:07 PDT
(In reply to comment #59)
> Honza, we should add a mochitest for this, shouldn't we?

We already have it: http://mxr.mozilla.org/mozilla-central/find?text=&kind=text&string=test_bug480509.html

Note You need to log in before you can comment on or make changes to this bug.