1368105 - Assertion failure: offset < base()->length(), at js/src/vm/String.h:735 with dumpStringRepresentation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 0ed0fd886134 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu, run with --fuzzing-safe --ion-offthread-compile=off):

var s = "xxxxxxxx";
var t = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; // 31 characters
var r1 = t + s;
var r2 = r1 + s;
r2.match(/x/);
r3 = r2 + s;
r3.match(/x/);
parseModule(r2);
dumpStringRepresentation(r1);


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0883a8a6 in JSDependentString::baseOffset (this=0xf5583080) at js/src/vm/String.h:735
#0  0x0883a8a6 in JSDependentString::baseOffset (this=0xf5583080) at js/src/vm/String.h:735
#1  0x088272f0 in JSDependentString::dumpRepresentation (this=0xf5583080, fp=0xf7da3cc0 <_IO_2_1_stderr_>, indent=2) at js/src/vm/String.cpp:721
#2  0x08827175 in JSString::dumpRepresentation (this=<optimized out>, fp=<optimized out>, indent=<optimized out>) at js/src/vm/String.cpp:189
#3  0x0847956c in DumpStringRepresentation (cx=0xf793a800, argc=1, vp=0xf5250058) at js/src/builtin/TestingFunctions.cpp:3525
#4  0x08171fb6 in js::CallJSNative (cx=0xf793a800, native=0x84794c0 <DumpStringRepresentation(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
[...]
#18 main (argc=4, argv=0xffffd8a4, envp=0xffffd8b8) at js/src/shell/js.cpp:8467
eax	0x0	0
ebx	0x8d4dff4	148168692
ecx	0xf7da4864	-136689564
edx	0x0	0
esi	0xffffa530	-23248
edi	0xf5583080	-178769792
ebp	0xffffcc48	4294954056
esp	0xffffcc10	4294954000
eip	0x883a8a6 <JSDependentString::baseOffset() const+262>
=> 0x883a8a6 <JSDependentString::baseOffset() const+262>:	movl   $0x0,0x0
   0x883a8b0 <JSDependentString::baseOffset() const+272>:	ud2    



Marking s-s as a start, but I assume that it's shell-only and a problem with dumpStringRepresentation.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Jim, can you take a look?

Comment 2

[Jan de Mooij [:jandem]]

Just from glancing at this I'm not convinced it's a bug in dumpStringRepresentation, and the following testcase asserts without it:

var s = "xxxxxxxx";
var t = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; // 31 characters
var r1 = t + s;
var r2 = r1 + s;
r2.match(/x/);
r3 = r2 + s;
r3.match(/x/);
parseModule(r2);
res = r1.slice(0, 38);

Comment 3

[Jan de Mooij [:jandem]]

Oh wow, this may be an ancient bug in our extensible string code. I think what's happening is something like this:

(1) Construct an extensible string A that has sufficient unused capacity.

(2) Create a dependent string D that points to A and A's chars.

(3) Concatenate, B = A + "x". B is a rope.

(4) Flatten B. This turns B into an extensible string (steals A's buffer) and turns A into a dependent string.

(5) Flatten A. This will copy its chars to a new buffer to ensure null-termination.

(6) Now the dependent string D we created in (2) still points to A but into a different buffer.

Comment 4

[Jan de Mooij [:jandem]]

I think JSDependentString::baseOffset needs to be smarter when the base string is a JSUndepended string.

Longer-term we really need to get rid of flat strings (bug 1330776), the flattening causes a lot of bugs that are hard to reason about.

Comment 5

[Al Billings [:abillings - ex-MoCo]]

Jan, can we get someone actively working on this one? It's a sec-critical so we (critsmash triage) don't want it to get too stale.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Patch

This changes JSDependentString::baseOffset to return a Maybe<size_t> so we can return Nothing() if the base string is an undepended string (in this case its buffer is unrelated to the dependent string's chars).

I'm not sure if this is really exploitable - I think the bogus offset is added and subtracted so they cancel each other out and I wasn't able to come up with a test where we crash or read uninitialized memory. I didn't try very hard though and the fix is easy, so it's best to treat this as s-s.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Add test

Comment 8

[Jan de Mooij [:jandem]]

This goes back years.

Comment 9

[Gerry Chang [:gchang]]

Track 54+/55+/56+ as sec-critical.

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8879564 [details] [diff] [review]
Patch

Review of attachment 8879564 [details] [diff] [review]:
-----------------------------------------------------------------

This makes sense as a minimal branch patch; nice job tracking this down!  Since a dependent string's char* ultimately points into *some* string's char array that is transitively reachable via base(), I think an alternate fix would be to have baseOffset() iterate through (un)dependent string base()s until it found a non-(un)dependendent string and then it could return both that string and an offset into that string's char array.  This would ensure that JSDependentString::new_ never created dependent chains.  baseOffset() is rather subtle in meaning and only used in JSDependentString::new_ so maybe we could kill baseOffset() altogether just put the logic in JSDependentString::new_ where it would make more sense in context.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #10)
> I think an alternate fix
> would be to have baseOffset() iterate through (un)dependent string base()s
> until it found a non-(un)dependendent string and then it could return both
> that string and an offset into that string's char array.  This would ensure
> that JSDependentString::new_ never created dependent chains.  baseOffset()
> is rather subtle in meaning and only used in JSDependentString::new_ so
> maybe we could kill baseOffset() altogether just put the logic in
> JSDependentString::new_ where it would make more sense in context.

Yeah, I just wanted to go with the most minimal fix for now. Long term I really want to kill off flat strings (bug 1330776), it will simplify things tremendously because we can remove the whole JSUndependedString concept and a lot of other complexity. Almost no consumers actually *need* null-terminated strings, and the few odd places that do can probably be fixed or they can copy.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8879564 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Probably not very easily but hard to say for sure.

> 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?
All.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be easy to backport.

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

Comment 13

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want branch patches made and nominated.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

It doesn't sound like we need to take this in a dot release for 54, marking wontfix.

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/283a49e31a9d140db349ff891416005d701b16b0

Comment 16

[Jan de Mooij [:jandem]]

Comment on attachment 8879564 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Very old bug.
[User impact if declined]: Crashes or security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: Small/local patch. This undepended string case is not very common.
[String changes made/needed]: None.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/283a49e31a9d

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9071d8e96fc8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/11c8e23f0fd7