Closed Bug 1368105 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr45 --- wontfix
firefox-esr52 55+ fixed
firefox54 + wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(6 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main55+][adv-esr52.3+][post-critsmash-triage])

Attachments

(2 files)

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.
Jim, can you take a look?
Flags: needinfo?(jimb)
Group: javascript-core-security
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);
Group: javascript-core-security
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.
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.
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.
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jimb)
Flags: needinfo?(jdemooij)
Attachment #8879564 - Flags: review?(luke)
Attached patch Add testSplinter Review
Attachment #8879566 - Flags: review?(luke)
Track 54+/55+/56+ as sec-critical.
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.
Attachment #8879564 - Flags: review?(luke) → review+
Attachment #8879566 - Flags: review?(luke) → review+
(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 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.
Attachment #8879564 - Flags: sec-approval?
sec-approval+ for trunk. We'll want branch patches made and nominated.
Attachment #8879564 - Flags: sec-approval? → sec-approval+
It doesn't sound like we need to take this in a dot release for 54, marking wontfix.
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.
Attachment #8879564 - Flags: approval-mozilla-esr52?
Attachment #8879564 - Flags: approval-mozilla-beta?
Attachment #8879564 - Flags: approval-mozilla-esr52?
Attachment #8879564 - Flags: approval-mozilla-esr52+
Attachment #8879564 - Flags: approval-mozilla-beta?
Attachment #8879564 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/283a49e31a9d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main55+][adv-esr52.3+]
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][adv-main55+][adv-esr52.3+] → [jsbugmon:update,bisect][adv-main55+][adv-esr52.3+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.