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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

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.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 260024 [details] [diff] [review]
Fix

This is a pretty safe correctness fix.

Comment 5

[chris hofmann]

sounds like something that should go on the branch after a tiny bit of baking on the trunk

Comment 6

[Daniel Veditz [:dveditz]]

Bug has "crash" keyword, but comment 2 says it shouldn't crash. Changing keyword.

Comment 7

[Jesse Ruderman]

I've been using the "crash" keyword for all JavaScript Engine assertions, because those assertions are fatal.

Comment 8

[Brendan Eich [:brendan]]

(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

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I've backed out this patch because it broke Gmail (bug 375931).

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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).

Comment 12

[Daniel Veditz [:dveditz]]

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')

Comment 13

[Jesse Ruderman]

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

Comment 14

[Brian Crowder]

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

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 16

[Brian Crowder]

Added the parens requested by mrbkap and landed:

jsregexp.c: 3.149

Comment 17

[Bob Clary [:bc] (inactive)]

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

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/RegExp/regress-375715-02.js

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/RegExp/regress-375715-03.js

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/RegExp/regress-375715-04.js

Comment 21

[Brian Crowder]

Attachment: more checking (I hate this version, better coming up)

Comment 22

[Brian Crowder]

Attachment: better

Instead of asserting, just make this a runtime check in the most central location.

Comment 23

[Brian Crowder]

mrbkap: Of the last two patches, pick and r+ your fave.  My fave is the one I asked for review on.

Comment 24

[Brian Crowder]

Attachment: whoops, alligator is eating the wrong c

this one.

Comment 25

[Brian Crowder]

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.

Comment 26

[Brian Crowder]

Attachment: emit error messages for bad ranges

Comment 27

[Blake Kaplan (:mrbkap) (inactive)]

Why can't we catch these cases in CalculateBitmapSize?

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 29

[Brian Crowder]

Attachment: As suggested by mrbkap

This includes Blake's old patch and adds another range check that should catch the remaining cases in this bug.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 31

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 265703 [details] [diff] [review]
As suggested by mrbkap

Further investigation shows that this actually does match Perl's behavior.

Comment 32

[Brian Crowder]

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

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

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.

Comment 34

[Brian Crowder]

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.

Comment 35

[Brian Crowder]

jsregexp.c: 3.150

Comment 36

[Brian Crowder]

Comment on attachment 260173 [details] [diff] [review]
Fixed fix

Landed this one.

jsregexp.c: 3.150

Comment 37

[Daniel Veditz [:dveditz]]

Consensus was that this was not an exploitable problem? Please remove the security flag if that is true.

Comment 38

[Bob Clary [:bc] (inactive)]

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

Comment 39

[Daniel Veditz [:dveditz]]

Given comment 37 ("not exploitable") do we need this fix for the branches? I'm guessing not.

Comment 40

[Bob Clary [:bc] (inactive)]

/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

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux/mac*/windows.