Closed Bug 480509 Opened 15 years ago Closed 15 years ago

Improper character escaping and unescaping in alg1485.c & secname.c

Categories

(NSS :: Libraries, defect, P1)

x86
All
defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.3

People

(Reporter: nelson, Assigned: nelson)

References

Details

(Keywords: fixed1.9.0.13, fixed1.9.0.14, fixed1.9.1, Whiteboard: [sg:high])

Attachments

(6 files, 8 obsolete files)

749 bytes, text/plain
Details
734 bytes, text/plain
Details
26.78 KB, patch
rrelyea
: review+
Details | Diff | Splinter Review
550 bytes, patch
mayhemer
: review+
Details | Diff | Splinter Review
1.28 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
525 bytes, patch
alvolkov.bgs
: review+
Details | Diff | Splinter Review
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.
Attached patch Work in Progress (obsolete) — Splinter Review
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.
Attached patch Work in Progress - v2 (obsolete) — Splinter Review
This seems to solve the unwanted quoting problem.  
Still must tackle quoting of UTF8 char fragments.
Need more test certs.
Attachment #364604 - Attachment is obsolete: true
Getting closer.
Attachment #364614 - Attachment is obsolete: true
Attachment #364741 - Attachment is patch: true
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.
Priority: -- → P1
Attached patch patch v4 - for review (obsolete) — Splinter 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.
Attachment #364741 - Attachment is obsolete: true
Attachment #365118 - Flags: superreview?(wtc)
Attachment #365118 - Flags: review?(rrelyea)
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.
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.
Honza, Thanks.  Please attach that cert to this bug.  It seems useful as a 
test case.  Thanks.
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).
(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.
Attachment #365118 - Attachment is obsolete: true
Attachment #366443 - Flags: superreview?(wtc)
Attachment #366443 - Flags: review?(rrelyea)
Attachment #365118 - Flags: superreview?(wtc)
Attachment #365118 - Flags: review?(rrelyea)
Bugzilla fails to diff v5 (above) with v4 of this patch. 
Maybe it will be able to properly diff this v5 against v4.
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.
Attachment #366447 - Attachment is obsolete: true
Attached patch interdiff of v4 and v5 (obsolete) — Splinter Review
If this works, I think Wan-Teh will find this will be easier to review 
than a complete from-scratch review of v5.
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.
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
Seeking patch review from any NSS peer
Attachment #366443 - Flags: review?(julien.pierre.boogz)
Attachment #366477 - Attachment description: sample PEM encoded PKCS10 certificate request → sample PEM encoded PKCS10 certificate request attack
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
Attachment #366443 - Flags: review?(rrelyea) → review-
Flags: blocking1.9.1?
Whiteboard: [sg:high]
(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.
Flags: blocking1.9.1? → blocking1.9.1+
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.
Attachment #366443 - Attachment is obsolete: true
Attachment #366468 - Attachment is obsolete: true
Attachment #366443 - Flags: superreview?(wtc)
Attachment #366443 - Flags: review?(julien.pierre.boogz)
Attachment #367178 - Flags: review?(rrelyea)
Attachment #367178 - Flags: review?(wtc)
Argh... line 1000 still has the PRBool needsQuoting parameter.

Everything else about the patch looks good to me.

bob
What's amazing is: my compiler didn't even emit a warning about that gaffe!
Let's try this again.
Attachment #367178 - Attachment is obsolete: true
Attachment #367178 - Flags: review?(wtc)
Attachment #367178 - Flags: review?(rrelyea)
Attachment #367350 - Attachment is patch: true
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)

Third time's a charm?
Attachment #367350 - Flags: review?(rrelyea)
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.
Blocks: 483437
Blocks: 483440
Attachment #367350 - Flags: review?(rrelyea) → review+
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)

r+ good to go...

bob
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.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.
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
Why is not CERT_RFC1485_EscapeAndQuote in any header while it's being exported?
(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?
+	} 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?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #368099 - Flags: review?(honzab.moz)
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.
Attachment #368099 - Flags: review?(honzab.moz) → review+
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.
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 "'
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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.
Honza, 
Where do we do this "PORT_Strcasecmp on the extension data, unescaped" ?
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.
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.
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #367350 - Attachment description: patch v7 - aargh indeed! → patch v7 - aargh indeed! (checked in)
Attachment #368099 - Attachment description: patch - remove duplicate line → patch - remove duplicate line (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.
Attachment #372238 - Flags: review?(honzab.moz)
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.
Attachment #372238 - Flags: review?(honzab.moz) → review+
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 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
Attachment #372238 - Attachment description: patch, part 3, v1 - don't assume last byte has a zero continuation bit → patch, part 3, v1 - don't assume last byte has a zero continuation bit (checked in)
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The previous patch used a macro that lacks the case label.  
This patch puts the case label back explicitly.
Attachment #372266 - Flags: review?(alexei.volkov.bugs)
Attachment #372266 - Flags: review?(alexei.volkov.bugs) → review+
Comment on attachment 372266 [details] [diff] [review]
patch part 4, v1 - fix bug in previous patch

r=alexei
Grrr... I'm blind.
Declaring victory (again :).
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
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?
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?
(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.
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
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.
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.
Keywords: fixed1.9.1
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Flags: blocking1.9.0.13? → blocking1.9.0.13+
Depends on: 500495
Assigned CVE-2009-2408 (for this and bz#484111 together)
Adding Elio to cc list, he is going to work on the fixed NSS package for RHEL.
Group: core-security
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.
(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
You need to log in before you can comment on or make changes to this bug.