"Assertion failure: (c2 <= cs->length) && (c1 <= c2)" with /[\Wb-G]/

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: Brian Crowder)

Tracking

({assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 -
blocking1.8.0.13 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:nse?] patch broke GMail)

Attachments

(5 attachments, 8 obsolete attachments)

(Reporter)

Description

11 years ago
js> /[\Wb-G]/.exec("")
Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2233

Security-sensitive for now because the code involved is playing with bit arrays.
Created attachment 260024 [details] [diff] [review]
Fix

Although we've found our max when we process a \W, we still need to complete processing the character set to find invalid ranges.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #260024 - Flags: review?(crowder)
FWIW, I don't think this is exploitable. We allocate the maximum bitmap size thanks the the \W and then we add an invalid character range, which I believe will cause incorrect results (it looks like we invert the range) but shouldn't crash. I'm not entirely sure though.
(Assignee)

Updated

11 years ago
Attachment #260024 - Flags: review?(crowder) → review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Comment on attachment 260024 [details] [diff] [review]
Fix

This is a pretty safe correctness fix.
Attachment #260024 - Flags: approval1.8.1.4?
Attachment #260024 - Flags: approval1.8.0.12?

Comment 5

11 years ago
sounds like something that should go on the branch after a tiny bit of baking on the trunk
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Bug has "crash" keyword, but comment 2 says it shouldn't crash. Changing keyword.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Keywords: crash → assertion
Whiteboard: [sg:nse?]
(Reporter)

Comment 7

11 years ago
I've been using the "crash" keyword for all JavaScript Engine assertions, because those assertions are fatal.
(In reply to comment #7)
> I've been using the "crash" keyword for all JavaScript Engine assertions,
> because those assertions are fatal.

DEBUG build only -- a release build may or may not crash, so I don't think this keyword is appropriate for assertbotches.

/be
I've backed out this patch because it broke Gmail (bug 375931).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 375931
Whiteboard: [sg:nse?] → [sg:nse?] patch broke GMail
Sigh, reduced testcase is: /[\s-:]/

With this patch \s sets localMax to be 65535, so when we ensure that the end of the range is >= the beginning, we throw an error.
Created attachment 260173 [details] [diff] [review]
Fixed fix

This artifically sets localMax to 0 to ensure that whatever character is next in the range is valid (which I think is correct, there is no maximum character for any of the ranges).
Attachment #260024 - Attachment is obsolete: true
Attachment #260173 - Flags: review?(crowder)
Attachment #260024 - Flags: approval1.8.1.4?
Attachment #260024 - Flags: approval1.8.0.12?
(Assignee)

Updated

11 years ago
Attachment #260173 - Flags: review?(crowder) → review+
Please ask for branch approval after this has been on the trunk a few days, (but we probably won't hold the release for this one so not a 'blocker')
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
(Reporter)

Comment 13

11 years ago
Even with the patch, I get the same assertion with some other testcases:

js> /[_-t]/i.exec("")
Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2237

js> (new RegExp("[\xDF-\xC7]]", "i")).exec("")
Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2237
(Assignee)

Comment 14

11 years ago
Created attachment 261759 [details] [diff] [review]
better handling for fold flag on some ranges of characters

The oddly-named "fold" flag screws us up here because we end up potentially adding an invalid range here.  Fold implies two ranges, 1) the [downcase(start), downcase(end)], and 2) the [upcase(start), upcase(end)] range.  For characters that cannot be up/down-cased (ie., the up/down-cased value is the same) this can yield a range where the start value is higher than the end value; thus an assertion.  I don't think this can be made to yield a crash in release, but I am not convinced that it might not yield invalid results, at least.
Assignee: mrbkap → crowder
Status: REOPENED → ASSIGNED
Attachment #261759 - Flags: review?(mrbkap)
Comment on attachment 261759 [details] [diff] [review]
better handling for fold flag on some ranges of characters

Add braces around the multilined ifs and r=mrbkap
Attachment #261759 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 16

11 years ago
Added the parens requested by mrbkap and landed:

jsregexp.c: 3.149
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 17

11 years ago
Created attachment 263489 [details]
ecma_3/RegExp/regress-375715-01.js

In Linux shell from today, with this test still get

Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2231
Trace/breakpoint trap

Comment 18

11 years ago
Created attachment 263491 [details]
ecma_3/RegExp/regress-375715-02.js

Comment 19

11 years ago
Created attachment 263492 [details]
ecma_3/RegExp/regress-375715-03.js

Comment 20

11 years ago
Created attachment 263493 [details]
ecma_3/RegExp/regress-375715-04.js

Updated

11 years ago
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
(Assignee)

Comment 21

11 years ago
Created attachment 263501 [details] [diff] [review]
more checking (I hate this version, better coming up)
(Assignee)

Comment 22

11 years ago
Created attachment 263502 [details] [diff] [review]
better

Instead of asserting, just make this a runtime check in the most central location.
Attachment #263502 - Flags: review?(mrbkap)
(Assignee)

Comment 23

11 years ago
mrbkap: Of the last two patches, pick and r+ your fave.  My fave is the one I asked for review on.
(Assignee)

Comment 24

11 years ago
Created attachment 263503 [details] [diff] [review]
whoops, alligator is eating the wrong c

this one.
Attachment #263502 - Attachment is obsolete: true
Attachment #263502 - Flags: review?(mrbkap)
(Assignee)

Updated

11 years ago
Attachment #263503 - Flags: review?(mrbkap)
(Assignee)

Comment 25

11 years ago
Comment on attachment 263503 [details] [diff] [review]
whoops, alligator is eating the wrong c

Ok, no.  This whole thing is bogus.  We must report an invalid character range like this and bail.  A bug my patch introduces is that the first character of the range will still be added even if the range is invalid.  A patch that does the right reporting coming up.  Sorry for the bugspam.
Attachment #263503 - Attachment is obsolete: true
Attachment #263503 - Flags: review?(mrbkap)
(Assignee)

Comment 26

11 years ago
Created attachment 263686 [details] [diff] [review]
emit error messages for bad ranges
Attachment #263501 - Attachment is obsolete: true
Attachment #263686 - Flags: review?(mrbkap)
Why can't we catch these cases in CalculateBitmapSize?
Comment on attachment 263686 [details] [diff] [review]
emit error messages for bad ranges

Brian and I talked about this and he's going to come up with a comprehensive patch.
Attachment #263686 - Flags: review?(mrbkap)
(Assignee)

Comment 29

11 years ago
Created attachment 265703 [details] [diff] [review]
As suggested by mrbkap

This includes Blake's old patch and adds another range check that should catch the remaining cases in this bug.
Attachment #260173 - Attachment is obsolete: true
Attachment #263686 - Attachment is obsolete: true
Attachment #265703 - Flags: review?(mrbkap)
Comment on attachment 265703 [details] [diff] [review]
As suggested by mrbkap

This is missing Perl compatibility that we discussed (or, at least, doesn't bring sanity to the /[\s-:]/ regexp).
Attachment #265703 - Flags: review?(mrbkap) → review-
Comment on attachment 265703 [details] [diff] [review]
As suggested by mrbkap

Further investigation shows that this actually does match Perl's behavior.
Attachment #265703 - Flags: review- → review+
(Assignee)

Comment 32

11 years ago
Created attachment 265836 [details] [diff] [review]
fixing an off-by-one

Last patch introduced an off-by-one that caused all sorts of problems (found in extra testing before landing the patch).  This fixes that.
Attachment #265836 - Flags: review?(mrbkap)
Comment on attachment 265836 [details] [diff] [review]
fixing an off-by-one

>-            if (rangeStart > localMax) {
>+            if (rangeStart > localMax || rangeStart > localMax) {

This change seems to be unnecessary.
(Assignee)

Comment 34

11 years ago
Wow, not enough coffee today, huh?  Odd; I wonder how your original patch never landed, mrbkap, since that seems to be all that's needed here.
(Assignee)

Updated

11 years ago
Attachment #265836 - Attachment is obsolete: true
Attachment #265836 - Flags: review?(mrbkap)
(Assignee)

Updated

11 years ago
Attachment #265703 - Attachment is obsolete: true
(Assignee)

Comment 35

11 years ago
jsregexp.c: 3.150
Status: REOPENED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 36

11 years ago
Comment on attachment 260173 [details] [diff] [review]
Fixed fix

Landed this one.

jsregexp.c: 3.150
Attachment #260173 - Attachment is obsolete: false
Consensus was that this was not an exploitable problem? Please remove the security flag if that is true.
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?

Comment 38

11 years ago
I accidentally checked this test in rather than make it an attachment. Hopefully dveditz is correct and this is not a security issue.

The test must be negative since the error can not be caught and still exhibit the assertion.

/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-01-n.js,v  <--  regress-375715-01-n.js
initial revision: 1.1

Updated

11 years ago
Attachment #263489 - Attachment is obsolete: true
Group: security
Given comment 37 ("not exploitable") do we need this fix for the branches? I'm guessing not.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5-
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13-

Comment 40

11 years ago
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-02.js,v  <--  regress-375715-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-03.js,v  <--  regress-375715-03.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-04.js,v  <--  regress-375715-04.js
initial revision: 1.1

Comment 41

11 years ago
verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.