Last Comment Bug 653672 - (CVE-2011-3232) Reproducible crash at js::RegExp::executeInternal
(CVE-2011-3232)
: Reproducible crash at js::RegExp::executeInternal
Status: VERIFIED FIXED
[sg:critical?] don't unhide until App...
: verified-aurora, verified-beta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86_64 Linux
: -- critical (vote)
: mozilla7
Assigned To: David Mandelin [:dmandelin]
:
: Jason Orendorff [:jorendorff]
Mentors:
: 605998 (view as bug list)
Depends on: 625600
Blocks: 664920
  Show dependency treegraph
 
Reported: 2011-04-29 01:37 PDT by Aki Helin
Modified: 2014-06-26 06:16 PDT (History)
10 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
affected
+
fixed
+
fixed
unaffected
unaffected


Attachments
Patch (1.68 KB, patch)
2011-05-26 16:40 PDT, David Mandelin [:dmandelin]
cdleary: review+
Details | Diff | Splinter Review
Patch from Apple (16.61 KB, patch)
2011-06-21 09:51 PDT, David Mandelin [:dmandelin]
cdleary: review+
Details | Diff | Splinter Review

Description Aki Helin 2011-04-29 01:37:24 PDT
User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

The crash looks like:

Program received signal SIGSEGV, Segmentation fault.
0x00007fffe5d60967 in ?? ()
(gdb) x/i $rip
0x7fffe5d60967: cmpw   $0xd,-0x4(%rdi,%rsi,2)
(gdb) p $rdi
$1 = 140736947844760
(gdb) p $rsi
$2 = 2147483650

and the values varied while the testcase was being minimized.

Reproducible: Always

Steps to Reproduce:
1. $ echo '<script> "".match(/(?:(?=g))|(?:m).{2147483648,}/); </script>' > rex.html
2. ...
3. $ firefox rex.html

Actual Results:  
Firefox crashes.

Expected Results:  
Firefox remains running.

Bug https://bugzilla.mozilla.org/show_bug.cgi?id=605998 sounds similar to this one, but since this is cleanly reproducible and looks potentially exploitable to me, filing this separately as a hidden bug to be on the safe side.

Crash state dump at https://crash-stats.mozilla.com/report/index/140f49fb-e689-48ff-88b8-131f22110429
Comment 1 Daniel Veditz [:dveditz] 2011-05-03 17:08:11 PDT
assuming the worst based on the crash stack
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2011-05-19 14:17:12 PDT
Not holding 5 for this, but we should see what we can do for 6. The fix on the table is a large change that may or may not make it for 6, but in the event that it doesn't we should look into fixing the security problem here with a smaller safer patch for 6.
Comment 4 Daniel Veditz [:dveditz] 2011-05-20 11:33:21 PDT
Assigning to dmandelin because he's got bug 625600.

That bug is basically "upgrade YARR". Does that mean you know that this bug is already fixed in the upstream YARR (therefore it's a sort of dupe/previously reported bug)? Are you simply hoping that it is? Or were simply going to wait until after that big change to figure out how to merge a fix in?

I'm concerned that the bug this depends on is blocked on a legal bug due to potential license incompatibility. This security bug exists in our current codebase and should absolutely not be blocked on a legal opinion about incorporating some other code.
Comment 5 David Mandelin [:dmandelin] 2011-05-23 17:33:22 PDT
(In reply to comment #4)
> Assigning to dmandelin because he's got bug 625600.
> 
> That bug is basically "upgrade YARR". Does that mean you know that this bug
> is already fixed in the upstream YARR (therefore it's a sort of
> dupe/previously reported bug)? Are you simply hoping that it is? Or were
> simply going to wait until after that big change to figure out how to merge
> a fix in?

For now, hoping. I figured that refreshing is required before we would really try to fix additional bugs in Yarr, so I might as well get that done, then see if those other bugs are taken care of them and then deal with if not.

> I'm concerned that the bug this depends on is blocked on a legal bug due to
> potential license incompatibility. This security bug exists in our current
> codebase and should absolutely not be blocked on a legal opinion about
> incorporating some other code.

The legal opinion doesn't block current progress. I worked around those libraries, by not using reference counting. The legal bug may tell us that we can use some actual replacements for the refcounting classes, which would keep us closer to Apple's code, which makes merges easier. But we are good to go with the current patch.
Comment 6 David Mandelin [:dmandelin] 2011-05-26 16:40:58 PDT
Created attachment 535503 [details] [diff] [review]
Patch

This stops the crash. The proximate bug is that Yarr computes a length delta for two adjacent disjuncts as

  [int] delta = [unsigned] minSize1 - [unsigned] minSize2

They need to check if at the end of the string only if delta > 0. But the computation above can return a positive number that overflows to negative when converted to an int. The wider type makes this not happen.
Comment 7 David Mandelin [:dmandelin] 2011-05-26 16:52:27 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=61585.
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-06-13 15:33:15 PDT
Comment on attachment 535503 [details] [diff] [review]
Patch

r+ (for non-SPARC systems) on this one test, but I'm not sure we want to commit this without a full audit.

One thing of note is that int64 won't be wider than int on ILP64 systems (like SPARC), but there are also a number of other cases where YARR-JIT subtracts unchecked-value unsigned offsets from each other and sticks them into an int. Most of the places where you see the pattern |int result = thing - other;| in this file is a potential candidate for overflow. Maybe instead of committing this we should perform a full audit? Things like setupAlternative/DisjunctionOffsets and backtrack have similar errors. IMO we should try to go through and construct a test case to demonstrate each one to prevent further regressions.
Comment 9 Daniel Veditz [:dveditz] 2011-06-15 10:18:21 PDT
CC'ing Chris Evans so he can check and make sure Chrome's version of YARR gets fixed or is unaffected.
Comment 10 Chris Evans 2011-06-15 17:07:23 PDT
Chrome uses irregexp, AFAIK... Aki, this doesn't affect Chrome does it?
Comment 11 Aki Helin 2011-06-15 23:11:48 PDT
Chris, irregexp isn't affected, so Chrome is safe. The JavaScriptCore of WebKit has a similar YARR though (http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/yarr/YarrJIT.cpp#L1211), so it might be a good idea to send the patch also upstream to WebKit after the other overflow candidates have been checked.
Comment 12 Chris Evans 2011-06-16 11:05:00 PDT
@Aki: Yeah, I don't think YARR is exposed to web pages in Chrome. Although you never know if someone might add regex support to CSS or something else tedious, and suddenly expose it. Let us know if you find anything.

Perhaps Safari's JavaScript engine uses YARR?
Comment 13 Daniel Veditz [:dveditz] 2011-06-16 13:59:41 PDT
Webkit bug has been filed (comment 7).  I can't access it so hopefully that means their security team is aware of the problem.
Comment 14 Brendan Eich [:brendan] 2011-06-16 15:59:55 PDT
(In reply to comment #12)
> Perhaps Safari's JavaScript engine uses YARR?

Yes.

/be
Comment 15 David Mandelin [:dmandelin] 2011-06-16 17:45:21 PDT
I announced in https://bugs.webkit.org/show_bug.cgi?id=61585 that I'm going to land next Thursday (Jun 23) unless they ask me not to.
Comment 16 David Mandelin [:dmandelin] 2011-06-21 09:51:24 PDT
Created attachment 540780 [details] [diff] [review]
Patch from Apple

Gavin Barraclough has created a much better, more comprehensive fix over in the WebKit bug, so we should take this instead (his fix rebased to our tree). The plan is still synchronized landing on Thursday.
Comment 17 Daniel Veditz [:dveditz] 2011-06-23 13:13:58 PDT
Didn't get an answer in the webkit bug, maybe will here:

If m_alternative can be large enough to cause this bug aren't lines like

    m_checked += alternative->m_minimumSize;

equally problematic (m_checked is an int)?
Comment 18 David Mandelin [:dmandelin] 2011-06-23 17:45:01 PDT
http://hg.mozilla.org/tracemonkey/rev/92dc501e8ef6
Comment 19 David Mandelin [:dmandelin] 2011-06-24 13:53:34 PDT
*** Bug 605998 has been marked as a duplicate of this bug. ***
Comment 20 Chris Leary [:cdleary] (not checking bugmail) 2011-06-27 11:41:40 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/92dc501e8ef6
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-07 13:36:09 PDT
Is the patch in this bug good for 6 (i.e. the beta branch)? If so, please request beta approval for the patch.
Comment 22 David Mandelin [:dmandelin] 2011-07-07 18:22:38 PDT
(In reply to comment #21)
> Is the patch in this bug good for 6 (i.e. the beta branch)? If so, please
> request beta approval for the patch.

Fx 6 has a much older import of Yarr, so the patch doesn't apply.
Comment 23 Johnny Stenback (:jst, jst@mozilla.com) 2011-07-14 13:29:29 PDT
David, what do you think of fixing this for 6? Are the differences large, is there significant risk involved in porting here? If we do take this for 6 we should do it asap.
Comment 24 David Mandelin [:dmandelin] 2011-07-14 15:29:10 PDT
(In reply to comment #23)
> David, what do you think of fixing this for 6? Are the differences large, is
> there significant risk involved in porting here? If we do take this for 6 we
> should do it asap.

None of the code affected by the patch from Apple even exists in 6. I don't even know the cause of the bug in 6. We'd have to start from scratch. I recommend not fixing for 6.
Comment 25 Daniel Veditz [:dveditz] 2011-09-08 21:13:57 PDT
Over in the Safari bug https://bugs.webkit.org/show_bug.cgi?id=61585#c16 they say
> Apple is targeting the fix to ship in a Safari update in September,
> marked as a security bug and crediting Aki Helin.
>
> Concerns about disclosure timelines should be discussed with the
> security@webkit.org list.

That should line up pretty well with our Sept 27 ship for Firefox 7.

Apple has assigned CVE-2011-3232 to this bug in YARR
Comment 26 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:36:21 PDT
qa+: tracking for fix verification on Firefox 7, see comment 0
Comment 27 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 09:33:16 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 ID:20110922153450
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a2) Gecko/20110926 Firefox/8.0a2 ID:20110926042011

Verified fixed on Ubuntu 11.04 64-bit using test in comment 0.

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