504456 - (CVE-2009-2404) Exploitable heap overflow in NSS shell expression (filename globbing) parsing

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Brandon Sterne (:bsterne)]

This bug was reported to security@m.o by Moxie Marlinspike.  From the email:

-----

The second [of two] is a heap overflow in the NSS code, for which the stars actually align and make it exploitable.

security/nss/lib/util/portreg.c:

141 static int
142 _handle_union(const char *str, const char *exp, PRBool case_insensitive)
143 {
144     char *e2 = (char *) PORT_Alloc(sizeof(char)*strlen(exp));
145     register int t,p2,p1 = 1;
146     int cp;
147
148     while(1) {
149         for(cp=1;exp[cp] != ')';cp++)
150             if(exp[cp] == '\\')
151                 ++cp;
152         for(p2 = 0;(exp[p1] != '|') && (p1 != cp);p1++,p2++) {
153             if(exp[p1] == '\\')
154                 e2[p2++] = exp[p1++];
155             e2[p2] = exp[p1];
156         }
157         for (t=cp+1; ((e2[p2] = exp[t]) != 0); ++t,++p2) {}
158         if(_shexp_match(str,e2, case_insensitive) == MATCH) {
159             PORT_Free(e2);
160             return MATCH;
161         }
162         if(p1 == cp) {
163             PORT_Free(e2);
164             return NOMATCH;
165         }
166         else ++p1;
167     }
168 }

You malloc out based on the strlen() on line 144, but then copy -- and happily copy past '\0' -- until you see ')' on line 154.  A '\0' char can be placed in this string, wherever you want, by including a '~' in the string that is originally passed in to the parent function that is two levels up:

263 static int
264 port_RegExpMatch(const char *str, const char *xp, PRBool case_insensitive) {
265     register int x;
266     char *exp = 0;
267
268     exp = PORT_Strdup(xp);
269
270     if(!exp)
271         return 1;
272
273     for(x=strlen(exp)-1;x;--x) {
274         if((exp[x] == '~') && (exp[x-1] != '\\')) {
275             exp[x] = '\0';
276             if(_shexp_match(str,&exp[++x], case_insensitive) == MATCH)
277                 goto punt;
278             break;
279         }
280     }
281     if(_shexp_match(str,exp, case_insensitive) == MATCH) {
282         PORT_Free(exp);
283         return 0;
284     }
285
286   punt:
287     PORT_Free(exp);
288     return 1;
289 }

You can see what happens to the '~' on line 274.  So a string such as '(foo~bar)'  will allocate strlen(foo) and then overwrite the bytes "bar" past the allocated memory.

This heap overflow can be triggered through, for instance, the CN field of a certificate.  It seems like it shouldn't be possible to get arbitrary shellcode this far, but it actually is.  If you generate a cert with your shellcode in the CN, and then mark that ASN.1 string as type 0x03, NSS lets everything in without mangling it.

The two nice things about this are:

1) You don't even have to get this signed.  You can present a self-signed cert to NSS, and it'll go to validate the CN anyway in order to compile the complete list of problems with the certificate.

2) X509 is so fucked up that you can do almost all of the heap massaging and data placement you need with the certificate itself.  This means that it's possible to exploit this with <img> tags to https links, since most of what you need can come in through the SSL connection itself.  It makes scatter-shot exploitation easier, since you can post malicious img tags to forums and not have them filtered (in contrast to many browser exploits which require that lots of JS be rendered out).

Comment 1

[chris hofmann]

this might be announced at blackhat/defcon

http://blackhat.com/html/bh-usa-09/bh-usa-09-schedule.html july 29  1345 - 1500 
or
https://www.defcon.org/html/defcon-17/dc-17-speakers.html#Marlinspike

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This code is no longer used in NSS 3.21.3 (which is in FF 3.5) unless the 
user specifically enables backwards-compatible wildcard matching (which 
I'll bet NO USERS in the world have ever done).

The author confuses NSS and PSM functionality here.  NSS does not check 
for host name matches until after the cert passes validation.  If a cert 
does not pass validation, then its host name is never checked by NSS.  
However, it may be that PSM still invokes the host name matching code 
even for a cert that has failed validation.  That would be a PSM bug.
But this is moot for FF 3.5 anyway.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Regarding the claims about IMG tags, there are no vulnerabilities here UNLESS
the user chooses to "trust" a CA that is willing to issue such malformed certs.
A cert from an untrusted issuer will fail validation and no host name match
will be performed on it.  PSM will not do additional checks, because PSM does
not offer the user overrides for IMG tag connections.  So, the only way that
a user would be vulnerable would be if the user trusted a CA issuing these 
bad certs AND the user had enabled backward-compatible cert name matching in 
FF 3.5.  (Try this test: Can you even figure out how to do that?)

Comment 4

[Daniel Veditz [:dveditz]]

There are still a couple-hundred million Firefox 3.0 users. Still a problem for them, right?

And if NSS is fixed but PSM has the bug, Firefox users are still at risk either way.

What about self-signed certs? It is not too terribly hard to socially engineer people into accepting those, especially if they can be convinced the cert isn't trying to spoof their bank or some other site they care about. Firefox 3 makes it harder for people to click-through, but if you make the content sound enticing enough what can a temporary override hurt? Stupid, yes, but does not justify allowing them to be infected with malware.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Dan, I agree that there are numerous issues that are resolved in FF 3.5 
but are still unresolved in the old version of NSS used in FF 3.0.  
Correcting that is not an NSS bug/issue (as you know), but is a matter 
of Mozilla taking a newer NSS back to the 1.9.0 branch.  
What is the status of that effort?

Comment 6

[Daniel Veditz [:dveditz]]

Bug 500495, blocking Firefox 3.0.13  (3.0.12 just went to beta)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

In NSS 3.12.3, for FF 3.5, we decided to no longer use this 14+ year old 
shexp code in NSS by default because its syntax is non-standard and allows
wildcards to be used in a way that is more relaxed than RFC 2818 allows.
So we decided to switch to a scheme that follows that RFC more closely, by 
default.  

We left the old code in there, and provided a way for users to re-enable it.  
This was to allow corporate users whose certificates depend on the old shexp 
syntax to continue to use those certs while they arrange to get new ones.  
So, today, in NSS 3.12.3 and later (FF 3.5 and later), only users who have 
chosen to enable that old code are vulnerable.  They are a tiny minority 
(maybe zero) of Firefox 3.5 users.  For users of older software, the solution
is to upgrade to NSS 3.12.3, which is backwards binary compatible (except 
for the change to shexp matching).

There are a variety of things we could do to close that vulnerability for the
few remaining users who experience it.  They include:
a) remove the shexp code altogether, completely decommitting support for it.
  (I think this is a non-starter for NSS's corporate sponsors.)
b) Remove support for the troublesome '~' syntax, which is little understood
   anyway, I think, but preserve (most of) the rest of it.  
c) try to preserve the entire existing grammar, and fix the code to work as 
   it was originally expected/intended.  This may be a big job for code that 
   is on its way out.

Comment 8

[Kai Engert (:KaiE:)]

Adding Elio to CC list, who works on core NSS and is also working on Red Hat packaging of NSS.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> The author confuses NSS and PSM functionality here.  NSS does not check 
> for host name matches until after the cert passes validation.  If a cert 
> does not pass validation, then its host name is never checked by NSS.  
> However, it may be that PSM still invokes the host name matching code 
> even for a cert that has failed validation.  That would be a PSM bug.
> But this is moot for FF 3.5 anyway.


This bug claims that "exploit works with self-signed certs", too. I don't see a proof that this is true.

Nelson claims this can't be true for NSS, or must be a bug in PSM.

Investigate this detail might be irrelevant, because the exploit is said to work with correctly signed certs and with certs allowed by an exception, too.

Comment 10

[Daniel Veditz [:dveditz]]

We will most likely fix this for 1.8.1 and 1.9.0 by upgrading NSS, but it needs to get fixed one way or the other.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v1 for NSS Trunk (Work In Progress)

This expression comparison function was designed to work in two passes:
1. First  pass: determine if expression is valid
2. Second pass: determine if input string matches expression

The second pass intentionally omits many sanity tests (such as tests for 
unexpected NUL characters in unions) because the first pass should detect
and eliminate them all.  This is intended as a performance optimization.

The problem is that the first pass did not detect many expressions that the
second pass cannot handle.  In particular, the second pass was not designed
to handle nested unions, nor exclusions ('~' rules) within unions, but the 
first pass did not detect those things.  

This patch changes the first pass to detect those things.  I believe it 
fixes the vulnerabilities previously disclosed in comment 0 above, without reducing the capabilities of the function from its previous ones (because I believe it never did handle nested unions nor exclusions within unions correctly).  However, I suspect there may be more vulnerabilities in this code, 
and I am continuing to look for them.  So I am not yet requesting review.

@dveditz, Since this isn't crypto code, perhaps you can suggest some non-NSS
reviewers to help review it this weekend.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389270 [details] [diff] [review]
Patch v1 for NSS Trunk (Work In Progress)

Exclusions are fundamentally broken.  More work is needed.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v2 for NSS Trunk (Work In Progress)

This is better.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389275 [details] [diff] [review]
Patch v2 for NSS Trunk (Work In Progress)

This is lame. I have a MUCH better patch coming.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Attachment: New test program for this library function

I wrote a test program to facilitate testing this library function.
I also wrote a test script which I am still enhancing. Will attach soon.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v3 for trunk - for review

I will do more testing before requesting review.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Attachment: new test script (work in progress)

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Attachment: sample test script output

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 389397 [details] [diff] [review]
Patch v3 for trunk - for review

Please review for correctness.  Feel free to use attached test program and script to aid in evaluation.

Comment 20

[Mark Cox]

Josh Bressers would usually assign CVE names, but he is out this week so I'll assign it one instead.

Use CVE-2009-2404 (Please tell the original reporter that name)

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

As per comment 7, this doesn't affect stock mozilla-1.9.1, so setting flags appropriately; please reset if I'm wrong, here.

Comment 22

[:Gavin Sharp [email: [e-mail]]]

I think we should leave this on the wanted list, since the buggy code can still be enabled on 1.9.1.

Comment 23

[Robert Relyea]

Comment on attachment 389397 [details] [diff] [review]
Patch v3 for trunk - for review

General comments:

After trying to follow what the code is trying to do, and seeing all the corner cases, I would be in favor of moving toward excising it completely at some point:).

None of these are actual bugs, so perhaps the r- is undeserved, but it would be good to clear these up. Issues 1 & 2 are the ones most critical in my mind.

1. Handling ~:

It appears we still have a lot of code trying to handle ~ which appears to be dead code now. The Validate only allows one 'active' ~ (escaped ~ and ~ inside [] are ok and interpreted as just plain ~). That ~ is handled directly in port_RegExpMatch(). There can be no other occurances, yet we have code trying to do something with the ~, most notably lines 205, 216, 248, & 328-343.

2. Use of ret variable in _shexp_match.

The original version of _shexp_match had this bizzar mix of setting the return value and dropping out of the loop and simply returning with the expected return value. The new function converts almost all these instances to straight returns, but not all of them. Currently ret is being set at line 355 (but then used in a switch statement in which all cases return) and at lines 354 and 357, just before falling out to the if at 361, which, in turn drops out to line 364. It would be much easier to read if 354 and 357 simply return, The if check at 361 could be removed and the final return at line 364 would be simply:
   return str[x] ? NOMATCH : MATCH;

3. line 195 the comment claims exp[0] points to the previous delimiter, when in fact it always points to the initial open paren '('. and exp+sx always points to the first character after the previous delimiter. The code itself is correct.

4. Question about handling of ranges.

The comments do not say that ranges are limited to starting and ending with alphanumerics (though the restiction is reasonable and implemented in the old version of the code). Even though both sets of code (new and old) only allow alphanumerics as the starting and ending of a range, they allow matches of non-alphanumerics within those ranges. In fact it takes a range like B-b or 0-f as valid (though not intuitive). This is why the case insensitive range match is so complicated, to handle these oddball matchings. It also means matching specifying A-z and a-Z yield different results when matching case insensitive (the former matches all letters and all non-alphanumerics between z and A, while the latter matches those same non-alphanumerics, but only matches a,A,z,Z). This last issue is more a documentation issue, but it does hightlight why turning this code off by default was the correct behavior.

bob

Comment 24

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v4 - for review

Bob, thanks for your excellent review.  
I believe I have addressed all your comments.  
1. removed all the special '~' handling from _shexp_match.
2. removed variable ret entirely from _shexp_match.
3. corrected several comments and added several more at Julien's suggestion.
4. Yes, ranges are strange.  I've added more comments in the header file.
   I also simplified the case-insensitive range checking logic quite a bit.

Please let me know if my changes meet with your approval.

Comment 25

[Robert Relyea]

Comment on attachment 389599 [details] [diff] [review]
patch v4 - for review

r+ rrelyea

I particularly like the recasting of loops into for loops as well as addressing my comments.

bob

Comment 26

[Julien Pierre]

Comment on attachment 389395 [details] [diff] [review]
New test program for this library function

There are a couple minor issues with this patch :

1. no pathnames , though I assume it is meant to be in mozilla/security/nss/cmd/shexp

2. there are some C++ // comments in the program . They may break some compilers. This is the main reason for r-

3. typo in Usage - the word "strng" should be "string"

4. in Usage, the format of the [expected result] should be documented. I had to read the code to figure out that it wanted  0 for success and 1 for failure.

5. RFE :
I would much rather have the test program read multiple values from stdin or another file, so that it could do more than one test per program execution.

This would greatly facilitate running things like dbx check access to check for invalid boundary access, in a single dbx session.

Comment 27

[Julien Pierre]

Comment on attachment 389398 [details] [diff] [review]
new test script (work in progress)

This is OK, but I would prefer to have all the expressions in a data file rather than invoking the program multiple times. We may also want to add more expressions based on some recent review feedback.

Comment 28

[Julien Pierre]

Comment on attachment 389398 [details] [diff] [review]
new test script (work in progress)

This script needs #!/bin/sh at the beginning to run properly on Unix, also.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Patch v5 - (checked in)

At Jeff Walden's suggestion, I added additional test cases and found a few 
edge cases where we generated the wrong answer.  This patch corrects those.
Newer test script will follow.  Bob, I won't ask you to review this again.

Comment 30

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Newer test program and test script with more cases

Comment 31

[Alexei Volkov]

Comment on attachment 390410 [details] [diff] [review]
Patch v5 - (checked in)

r=alexei

Comment 32

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 390410 [details] [diff] [review]
Patch v5 - (checked in)

Checking in portreg.c; new revision: 1.5; previous revision: 1.4
Checking in portreg.h; new revision: 1.5; previous revision: 1.4

This patch is almost identical to the one that was reviewed (r+)
in bug 332173 by Jeff Walden.  I consider Jeff's and Alexei's 
two reviews to be sufficient.

Comment 33

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 390410 [details] [diff] [review]
Patch v5 - (checked in)

It applies fine to 1.8.

Comment 34

[Nelson Bolyard (seldom reads bugmail)]

There's another problem not caught by previous test cases. 
New test case and supplemental patch forthcoming shortly

Comment 35

[Nelson Bolyard (seldom reads bugmail)]

Attachment: supplemental patch, v1

This patch corrects the behavior for the following test case, which now fails:
$SHEXP '(A|/B)C(D|E)(F|g).(HI|jk)$' 'p/q/r/s/t' 1

It fails because the function returns -1 (INVALID_SXP) instead of 1 (NO MATCH).
While not a security vulnerability, it is a regression for code that does the
wrong thing with -1 return values.   

The patch is contributed by Jeff Walden <[e-mail]>

Comment 36

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 391497 [details] [diff] [review]
supplemental patch, v1

Wait:  Since I didn't write this patch, I can review it.  r+=nelson
But I'd still invite additional review.

Comment 37

[Nelson Bolyard (seldom reads bugmail)]

Checking in portreg.c; new revision: 1.6; previous revision: 1.5

Comment 38

[Mark Cox]

Moxie gave his talk at Blackhat yesterday, and while I find lots of mentions of the other issues he disclosed, it isn't clear if he mentioned this specific issue or not.  Moxie, if you are watching these emails, is this issue now public?

Comment 39

[juan becerra [:juanb]]

Nelson, is this something you could verify? We are going through bug verifications for 3.5.2.

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Does anyone here know who applied for and received CVE number 3009-2404,
which is now applied to this bug?

Comment 41

[Moxie]

(In reply to comment #38)
> Moxie gave his talk at Blackhat yesterday, and while I find lots of mentions of
> the other issues he disclosed, it isn't clear if he mentioned this specific
> issue or not.  Moxie, if you are watching these emails, is this issue now
> public?

Yes, this issue is now public, although I haven't released an exploit.

Comment 42

[Mark Cox]

c#40; Mitre write the descriptions but we can correct them if they get it wrong.  I pointed them at the Red Hat bug https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2009-2404 for this which clearly states it is an NSS issue.

Moxie's talk is available online now, PDF and Video and explains this issue and points to the code:
http://www.blackhat.com/presentations/bh-usa-09/MARLINSPIKE/BHUSA09-Marlinspike-DefeatSSL-SLIDES.pdf

Comment 43

[Nelson Bolyard (seldom reads bugmail)]

Can we ask Moxie to verify this?

Comment 45

[georgi - hopefully not receiving bugspam]

> They are a tiny minority (maybe zero) 

stay assured they are positive and *not zero*.

i am using it and i wouldn't like my ***ssl protected*** porn browsing to come to your attention...