Closed
Bug 480509
Opened 16 years ago
Closed 16 years ago
Improper character escaping and unescaping in alg1485.c & secname.c
Categories
(NSS :: Libraries, defect, P1)
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.
Assignee | ||
Comment 1•16 years ago
|
||
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.
Assignee | ||
Comment 2•16 years ago
|
||
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
Assignee | ||
Updated•16 years ago
|
Attachment #364741 -
Attachment is patch: true
Assignee | ||
Comment 4•16 years ago
|
||
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
Assignee | ||
Comment 5•16 years ago
|
||
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)
Assignee | ||
Comment 6•16 years ago
|
||
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•16 years ago
|
||
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.
Assignee | ||
Comment 8•16 years ago
|
||
Honza, Thanks. Please attach that cert to this bug. It seems useful as a
test case. Thanks.
Comment 9•16 years ago
|
||
Comment 10•16 years ago
|
||
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).
Assignee | ||
Comment 11•16 years ago
|
||
(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)
Assignee | ||
Comment 12•16 years ago
|
||
Bugzilla fails to diff v5 (above) with v4 of this patch.
Maybe it will be able to properly diff this v5 against v4.
Assignee | ||
Comment 13•16 years ago
|
||
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
Assignee | ||
Comment 14•16 years ago
|
||
If this works, I think Wan-Teh will find this will be easier to review
than a complete from-scratch review of v5.
Assignee | ||
Comment 15•16 years ago
|
||
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.
Assignee | ||
Comment 16•16 years ago
|
||
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
Assignee | ||
Comment 17•16 years ago
|
||
Seeking patch review from any NSS peer
Assignee | ||
Updated•16 years ago
|
Attachment #366443 -
Flags: review?(julien.pierre.boogz)
Assignee | ||
Updated•16 years ago
|
Attachment #366477 -
Attachment description: sample PEM encoded PKCS10 certificate request → sample PEM encoded PKCS10 certificate request attack
Comment 18•16 years ago
|
||
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-
Updated•16 years ago
|
Flags: blocking1.9.1?
Updated•16 years ago
|
Whiteboard: [sg:high]
Assignee | ||
Comment 19•16 years ago
|
||
(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.
Updated•16 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Assignee | ||
Comment 20•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #367178 -
Flags: review?(rrelyea)
Assignee | ||
Updated•16 years ago
|
Attachment #367178 -
Flags: review?(wtc)
Comment 21•16 years ago
|
||
Argh... line 1000 still has the PRBool needsQuoting parameter.
Everything else about the patch looks good to me.
bob
Assignee | ||
Comment 22•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Attachment #367350 -
Attachment is patch: true
Assignee | ||
Comment 23•16 years ago
|
||
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)
Third time's a charm?
Attachment #367350 -
Flags: review?(rrelyea)
Assignee | ||
Comment 24•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #367350 -
Flags: review?(rrelyea) → review+
Comment 25•16 years ago
|
||
Comment on attachment 367350 [details] [diff] [review]
patch v7 - aargh indeed! (checked in)
r+ good to go...
bob
Assignee | ||
Comment 26•16 years ago
|
||
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: 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 27•16 years ago
|
||
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.
Assignee | ||
Comment 28•16 years ago
|
||
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•16 years ago
|
||
Why is not CERT_RFC1485_EscapeAndQuote in any header while it's being exported?
Assignee | ||
Comment 30•16 years ago
|
||
(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•16 years ago
|
||
+ } 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?
Assignee | ||
Comment 32•16 years ago
|
||
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 → ---
Assignee | ||
Comment 33•16 years ago
|
||
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 34•16 years ago
|
||
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+
Comment 35•16 years ago
|
||
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.
Assignee | ||
Comment 36•16 years ago
|
||
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: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 37•16 years ago
|
||
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.
Assignee | ||
Comment 38•16 years ago
|
||
Honza,
Where do we do this "PORT_Strcasecmp on the extension data, unescaped" ?
Assignee | ||
Comment 39•16 years ago
|
||
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.
Assignee | ||
Comment 40•16 years ago
|
||
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.
Assignee | ||
Comment 41•16 years ago
|
||
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 → ---
Assignee | ||
Updated•16 years ago
|
Attachment #367350 -
Attachment description: patch v7 - aargh indeed! → patch v7 - aargh indeed! (checked in)
Assignee | ||
Updated•16 years ago
|
Attachment #368099 -
Attachment description: patch - remove duplicate line → patch - remove duplicate line (checked in)
Assignee | ||
Comment 42•16 years ago
|
||
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 43•16 years ago
|
||
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+
Assignee | ||
Comment 44•16 years ago
|
||
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.
Assignee | ||
Comment 45•16 years ago
|
||
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)
Assignee | ||
Updated•16 years ago
|
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•16 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 46•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #372266 -
Flags: review?(alexei.volkov.bugs) → review+
Comment 47•16 years ago
|
||
Comment on attachment 372266 [details] [diff] [review]
patch part 4, v1 - fix bug in previous patch
r=alexei
Comment 48•16 years ago
|
||
Grrr... I'm blind.
Assignee | ||
Comment 49•16 years ago
|
||
Declaring victory (again :).
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Comment 50•16 years ago
|
||
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?
Assignee | ||
Comment 51•16 years ago
|
||
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•16 years ago
|
||
(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•16 years ago
|
||
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
Assignee | ||
Comment 54•16 years ago
|
||
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•16 years ago
|
||
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
Updated•15 years ago
|
Flags: wanted1.9.1.x+
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.13?
Updated•15 years ago
|
Flags: blocking1.9.0.13? → blocking1.9.0.13+
Comment 57•15 years ago
|
||
Assigned CVE-2009-2408 (for this and bz#484111 together)
Comment 58•15 years ago
|
||
Adding Elio to cc list, he is going to work on the fixed NSS package for RHEL.
Updated•15 years ago
|
Group: core-security
Updated•15 years ago
|
Keywords: fixed1.9.0.13,
fixed1.9.0.14
Comment 59•15 years ago
|
||
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•15 years ago
|
||
(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.
Description
•