Closed Bug 874974 Opened 7 years ago Closed 6 years ago

Crash [@ setStringThis] with controllable invalid read

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 + fixed
firefox24 + verified
firefox-esr17 --- unaffected
b2g18 --- wontfix

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17,ignore][adv-main23+])

Crash Data

Attachments

(3 files)

The following testcase crashes on mozilla-central revision c21ef3664c67 (no options required):


var gTestcases = new Array();
var x = '';
function TestCase(n, d, e, a) {
  this.bugnumber = x;
  gTestcases[gTc++] = this;
  TestCase.prototype.dump = function () {
    this.bugnumber.toString();
  }
};
function reportCompare () {
  var testcase = new TestCase();
}
for ( gTc=0; gTc < gTestcases.length; gTc++ ) {}
function jsTestDriverEnd() {
  for (var i = 0; i < gTestcases.length; i++)
    gTestcases[i].dump();
}
for (var i = 0x0020; i < 0x007e; i++ ) {
  new TestCase();
  new TestCase();
  new TestCase();
}
gc();
jsTestDriverEnd();
jsTestDriverEnd();
jsTestDriverEnd();
var x = 0xbeef; // Yummy!
jsTestDriverEnd();
reportCompare();
jsTestDriverEnd();
I took the liberty to enhance this test with a little beef...



Program received signal SIGSEGV, Segmentation fault.
setStringThis (str=<error reading variable: Cannot access memory at address 0xbeef>, this=(js::StringObject * const) 0x7ffff653b7c0 [object String]) at js/src/vm/StringObject.h:51
51              setFixedSlot(LENGTH_SLOT, Int32Value(int32_t(str->length())));
#0  setStringThis (str=<error reading variable: Cannot access memory at address 0xbeef>, this=(js::StringObject * const) 0x7ffff653b7c0 [object String]) at js/src/vm/StringObject.h:51
#1  js::StringObject::init (this=<optimized out>, cx=<optimized out>, str=...) at js/src/vm/StringObject-inl.h:36
#2  0x000000000054d32f in create (newKind=js::GenericObject, str=..., cx=<optimized out>) at js/src/vm/StringObject-inl.h:48
#3  PrimitiveToObject (cx=<optimized out>, v=...) at js/src/jsobj.cpp:4848
#4  0x000000000054d44d in js::ToObjectSlow (cx=<optimized out>, val=..., reportScanStack=<optimized out>) at js/src/jsobj.cpp:4903
#5  0x000000000051118b in ToObjectFromStack (vp=$jsval(<error reading variable: Cannot access memory at address 0xbeef>), cx=0x1851730) at ../jsobj.h:1440
#6  js::GetProperty (cx=0x1851730, v=$jsval(<error reading variable: Cannot access memory at address 0xbeef>), name="toString", vp=...) at js/src/jsinterp.cpp:3284
#7  0x00007ffff67c2d5c in ?? ()
r12     0xbeef  48879
rip     0x53b037 <js::StringObject::init(JSContext*, JS::Handle<JSString*>)+263>
=> 0x53b037 <js::StringObject::init(JSContext*, JS::Handle<JSString*>)+263>:    mov    (%r12),%rax


This doesn't look good at all. Marking sec-critical unless shown to be less severe.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   131119:0f91553baf96
user:        Jan de Mooij
date:        Tue May 07 16:31:25 2013 +0200
summary:     Bug 866050 - Fix UsesBeforeIonRecompile. r=bhackett

This iteration took 327.746 seconds to run.
Needinfo from Jan based on comment 3 :)
Flags: needinfo?(jdemooij)
autoBisect is wrong, the underlying bug is older.

What's happening is that in IonBuilder::getPropTryDefiniteSlot we call GetDefiniteSlot => propertyTypes->isOwnProperty => CheckNewScriptProperties and this adds an extra type (int32) to the property's heap TypeSet.

When we return to IonBuilder::getPropTryDefiniteSlot, it still thinks the GETPROP's type is definitely string and the code it generates crashes because it treats an int32 value as string pointer.

Brian, how is this supposed to work? Do we have a way to guard against this somehow?
Flags: needinfo?(jdemooij) → needinfo?(bhackett1024)
Marking flags under the assumption that this is a regression from the IonMonkey landing.  Adjust as appropriate.
(In reply to Jan de Mooij [:jandem] from comment #5)
> autoBisect is wrong, the underlying bug is older.
> 
> What's happening is that in IonBuilder::getPropTryDefiniteSlot we call
> GetDefiniteSlot => propertyTypes->isOwnProperty => CheckNewScriptProperties
> and this adds an extra type (int32) to the property's heap TypeSet.
> 
> When we return to IonBuilder::getPropTryDefiniteSlot, it still thinks the
> GETPROP's type is definitely string and the code it generates crashes
> because it treats an int32 value as string pointer.
> 
> Brian, how is this supposed to work? Do we have a way to guard against this
> somehow?

The freeze constraint added under PropertyReadNeedsTypeBarrier is supposed to ensure that the compilation is invalidated immediately after it finishes.  It doesn't look like that is happening, and from ~AutoEnterCompilation it doesn't look like we are ever able to invalidate the currently active compilation.  Nicolas, thoughts?
Flags: needinfo?(bhackett1024) → needinfo?(nicolas.b.pierron)
Assigning this to jandem, as it looks like he's investigating.
Assignee: general → jdemooij
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision cc35f8929768).
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision cc35f8929768).
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0f91553baf96
user:        Jan de Mooij
date:        Tue May 07 16:31:25 2013 +0200
summary:     Bug 866050 - Fix UsesBeforeIonRecompile. r=bhackett

This iteration took 0.918 seconds to run.
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore,bisectfix]
Whiteboard: [jsbugmon:update,ignore,bisectfix] → [jsbugmon:bisectfix]
Whiteboard: [jsbugmon:bisectfix] → [jsbugmon:]
JSBugMon: Fix Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/1684c32be328
user:        Kannan Vijayan
date:        Tue Jun 11 15:49:51 2013 -0400
summary:     Bug 859609 - Inline functions that use the scope chain, and also inline call sites with monomorphic cloned lambdas. r=h4writer

This iteration took 317.084 seconds to run.
djvj, is bug 859609 a possible fix?
Flags: needinfo?(nicolas.b.pierron) → needinfo?(kvijayan)
Nope.  This patch is unrelated, and simply masks the issue in this bug.
Flags: needinfo?(kvijayan)
Reverting needinfo to nbp as per comment 7.
Flags: needinfo?(nicolas.b.pierron)
Kannan, do you know how to transform the testcase into one that still shows the bug? (Just in case fuzzing doesn't turn one up easily.)
Whiteboard: [jsbugmon:] → [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17]
Whiteboard: [jsbugmon:verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17] → [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17]
Attached patch PatchSplinter Review
There are two functions used by TI code to invalidate scripts:

(1) TypeCompartment::addPendingRecompile(JSContext *cx, const RecompileInfo &info)
(2) AddPendingRecompile(JSContext *cx, JSScript *script)

The second one correctly handles invalidation during compilation by checking if there's a compiler running and if so it calls CompilerOutput::invalidate(). Ion's CodeGenerator::link checks this and aborts compilation if that's set.

However, the first method is basically a no-op during compilation, because AutoEnterCompilation sets pendingRecompilation to |true| during compilation and addPendingRecompile returns immediately in that case.

Brian, do you have any idea why we're only seeing this now? I think it should also affect aurora, beta and release.
Attachment #762008 - Flags: review?(bhackett1024)
And btw, we're running code based on outdated type information so this is definitely sec-critical.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 762008 [details] [diff] [review]
Patch

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

This is definitely an older bug, though I don't know why it would start showing up now.  Maybe 804676 made this easier to expose?
Attachment #762008 - Flags: review?(bhackett1024) → review+
Comment on attachment 762008 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? It's possible for somebody who knows what he/she is doing, but 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 tests. There are some short comments but you could get the same info from the code with little more effort.

Which older supported branches are affected by this flaw? Release, beta, aurora. This is the first reported (fuzz) bug that triggers it though so for some reason it may be hard/impossible to trigger on older branches.

If not all supported branches, which bug introduced the flaw? -

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Patch won't apply to branches, but we need to backport only a small part of it, should be easy.

How likely is this patch to cause regressions; how much testing does it need? Unlikely - usual fuzz testing + a few nightlies should be enough.
Attachment #762008 - Flags: sec-approval?
I'm unclear of how far back we need to take this but I'll give sec-approval+ for trunk to get it onto 24 before we branch. If 23 is the only affected version (effectively), we should get a patch for it prepared.
Attachment #762008 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6f5699da1fd

I will also prepare a patch for 23.
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f6f5699da1fd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17] → [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-aurora  (reproduced on revision 6b40d5226c13).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-beta (tried revision 9f0d4fe5db55).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-release (tried revision 88ff4d1b205f).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-esr17 (tried revision 468f2568342c).
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Attached patch Patch for auroraSplinter Review
Minimal fix for aurora. This fixes the crash for me.
Attachment #764086 - Flags: review?(bhackett1024)
Attachment #764086 - Flags: review?(bhackett1024) → review+
Comment on attachment 764086 [details] [diff] [review]
Patch for aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Unknown, we think bug 804676 may have exposed an older bug.
User impact if declined: Security bugs, crashes.
Testing completed (on m-c, etc.): A similar patch landed on m-c.
Risk to taking this patch (and alternatives if risky): Low.
String or IDL/UUID changes made by this patch: None.
Attachment #764086 - Flags: approval-mozilla-aurora?
Attachment #764086 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking status-firefox24:verified based on comment 24.
Whiteboard: [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17,ignore] → [jsbugmon:update,verify-branch=mozilla-aurora;mozilla-beta;mozilla-release;mozilla-esr17,ignore][adv-main23+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.