Closed Bug 845899 Opened 7 years ago Closed 6 years ago

typed array uninline/transfer operations leave stale pointers in IM/JM jit code

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: luke, Assigned: sfink)

References

Details

(Keywords: csectype-uaf, sec-high)

When we bake in pointers to the typed array's dataElements() in jit code (mjit::Compiler::jsop_(set|get)elem_typed and IonBuilder::getTypedArrayElements) we don't have any guard in place to invalidate this jit code if the dataElements() pointer changes.

The fix is pretty simple: we need a types::HeapTypeSet::WatchObjectStateChange in the above compiler functions and types::MarkObjectStateChange in ArrayBufferObject::changeContents/stealConents.

The lame part is that uninline data is given a cx when called via the friendapi functions.  A quick scan shows that there is a trivial cx on hand for most callers, but not all.

It'd be great if we could add a transfer() function to the shell so that we could test this.
I forgot to ask: is that right Brian or is there a more natural way to do this within TI?
Bug 861925 is going to add more code that'll need frobbing for this.  I think it'll just fall out of auditing callers and such, but it seems best to be safe and note it explicitly here, as I'm understandably not going to comment on this hole during patch reviews there.  :-)
This has been dead in the water for four months.  Is there somebody who can look at it?  Maybe Steve?
Blocks: 887495
Depends on: 893863
Can you look at this, Steve?  Thanks.
Flags: needinfo?(sphink)
I started on this a little while back. The one thing I ran into is that it'll require changing the way webidl handles typed arrays. All of the Get*Data functions need to take a cx parameter. But I haven't made it very far with this.
Group: javascript-core-security
Flags: needinfo?(sphink)
Flags: needinfo?(sphink)
Assignee: general → sphink
Duplicate of this bug: 887495
I'm pretty sure that, in the meantime, this has been fixed piecemeal; bug 971845 cleaned up the last bits, I think.  For example, neuter() now always gets a 'cx' and thus JIT code is appropriately invalidated by MarkObjectStateChange.  Tentatively resolving WFM.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
(In reply to Luke Wagner [:luke] from comment #7)
> I'm pretty sure that, in the meantime, this has been fixed piecemeal; bug
> 971845 cleaned up the last bits, I think.  For example, neuter() now always
> gets a 'cx' and thus JIT code is appropriately invalidated by
> MarkObjectStateChange.  Tentatively resolving WFM.

That is my belief too.
Flags: needinfo?(sphink)
(In reply to Luke Wagner [:luke] from comment #7)
> I'm pretty sure that, in the meantime, this has been fixed piecemeal; bug
> 971845 cleaned up the last bits, I think.

Do we have tests for this, and for every place where we inline a data pointer, a typed array length, a byteLength, anything along those lines?  Someone's going to break this at some point if we don't.
Flags: needinfo?(sphink)
Flags: in-testsuite?
Group: core-security, javascript-core-security
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> (In reply to Luke Wagner [:luke] from comment #7)
> > I'm pretty sure that, in the meantime, this has been fixed piecemeal; bug
> > 971845 cleaned up the last bits, I think.
> 
> Do we have tests for this, and for every place where we inline a data
> pointer, a typed array length, a byteLength, anything along those lines? 
> Someone's going to break this at some point if we don't.

Bug 991981 and its progeny added tests for (I believe) every aspect of this, including a few varieties that were broken even to the present day.
Flags: needinfo?(sphink)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.