999651 - Invalidation due to ArrayBuffer neutering doesn't work when the ArrayBuffer's data pointer doesn't change

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Jeff Walden [:Waldo]]

The constraint that handles ArrayBuffer neutering invalidations is so:

// Constraint which triggers recompilation when the underlying data pointer for
// a typed array changes.
class ConstraintDataFreezeObjectForTypedArrayBuffer
{
    void *viewData;

  public:
    ConstraintDataFreezeObjectForTypedArrayBuffer(void *viewData)
      : viewData(viewData)
    {}

    bool invalidateOnNewObjectState(TypeObject *object) {
        return object->singleton()->as<TypedArrayObject>().viewData() != viewData;
    }

    // ...
};

It assumes that the only reason invalidation needs to happen, is when the typed array's data pointer changes.  But that's not really complete.  There are cases where the data pointer *doesn't* change when the underlying ArrayBuffer is neutered.  asm.js ArrayBuffers in certain cases (although this is sort of only a temporary restriction, the fix for which just hasn't been implemented yet, I hear) are one instance.  The current behavior of the neuter() shell builtin is another.

Guarding on the data pointer changing is necessary, but not enough.  We also need to guard on the ArrayBuffer's byte length not changing.  Or, perhaps, we need different *sorts* of guards for different situations.  For example, when inlining the length of a typed array, we don't care about the data pointer remaining the same, only about the length remaining the same.  So perhaps we need two different constraints here, one watching just length, one watching length and data pointer both.  Or maybe, depending on use, only the data pointer one is needed, but care has to be taken how that data pointer (once inlined) is subsequently used.  Hmm.

I'm not sure about the security implications of this yet.  It depends on exactly what the ultimate dependency is, that the constraint's guarding against.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Test

Arguably this wants neuter("...") instead of testing these esoterica of when contents are stealable, but a little more testing of real-world cases isn't so bad, either.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Discard generated typed array code when the array's data pointer *or* length changes

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8412875 [details] [diff] [review]
Test

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

WFM, thanks, except you'll need to update to the new neuter().

Comment 4

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8412877 [details] [diff] [review]
Discard generated typed array code when the array's data pointer *or* length changes

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

Looks good from what I can tell.

Comment 5

[Shu-yu Guo [:shu]]

Comment on attachment 8412877 [details] [diff] [review]
Discard generated typed array code when the array's data pointer *or* length changes

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

Looks good to me.

Comment 6

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

How far back does this problem go?

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Refactor typed array elements accesses to be done with a single code path, not one each for get/set

Maybe we *can* just combine up all this element-access stuff into one method.  I'm not super-happy about the interface and all, but this should get the job done.  Test in this bug and in bug 995679 fail without this, both pass with.

Comment 8

[Jeff Walden [:Waldo]]

A long ways, probably since the constraint was added in the first place.  That looks to have happened with bug 840282, whose target is 22.  (And I guess before then we were just bad about burning in these length/pointers everywhere, and had no constraints at all on them changing with neutering. [!] )  So, basically forever.

Comment 9

[Shu-yu Guo [:shu]]

Comment on attachment 8418204 [details] [diff] [review]
Refactor typed array elements accesses to be done with a single code path, not one each for get/set

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

Patch looks good to me except for the one comment below.

My previous comment about a unified path was not for IonBuilder, but for having a single CompilerConstraint that should be used whenever a TypedArray's underlying data (buffer or length) is touched, to minimize cases where we might mess up security when trying to be clever. Sorry if there was confusion.

r=me with getTypedArrayLengthAndData reverted. Let me know if there was a compelling reason you did that refactor though.

::: js/src/jit/IonBuilder.cpp
@@ +7448,5 @@
> +void
> +IonBuilder::getTypedArrayLengthAndData(MDefinition *obj,
> +                                       BoundsChecking checking,
> +                                       MDefinition **index,
> +                                       MInstruction **length, MInstruction **elements)

What's the advantage of adding getTypedArrayLengthAndData over just changing the body of getTypedArrayElements to use |watchStateChangeForTypedArrayData|?

I apologize for not being clear in the earlier review -- I didn't want a unified IonBuilder path for getting length and elements, but a single CompilerConstraint that should be used whenever any TypedArray data is touched, as to avoid encouraging cleverness when security is important.

If the intention was to save on an extra compiler constraint -- I don't think that's worth the weird API split; we don't make any efforts in the compiler right now to make the constraint list a set. So if you getelem on a typed array N times, it will have N copies of the same constraint on the same typeset anyways. If this extra memory becomes a problem, we should solve that problem by pruning the constraint list to not contain dupes, not here anyways.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Delta patch to make getTypedArrayLengthAndData skip computing data if requested

As discussed yesterday, here's an interdiff patch to get rid of the separate getTypedArrayLength implementation, having one method compute length/data for everyone, using null arguments to indicate this.  (Although we actually do this with a helper method that feeds the right things into getTypedArrayLengthAndData.)  Now only one method does any of this, we have one constraint that guards the overall operation of accessing (to get or set) a typed array element, and it's impossible to get data or get length forgetting a constraint.  Final pushed patch will be the combo of the previous patch and this one, of course.

Comment 11

[Shu-yu Guo [:shu]]

Comment on attachment 8420344 [details] [diff] [review]
Delta patch to make getTypedArrayLengthAndData skip computing data if requested

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

::: js/src/jit/IonBuilder.cpp
@@ +7426,5 @@
>  void
>  IonBuilder::getTypedArrayLengthAndData(MDefinition *obj,
>                                         BoundsChecking checking,
>                                         MDefinition **index,
>                                         MInstruction **length, MInstruction **elements)

Just a note to self: at first I disliked how index is MDefinition ** while length and elements are MInstruction **, but this inconsistency is actually preferable because it says that index is both an in and a out param (and the input can be a phi), while length and elements are both solely outparams and cannot be phis.

::: js/src/jit/IonBuilder.h
@@ +543,3 @@
>                                              int32_t elemSize);
>  
>      enum BoundsChecking { DoBoundsCheck, SkipBoundsCheck };

Suggestion for followup: AsmJS.cpp also defines a NeedsBoundsCheck enum. Perhaps we can unify them.

Comment 12

[Shu-yu Guo [:shu]]

Comment on attachment 8420344 [details] [diff] [review]
Delta patch to make getTypedArrayLengthAndData skip computing data if requested

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

::: js/src/jit/IonBuilder.cpp
@@ +7426,5 @@
>  void
>  IonBuilder::getTypedArrayLengthAndData(MDefinition *obj,
>                                         BoundsChecking checking,
>                                         MDefinition **index,
>                                         MInstruction **length, MInstruction **elements)

Oops, one other thing: rename this to addTypedArrayLengthAndData (ditto for getTypedArrayLength -> addTypedArrayLength) since it adds the MIRs instead of allocating and returning (cf. addBoundsCheck, addConvertElementsToDoubles, etc).

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Final trunk patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Tricky to very tricky to impossible.  The affected code has redundant bounds-checks in the data-accessing case, that defend against the lack of check here right now.  The length-accessing case is more problematic in that someone might depend on its correctly changing upon neutering, to eliminate a bounds-check or so.  I'm not aware that we have anything like this right now.

> Do comments in the patch, the check-in comment, or tests included
> in the patch paint a bulls-eye on the security problem?
Patch contents clearly implicate something fishy, but as code has redundant bounds-checks right now, nothing memory-unsafe is squarely implicated.  I'll censor the checkin comment on landing, as usual.  The tests clearly point out the actual correctness issues, but I'm not landing them immediately -- rather, after a cycle or two for percolation time.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
The buggy code dates to bug 771383 mid-2012, so safe to say everything, looks like.

> Do you have backports for the affected branches? If not, how
> different, hard to create, and risky will they be?
Haven't made 'em yet.  I suspect it won't backport absolutely trivially, but the porting process shouldn't be too onerous.

> How likely is this patch to cause regressions; how much
> testing does it need?
I don't think it's *likely*, but the slight refactoring here to fix both this bug and bug 995679 make it slightly trickier than just adding a missed constraint.  If landing weren't disclosure of a sort, I'd want to get more testing.  But since it is, I'd probably rather err on the side of caution and land this as near-simultaneous with all the other typed array work as possible, across branches.

Comment 14

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

Comment on attachment 8423453 [details] [diff] [review]
Final trunk patch

Sec-approval+ for trunk. We'll want this on branches once it is in.

Comment 15

[Jeff Walden [:Waldo]]

Obfuscatory rollup patch of this bug, and the half-dozen others discovered along the way:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c631967ab9e

Backport work still ongoing.  Will try to have aurora/beta patches posted by EOD.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for various B2G test issues.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7be685b59845

https://tbpl.mozilla.org/php/getParsedLog.php?id=40468378&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40468742&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40468248&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40468765&tree=Mozilla-Inbound

Comment 17

[Jeff Walden [:Waldo]]

Relanded with CLOBBER:

https://hg.mozilla.org/integration/mozilla-inbound/rev/57b0932e2f06

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Aurora backport

Again, no help wanted landing this on branches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): to the introduction of neutering, methinks
User impact if declined: incorrect behaviors in scripts; it *looks* like there might not be true security issues, but this is finicky enough I'd rather not trust that judgment
Testing completed (on m-c, etc.): landed on m-c, ran test here against this and things are okay
Risk to taking this patch (and alternatives if risky): something got screwed up horribly in the backport -- not likely, as this code's only changed cosmetically over its lifetime
String or IDL/UUID changes made by this patch: N/A

Comment 19

[Jeff Walden [:Waldo]]

Comment on attachment 8429706 [details] [diff] [review]
Aurora backport

Looks like this is pretty much the beta-targeted patch as well as the aurora-targeted one, comparing qdiff.

As before, please leave the pushing of this patch to me.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Al - Can you expand on your comment 14. Can you confirm that this bug warrants inclusion in beta and esr?

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8429706 [details] [diff] [review]
Aurora backport

Waldo - is this patch also applicable to ESR branch?  If yes, please nom otherwise can you put up the ESR patch before end of the week?

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Lawrence Mandel [:lmandel] from comment #21)
> Al - Can you expand on your comment 14. Can you confirm that this bug
> warrants inclusion in beta and esr?

As this is a sec-high bug that is landed already to trunk, this should land as high up as we can take it.

Comment 24

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c5a58d90da90
https://hg.mozilla.org/releases/mozilla-beta/rev/4fd82c282519

b2g/esr24 backports shortly.

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 8429706 [details] [diff] [review]
Aurora backport

This applies in essence to b2g28, if you squint a little at various cosmetic changes to the relevant code.

Approval discussion in comment 18 applies to this request, except that this is landed on aurora/beta now since then.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fd82c282519

Comment 27

[Jeff Walden [:Waldo]]

Attachment: b2g26: just remove the optimization

The object type constraint mechanism for handling typed array data changes was added between 26 and 28.  I looked at backporting it, and the patch that added the constraint that prior patches here have adjusted, depends on yet another (large) patch that landed after 26.

So give up.  Remove the optimizations for known-constant typed arrays that burn in a length or data pointer.  An extra load or two is far from the end of the world here.

Comment 18's approval request info applies here.

Again, I don't want help landing this.

Comment 28

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/57cd741e4d0b
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/4c359ff76654

Comment 29

[Jeff Walden [:Waldo]]

Comment on attachment 8431268 [details] [diff] [review]
b2g26: just remove the optimization

The ESR24 backport is substantially the same as this, although it doesn't directly apply.

Comment 18 discussion applies as far as the approval request goes, plus we've landed patches for this everywhere at this point *except* esr24.

Comment 30

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8431268 [details] [diff] [review]
b2g26: just remove the optimization

ESR24 approval granted.

Comment 31

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/8c406adf76a0

Comment 32

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification, no test case, feel free to ping me should one arise. Thanks.

Comment 33

[Justin Wood (:Callek)]

trying to apply this to SeaMonkey 2.26.1 (Gecko 29) resulted in patch conflicts, and due to the nature of this patchset it seems like I won't be able to take it.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ffc95fd87d3