The default bug view has changed. See this FAQ.
Bug 653672 (CVE-2011-3232)

Reproducible crash at js::RegExp::executeInternal

VERIFIED FIXED in Firefox 7

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Aki Helin, Assigned: dmandelin)

Tracking

({verified-aurora, verified-beta})

unspecified
mozilla7
x86_64
Linux
verified-aurora, verified-beta
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox5- wontfix, firefox6- affected, firefox7+ fixed, firefox8+ fixed, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey][qa!])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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
assuming the worst based on the crash stack
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: String → JavaScript Engine
Ever confirmed: true
QA Contact: string → general
Whiteboard: [sg:critical?]
(Assignee)

Updated

6 years ago
Depends on: 625600
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.
tracking-firefox5: --- → -
tracking-firefox6: --- → ?
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.
Assignee: general → dmandelin
tracking-firefox6: ? → +
(Assignee)

Comment 5

6 years ago
(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.

Updated

6 years ago
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox7: --- → +
(Assignee)

Comment 6

6 years ago
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.
Attachment #535503 - Flags: review?(cdleary)
(Assignee)

Comment 7

6 years ago
Filed https://bugs.webkit.org/show_bug.cgi?id=61585.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
Whiteboard: [sg:critical?] → [sg:critical?] YARR bug -- do we need to coordinate disclosure?
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.
Attachment #535503 - Flags: review?(cdleary) → review+
CC'ing Chris Evans so he can check and make sure Chrome's version of YARR gets fixed or is unaffected.

Comment 10

6 years ago
Chrome uses irregexp, AFAIK... Aki, this doesn't affect Chrome does it?
(Reporter)

Comment 11

6 years ago
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

6 years ago
@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?
Webkit bug has been filed (comment 7).  I can't access it so hopefully that means their security team is aware of the problem.
(In reply to comment #12)
> Perhaps Safari's JavaScript engine uses YARR?

Yes.

/be
(Assignee)

Comment 15

6 years ago
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.
Blocks: 664920
(Assignee)

Comment 16

6 years ago
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.
Attachment #535503 - Attachment is obsolete: true
Attachment #540780 - Flags: review?(cdleary)
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)?
Attachment #540780 - Flags: review?(cdleary) → review+
(Assignee)

Comment 18

6 years ago
http://hg.mozilla.org/tracemonkey/rev/92dc501e8ef6
Whiteboard: [sg:critical?] YARR bug -- do we need to coordinate disclosure? → [sg:critical?][fixed-in-tracemonkey]
(Assignee)

Updated

6 years ago
Duplicate of this bug: 605998
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/92dc501e8ef6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
status-firefox7: affected → fixed
Target Milestone: --- → mozilla7
Is the patch in this bug good for 6 (i.e. the beta branch)? If so, please request beta approval for the patch.
status-firefox5: affected → wontfix
status-firefox8: --- → fixed
tracking-firefox8: --- → +
(Assignee)

Comment 22

6 years ago
(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.
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.
(Assignee)

Comment 24

6 years ago
(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.

Updated

6 years ago
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?] [see c#24 for tracking 6 status] [fixed-in-tracemonkey]
tracking-firefox6: + → -
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
Alias: CVE-2011-3232
Whiteboard: [sg:critical?] [see c#24 for tracking 6 status] [fixed-in-tracemonkey] → [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey]
qa+: tracking for fix verification on Firefox 7, see comment 0
Whiteboard: [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey] → [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey][qa+]
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.
Status: RESOLVED → VERIFIED
Keywords: verified-aurora, verified-beta
Whiteboard: [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey][qa+] → [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey][qa!]
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.