653672 - (CVE-2011-3232) Reproducible crash at js::RegExp::executeInternal

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Aki Helin]

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]]

assuming the worst based on the crash stack

Comment 3

[Johnny Stenback (:jst)]

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]]

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]]

(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]]

Attachment: 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]]

Filed https://bugs.webkit.org/show_bug.cgi?id=61585.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

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]]

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]

Chrome uses irregexp, AFAIK... Aki, this doesn't affect Chrome does it?

Comment 11

[Aki Helin]

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]

@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]]

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]]

(In reply to comment #12)
> Perhaps Safari's JavaScript engine uses YARR?

Yes.

/be

Comment 15

[David Mandelin [:dmandelin]]

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]]

Attachment: 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]]

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]]

http://hg.mozilla.org/tracemonkey/rev/92dc501e8ef6

Comment 20

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/92dc501e8ef6

Comment 21

[Johnny Stenback (:jst)]

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]]

(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)]

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]]

(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]]

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
> [e-mail] 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

[u279076]

qa+: tracking for fix verification on Firefox 7, see comment 0

Comment 27

[u279076]

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.