Crash [@ QuoteString] or [@ js_NewStringCopyN] or "Assertion failure: limit >= start,"

VERIFIED FIXED in Firefox 20

Status

()

--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: gkw, Assigned: dvander)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla22
All
Mac OS X
assertion, crash, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox17 unaffected, firefox18 unaffected, firefox19+ wontfix, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr10 unaffected, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [jsbugmon:][adv-main20+], crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 695866 [details]
stack

"xy".match(/((x)??){2}y/)

asserts js debug shell on m-c changeset f5ed2691d901 without any CLI arguments at Assertion failure: limit >= start,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   116360:cd2eb9705765
user:        Gary Kwong
date:        Mon Dec 17 16:57:48 2012 -0800
summary:     Workaround YARR assert (bug 808478, r=sstangl).

I forgot to set the actual patch author when I was helping to land the patch in bug 808478. :-/ The original patch author was dvander.

Prior to that changeset, the testcase was asserting at Assertion failure: (&term - term.atom.parenthesesWidth)->inputPosition == term.inputPosition, which bug 808478 worked around.


This seems to be asserting in our regex code instead of YARR code. The original assert was added in bug 673188. I'm not sure though.
(Reporter)

Comment 1

6 years ago
"2\"".match("(((2)??)+\")")()

crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments at QuoteString. Turning s-s and setting sec-critical because it seems to be accessing a weird memory address.

The incriminating changeset landed only in 20, but that was a workaround, and the original purported regression (upgrading YARR) landed in 19, so setting flags accordingly.
Group: core-security
status-b2g18: --- → unaffected
status-firefox-esr10: --- → unaffected
status-firefox17: --- → unaffected
status-firefox18: --- → unaffected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → unaffected
tracking-firefox19: --- → ?
tracking-firefox20: --- → ?
Keywords: crash
Hardware: x86_64 → All
Summary: "Assertion failure: limit >= start," → Crash [@ QuoteString] or "Assertion failure: limit >= start,"
(Reporter)

Comment 2

6 years ago
Created attachment 696775 [details]
stack for testcase in comment 1
(Reporter)

Updated

6 years ago
Flags: needinfo?(dvander)
(Reporter)

Comment 3

6 years ago
"\u66d6J".split(/((\u66d6)??){7}J/)

is another testcase that crashes js opt shell at memmove$VARIANT$sse42 with js_NewStringCopyN on the stack on m-c changeset dec6aa71da64.
Crash Signature: [@ QuoteString] [@ js_NewStringCopyN]
Summary: Crash [@ QuoteString] or "Assertion failure: limit >= start," → Crash [@ QuoteString] or [@ js_NewStringCopyN] or "Assertion failure: limit >= start,"
(Reporter)

Comment 4

6 years ago
Created attachment 697286 [details]
stack for testcase in comment 3

The testcase in comment 3 also has the same assertion, and also seems to be accessing a weird memory address 0xfffa5a00.

Comment 5

6 years ago
(In reply to Gary Kwong [:gkw] from comment #1)
> "2\"".match("(((2)??)+\")")()
> 
> crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments
> at QuoteString. Turning s-s and setting sec-critical because it seems to be
> accessing a weird memory address.
> 
> The incriminating changeset landed only in 20, but that was a workaround,
> and the original purported regression (upgrading YARR) landed in 19, so
> setting flags accordingly.

Can we mark this bug with sg:crit or sg:high prior to tracking? It's not clear how we're rating this internally.
(Reporter)

Comment 6

6 years ago
> Can we mark this bug with sg:crit or sg:high prior to tracking? It's not
> clear how we're rating this internally.

> crashes js opt shell on m-c changeset 0d771761b9b3 without any CLI arguments
> at QuoteString. Turning s-s and setting sec-critical because it seems to be
> accessing a weird memory address.

Oops, I missed out setting the sec-critical keyword. Added it now.
Keywords: sec-critical

Updated

6 years ago
tracking-firefox19: ? → +
tracking-firefox20: ? → +

Comment 7

6 years ago
David - this is an sg:crit JS regression in FF19. Can you help find an owner?
Assignee: general → dvander
(Reporter)

Updated

6 years ago
status-firefox21: --- → affected
tracking-firefox21: --- → ?
This was introduced with the Yarr import, which I did, so I'll take it.
Status: NEW → ASSIGNED
Flags: needinfo?(dvander)
tracking-firefox21: ? → +

Comment 9

6 years ago
(In reply to David Anderson [:dvander] from comment #8)
> This was introduced with the Yarr import, which I did, so I'll take it.

Any updates? We're now coming up on beta 4, and this is a FF19 regression. We don't have much time to resolve.
Crash Signature: [@ QuoteString] [@ js_NewStringCopyN] → [@ QuoteString] [@ js_NewStringCopyN]
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unknown exception (check manually)
Initial analysis is that this was introduced in https://bugs.webkit.org/show_bug.cgi?id=73728 (rev 108858). There are some subtraction expressions that were casually swapped, and this causes problems for certain inputs (i.e. 0 - -1 == 1, instead of -1 - 0 == -1).

I have no idea what the right fix for this is. I don't have access to the WebKit bug that made these changes, so maybe there was another underlying security issue. We could (1) revert the expressions back to what was in Firefox 18, since we don't know of any outstanding problems there. We could (2) revert just the ones causing this particular bug. We could (3) make these regular expressions throw a JavaScript exception, but that might break the web.

I'll try-run a patch for (2) and see what happens.

Filed WebKit bug @ https://bugs.webkit.org/show_bug.cgi?id=107898
Crash Signature: [@ QuoteString] [@ js_NewStringCopyN] → [@ QuoteString] [@ js_NewStringCopyN]
Created attachment 706247 [details] [diff] [review]
patch

verified to fix the test cases in this bug
Update: that idea totally failed on tbpl. Back to the drawing board.
Gary, I think we should backout the original patch, and disable the assert. Any thoughts on this? It doesn't seem like there is a security issue when the assert fires - for all we know it could be bogus, and fuzzing opt builds will tell us if/when there is a serious problem.
(Reporter)

Comment 15

6 years ago
(In reply to David Anderson [:dvander] from comment #14)
> Gary, I think we should backout the original patch, and disable the assert.
> Any thoughts on this? It doesn't seem like there is a security issue when
> the assert fires - for all we know it could be bogus, and fuzzing opt builds
> will tell us if/when there is a serious problem.

Sure, let's try that. Shall we also add the testcases in this bug while we're at it?
Created attachment 714659 [details] [diff] [review]
fix

This reverts the previous patch, but leaves the ASSERT disabled.
Attachment #706247 - Attachment is obsolete: true
Attachment #714659 - Flags: review?(sstangl)

Updated

6 years ago
Attachment #714659 - Flags: review?(sstangl) → review+
Comment on attachment 714659 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?

Regressing cset landed on m-c on Dec 17 and no prior branches.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch will apply.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.
Attachment #714659 - Flags: sec-approval?
Comment on attachment 714659 [details] [diff] [review]
fix

Sec-approval+. It would have been nice to have gotten this into 19 since then we would never have shipped this issue.
Attachment #714659 - Flags: sec-approval? → sec-approval+
status-firefox22: --- → affected
tracking-firefox22: --- → +
(Reporter)

Comment 19

6 years ago
I have verified that the patch here fixes the testcase in bug 828019 as well.
(Reporter)

Comment 21

6 years ago
This was landed.

http://hg.mozilla.org/mozilla-central/rev/4e5f82315a79
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox22: affected → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Status: RESOLVED → VERIFIED
Crash Signature: [@ QuoteString] [@ js_NewStringCopyN] → [@ QuoteString] [@ js_NewStringCopyN]
JSBugMon: This bug has been automatically verified fixed.
Let's get this nominated for uplift in that case, with a risk assessment, if we're going to take this in FF20 we really would want to have it landed before Tuesday's go to build on beta5.
Crash Signature: [@ QuoteString] [@ js_NewStringCopyN] → [@ QuoteString] [@ js_NewStringCopyN]
Comment on attachment 714659 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 808478
User impact if declined: possible sg:crit
Testing completed (on m-c, etc.): yes
Risk to taking this patch (and alternatives if risky): none
String or UUID changes made by this patch: none
Attachment #714659 - Flags: approval-mozilla-beta?
Attachment #714659 - Flags: approval-mozilla-aurora?
Attachment #714659 - Flags: approval-mozilla-beta?
Attachment #714659 - Flags: approval-mozilla-beta+
Attachment #714659 - Flags: approval-mozilla-aurora?
Attachment #714659 - Flags: approval-mozilla-aurora+
Comment on attachment 714659 [details] [diff] [review]
fix

low risk sec-crit regression, approving for uplift.
Please land by tonight(3/11) or early morning PT  to get it into our next beta.
status-firefox19: affected → wontfix
status-firefox20: affected → fixed
status-firefox21: affected → fixed
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.