Closed
Bug 653672
(CVE-2011-3232)
Opened 14 years ago
Closed 14 years ago
Reproducible crash at js::RegExp::executeInternal
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla7
Tracking | Status | |
---|---|---|
firefox5 | - | wontfix |
firefox6 | - | affected |
firefox7 | + | fixed |
firefox8 | + | fixed |
status1.9.2 | --- | unaffected |
status1.9.1 | --- | unaffected |
People
(Reporter: aki.helin, Assigned: dmandelin)
References
Details
(Keywords: reporter-external, verified-aurora, verified-beta, Whiteboard: [sg:critical?] don't unhide until Apple's release [fixed-in-tracemonkey][qa!])
Attachments
(1 file, 1 obsolete file)
16.61 KB,
patch
|
cdleary
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
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?]
Comment 3•14 years ago
|
||
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:
--- → ?
Comment 4•14 years ago
|
||
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
Assignee | ||
Comment 5•14 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•14 years ago
|
status-firefox5:
--- → affected
status-firefox6:
--- → affected
status-firefox7:
--- → affected
tracking-firefox7:
--- → +
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
Updated•14 years ago
|
See Also: → https://bugs.webkit.org/show_bug.cgi?id=61585
Updated•14 years ago
|
status1.9.1:
--- → unaffected
status1.9.2:
--- → unaffected
Whiteboard: [sg:critical?] → [sg:critical?] YARR bug -- do we need to coordinate disclosure?
Comment 8•14 years ago
|
||
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+
Comment 9•14 years ago
|
||
CC'ing Chris Evans so he can check and make sure Chrome's version of YARR gets fixed or is unaffected.
Comment 10•14 years ago
|
||
Chrome uses irregexp, AFAIK... Aki, this doesn't affect Chrome does it?
Reporter | ||
Comment 11•14 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•14 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?
Comment 13•14 years ago
|
||
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•14 years ago
|
||
(In reply to comment #12)
> Perhaps Safari's JavaScript engine uses YARR?
Yes.
/be
Assignee | ||
Comment 15•14 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.
Assignee | ||
Comment 16•14 years ago
|
||
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)
Comment 17•14 years ago
|
||
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)?
Updated•14 years ago
|
Attachment #540780 -
Flags: review?(cdleary) → review+
Assignee | ||
Comment 18•14 years ago
|
||
Whiteboard: [sg:critical?] YARR bug -- do we need to coordinate disclosure? → [sg:critical?][fixed-in-tracemonkey]
Comment 20•14 years ago
|
||
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/92dc501e8ef6
Updated•14 years ago
|
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → mozilla7
Comment 21•14 years ago
|
||
Is the patch in this bug good for 6 (i.e. the beta branch)? If so, please request beta approval for the patch.
Assignee | ||
Comment 22•14 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.
Comment 23•14 years ago
|
||
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•14 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•14 years ago
|
Whiteboard: [sg:critical?][fixed-in-tracemonkey] → [sg:critical?] [see c#24 for tracking 6 status] [fixed-in-tracemonkey]
Updated•14 years ago
|
Comment 25•13 years ago
|
||
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]
Comment 26•13 years ago
|
||
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+]
Comment 27•13 years ago
|
||
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!]
Updated•13 years ago
|
Group: core-security
Updated•12 years ago
|
Flags: sec-bounty+
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•