Lingering issues after bug 982974

RESOLVED FIXED in Firefox 30

Status

()

P1
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

({sec-critical})

unspecified
mozilla32
sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ fixed, firefox31+ fixed, firefox32+ fixed, firefox-esr2430+ fixed, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 wontfix)

Details

(Whiteboard: [reviewers: read comment 133][qa-][adv-main30+][adv-esr24.6+])

Attachments

(75 attachments, 7 obsolete attachments)

24.95 KB, patch
Details | Diff | Splinter Review
2.37 KB, patch
sfink
: review+
Details | Diff | Splinter Review
3.74 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.18 KB, patch
sfink
: review+
Details | Diff | Splinter Review
984 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
2.37 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.28 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.80 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.68 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.64 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.22 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.52 KB, patch
jdm
: review+
Details | Diff | Splinter Review
14.13 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.21 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
1.27 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.98 KB, patch
luke
: review+
Details | Diff | Splinter Review
1.36 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
2.80 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.99 KB, patch
luke
: review+
Details | Diff | Splinter Review
2.94 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.22 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
24.62 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.71 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.41 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
2.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
972 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.25 KB, patch
smaug
: review+
Details | Diff | Splinter Review
2.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.99 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
27.78 KB, patch
Details | Diff | Splinter Review
32.10 KB, patch
Details | Diff | Splinter Review
129.05 KB, patch
Details | Diff | Splinter Review
9.39 KB, patch
sfink
: review+
Details | Diff | Splinter Review
1.79 KB, patch
terrence
: review+
Details | Diff | Splinter Review
4.36 KB, patch
luke
: review+
Details | Diff | Splinter Review
16.18 KB, patch
sfink
: review+
Details | Diff | Splinter Review
14.33 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.23 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.32 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
22.47 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.16 KB, patch
sfink
: review+
Details | Diff | Splinter Review
968 bytes, patch
sfink
: review+
Details | Diff | Splinter Review
43.04 KB, patch
sfink
: review+
Details | Diff | Splinter Review
4.73 KB, patch
sfink
: review+
jorendorff
: review+
Details | Diff | Splinter Review
3.91 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
11.44 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.31 KB, patch
shu
: review+
nmatsakis
: review+
Details | Diff | Splinter Review
1.28 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
992 bytes, patch
nmatsakis
: review+
Details | Diff | Splinter Review
1.56 KB, patch
nmatsakis
: review+
Details | Diff | Splinter Review
877 bytes, patch
nmatsakis
: review+
Details | Diff | Splinter Review
21.85 KB, patch
nmatsakis
: review+
Waldo
: review+
Details | Diff | Splinter Review
1.39 KB, patch
bobbyholley
: review+
Details | Diff | Splinter Review
1.23 KB, patch
sfink
: review+
Details | Diff | Splinter Review
2.92 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
960 bytes, patch
sicking
: review+
Details | Diff | Splinter Review
965 bytes, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
4.84 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
4.59 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
744 bytes, patch
luke
: review+
Details | Diff | Splinter Review
4.74 KB, patch
sfink
: review+
Details | Diff | Splinter Review
820 bytes, patch
Ehsan
: review+
Details | Diff | Splinter Review
1.55 KB, patch
Ehsan
: review+
Details | Diff | Splinter Review
298.37 KB, patch
decoder
: feedback+
Details | Diff | Splinter Review
308.02 KB, patch
decoder
: feedback+
Details | Diff | Splinter Review
2.68 KB, text/plain
Details
1011 bytes, patch
Details | Diff | Splinter Review
95.16 KB, patch
Waldo
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
93.09 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
90.44 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
5.10 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
1.50 KB, patch
sfink
: review+
Details | Diff | Splinter Review
75.55 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
876 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
To be elaborated after I'm sure this bug's been filed with the right security bits set to keep it hidden, given we now have multiple security-sensitive checkboxen to consider.
(Assignee)

Comment 1

5 years ago
Long story short: the hackaround belt-and-suspenders fix for bug 982974 fixed some of the issues there, but it didn't fix all of them.  At the absolute minimum this includes the DataView::write issue I pointed out in comment 38.  (And that was noted in comment 0, even.  Fail.)  It may possibly extend beyond that.  Not sure how I missed this, maybe in-the-moment tunnel vision after having found a "complete" hackaround, despite efforts to be sure I was covering all the bases.

Given the one case is wrong, I suspect the full fix here might end up being doing that full audit (the one I thought was too risky to rely on, there), and cherrypicking all the necessary mini-fixes and backporting them everywhere.  Joy.  Lots of smallish patches coming up (I only have the DataView::write issue solved locally, still working through the rest), for all the issues noted in bug 982974 comment 33 and bug 982974 comment 38.  And someone else should probably do the audit as well, to double-check my work.
Keywords: sec-critical
(Assignee)

Comment 2

5 years ago
This fixes -- I think -- maybe all the things I reported in the couple comments in the original bug.  At least, in terms of making the relevant methods throw, or do "stuff" to be sane-ish.  The specs on all this are buggy, so there are no "correct" semantics to implement right now.  (And we can't reasonably raise the issue until things are fully fixed here, of course.)

I still plan to do more auditing -- probably run through the same steps I did last time as a double-check, maybe change the JSAPI methods for exposing this data to "something else" that might be more amenable to correct use.  As has already been discussed, tho, it's unclear what such an API would look like, or how easily it could be lightly integrated (without effect on GC or other more-intricate things) into the existing code without doing conceptual or runtime violence to it.  So this is still hazy right now.
Depends on: 994286
Comment on attachment 8403687 [details] [diff] [review]
Rollup patch of current patching efforts (including tests for each change made)

This didn't seem to show up any new issues. \o/
Attachment #8403687 - Flags: feedback+
(Assignee)

Comment 4

5 years ago
I lied when I said in bug 982974 comment 33 that I thought the JSAPI typed array methods were being used safely.

dom/bindings/TypedArray.h has a mozilla::dom::TypedArray struct that takes in an object, extracts data and length into fields using JS_GetObjectAsUint32Array or similar.  (I think I maybe didn't manage to find JS_GetObjectAs* when looking at JSAPI methods of concern.  That, or I trailed off when the magnitude of the problem became clear, and didn't bother investigating *all* the JSAPI methods.)

This TypedArray struct is used by WebIDL codegen to handle typed array arguments.  Any WebIDL method that takes as arguments a typed array, then a primitive argument, does all the same things all the spot-fixed places in attachment 8403687 [details] [diff] [review] did.  For example:

dom/webidl/AudioParam.webidl
33:    void setValueCurveAtTime(Float32Array values, double startTime, double duration);

You can pass in a Float32Array as first argument, an object as second argument that neuters the array, then you lose.  WebIDL will process all the arguments, then the underlying implementation method will use the cached length/data pointer on you.

I guess maybe some sort of actual object that inserts itself into view, or is *somehow* notified when array buffer data goes away, is a requirement here.  And then that structure's fields can be updated during the neutering operation.  We can't spot-fix all the places in generated code that have issues here, obviously.

Although, even with that, implementations of WebIDL'd methods will still have to be careful about not caching values, if they do anything that can potentially reenter.  Blah.
Ugh.  I think those structs predate the neutering.

So here's the thing: I'm totally happy to nuke the cached members in the dom::TypedArray structs if I can have _fast_ inline getters for getting that information lazily.  Right now we're trying hard to minimize the number of non-inline JSAPI calls we're making here, because they absolutely kill performance of passing these arguments.

And yes, any actual WebIDL method implementation will have to be careful.  :(  That's really annoying, and is a security bug just waiting to happen...
Priority: -- → P1
(Assignee)

Comment 6

5 years ago
Posted patch ArrayBufferInputStream test (obsolete) — Splinter Review
bzexport fail, switching to manual mode, disregard other mail you may or may not have gotten...

I could imagine setting an ABIS's data should steal the contents of the ArrayBuffer, or neutering should implicitly close the stream, or should cut off data however far it's gotten so far.  Not sure what's right, probably sure almost no one cares.

This patch fails without the subsequent implicit-closure patch, passes with.
Attachment #8409923 - Flags: review?(sphink)
(Assignee)

Updated

5 years ago
Attachment #8409923 - Flags: review?(josh)
(Assignee)

Comment 7

5 years ago
Attachment #8409928 - Flags: review?(sphink)
Attachment #8409928 - Flags: review?(josh)
(Assignee)

Comment 9

5 years ago
Attachment #8409933 - Flags: review?(sphink)
(Assignee)

Comment 10

5 years ago
Attachment #8409934 - Flags: review?(sphink)
(Assignee)

Comment 12

5 years ago
Attachment #8409936 - Flags: review?(sphink)
(Assignee)

Comment 13

5 years ago
Attachment #8409937 - Flags: review?(sphink)
(Assignee)

Comment 14

5 years ago
Attachment #8409938 - Flags: review?(sphink)
(Assignee)

Comment 15

5 years ago
Attachment #8409939 - Flags: review?(sphink)
(Assignee)

Comment 16

5 years ago
Attachment #8409940 - Flags: review?(sphink)
(Assignee)

Comment 17

5 years ago
Attachment #8409941 - Flags: review?(sphink)
(Assignee)

Comment 18

5 years ago
Posted patch Possible WebIDL fix, ish (obsolete) — Splinter Review
Setting aside worries about making more out-of-line calls and all, this is maybe a minimal fix.  Do we want to push harder, and perhaps change the entire TypedArray API so that there's an explicit cut point where the data/length are extracted?  We'd have to touch every TypedArray use: not necessarily a bad thing, to verify none of the places accidentally reenter in an unfortunate location.  (Is it at all possible specs will have considered neutering and avoided the related pitfalls, or no?)  Thoughts?

I haven't actually gone and written a testcase/verified this works in a debugger yet, just putting it out for feedback right now.
Attachment #8409980 - Flags: feedback?(bzbarsky)
Comment on attachment 8409980 [details] [diff] [review]
Possible WebIDL fix, ish

The worry about calls is a maybe-big deal, sadly.  How do the numbers on http://hg.mozilla.org/users/bjacob_mozilla.com/webgl-perf-tests/raw-file/3729e8afac99/uniform-int-taking-typed-array.html look with/without this patch?

Past that, this patch should mostly deal with issues due to WebIDL itself reentering script, I agree.  The one exception is that if we pass a chrome-to-content cross-compartment wrapper for a typed array the old code used to store the actual typed array in mObj while the new code stores the cross-compartment wrapper until ComputeData() is called.  So if something triggers hueyfix in between those two points, the UnboxArray call in ComputeData will in fact return null.  Actually, same situation can be produced by just changing document.domain to make two pages that used to be same-origin not be same-origin anymore so the CheckedUnwrap in JS_GetObjectAs* fails.

> Is it at all possible specs will have considered neutering and
> avoided the related pitfalls

Possibly, but pretty unlikely....  The good news is that most specs using typed array arguments are likely not calling back out to JS in practice.
Attachment #8409980 - Flags: feedback?(bzbarsky) → feedback+
(Assignee)

Comment 20

5 years ago
Perf looks like ~16.3ms before, ~17.5ms after.  Not a catastrophic change, seems to me, but certainly a change.  I kind of expected given your comments the inlining would be needed; this at least makes the variance clearer.

hueyfix implications here are rather sad.  We could do the unwrapping in the type-checking component of the change, I guess.  But, honestly, I'm pretty confused about what compartment mObj is "supposed" to be in here.  The passed-in object is of course current-compartment.  The unwrapping might select a different one.  It's really not clear to me how that doesn't cause compartment mismatches when .Obj() gets passed places.  Any chance you could enlighten?  (Maybe on IRC at greater length than a single comment here, perhaps.)

Oh, and I haven't tryservered that patch to see whether/if it works, because tryservering exposes the patch/issue publicly, being paranoid.  And I haven't found some other patch to squirrel it into in a push to at least obscure that.  I do know content/media/webaudio tests passed, and content/canvas tests ran pretty well up until hitting bug 905041 at some point, so it smoketests to at least some degree.
Attachment #8409932 - Flags: review?(sphink) → review+
Comment on attachment 8409933 [details] [diff] [review]
DataView set* fixing

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

Much nicer.
Attachment #8409933 - Flags: review?(sphink) → review+
Attachment #8409934 - Flags: review?(sphink) → review+
Attachment #8409935 - Flags: review?(sphink) → review+
Comment on attachment 8409936 [details] [diff] [review]
Test for ArrayBuffer.prototype.slice

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

Ooh, tests/ecma_6. I should be putting stuff there. I hate using js1_8_5/extensions.
Attachment #8409936 - Flags: review?(sphink) → review+
(Assignee)

Comment 23

5 years ago
Hmm, actually, most of these probably belong in an extensions/ directory (even if ecma_6/extensions), because neuter() or serialize().  Blah.  And, um, have I been tagging these tests as shell-only?  Probably not, I'll have to check them all in that regard.  Blah**2.
Comment on attachment 8409937 [details] [diff] [review]
Fix ArrayBuffer.prototype.slice

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

We do all the right checks, we just compile them out in opt builds...
Attachment #8409937 - Flags: review?(sphink) → review+
Comment on attachment 8409938 [details] [diff] [review]
Test for %TypedArray%.move

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

Not sure which will land first, but after bug 999755, all of these tests should use both neuter() variants.
Attachment #8409938 - Flags: review?(sphink) → review+
Comment on attachment 8409940 [details] [diff] [review]
%TypedArray%.subarray test

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

These tests would read better with assertThrows(() => ta.subarray(...), RangeError), but whatever.

Also, I think I have some of these tests hanging off of bug 982974, but yours are more complete anyway.
Attachment #8409940 - Flags: review?(sphink) → review+
Comment on attachment 8409939 [details] [diff] [review]
Fix %TypedArray%.move

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

::: js/src/vm/TypedArrayObject.cpp
@@ +573,5 @@
>          uint32_t nelts = srcEnd - srcBegin;
>  
> +        MOZ_ASSERT(dest <= INT32_MAX, "size limited to 2**31");
> +        MOZ_ASSERT(nelts <= INT32_MAX, "size limited to 2**31");
> +        if (dest + nelts > lengthDuringMove) {

So this is safe, but a little weird. You could end up with originalLength=20, srcBegin=20, srcEnd=20, dest=0, lengthDuringMove=0. dest + nelts <= lengthDuringMove, so no error, and you end up copying zero bytes, so everything is happy.

But if we were later to lose our minds and implement shrinking, so that lengthDuringMove=10, this would access invalid memory. This is only range-checking the destination, not the source. I think I'd feel a little more comfortable if it also checked the source, though that's a user-visible behavior difference and I don't know if the spec would ever allow it. If not, then never mind.

Specifically, I'm thinking of ((dest + nelts > lengthDuringMove) || (srcEnd > lengthDuringMove)).
Attachment #8409939 - Flags: review?(sphink) → review+
Attachment #8409941 - Flags: review?(sphink) → review+
Comment on attachment 8409928 [details] [diff] [review]
Fix nsArrayBufferInputStream handling for neutered buffers

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

Looks good from the jsapi perspective. I'll let jdm worry about .ref().get().toObject() (it'd better not be nullptr!) and the semantics of this stuff.
Attachment #8409928 - Flags: review?(sphink) → review+
Comment on attachment 8409923 [details] [diff] [review]
ArrayBufferInputStream test

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

Superficially makes sense. I will rely on jdm knowing what's actually going on with these stream things.

Side question: why the double GC?

::: netwerk/test/mochitests/test_arraybufferinputstream.html
@@ +10,5 @@
> +<script type="text/javascript">
> +function neuter(ab)
> +{
> +  var w = new Worker("data:application/javascript,");
> +  w.postMessage(ab, [ab]);

Wow, that's nice and short. Nice trick with the worker.
Attachment #8409923 - Flags: review?(sphink) → review+
(Assignee)

Comment 30

5 years ago
Posted patch With null deref fixed (obsolete) — Splinter Review
(In reply to Steve Fink [:sfink] from comment #28)
> I'll let jdm worry about .ref().get().toObject() (it'd better not be nullptr!)

Ugh, right.  I think this is a null deref if no ArrayBuffer has been set yet.  Fixed here, with semi-consistent/plausible semantics for that case.
Attachment #8410647 - Flags: review?(josh)
Attachment #8409923 - Flags: review?(josh) → review+
Attachment #8410647 - Flags: review?(josh) → review+
(Assignee)

Comment 31

5 years ago
Actually, let's update the test.  And then let's *actually* make a real fix in the code.  :-(
Attachment #8409923 - Attachment is obsolete: true
Attachment #8410671 - Flags: review?(josh)
(Assignee)

Comment 32

5 years ago
Attachment #8409928 - Attachment is obsolete: true
Attachment #8409928 - Flags: review?(josh)
Attachment #8410672 - Flags: review?(josh)
(Assignee)

Comment 33

5 years ago
(In reply to Steve Fink [:sfink] from comment #27)
> Specifically, I'm thinking of ((dest + nelts > lengthDuringMove) || (srcEnd
> > lengthDuringMove)).

I like this.  Change made.  (Who knows what specs will do, we're just making it up as we go right now, la la la....)
> Perf looks like ~16.3ms before, ~17.5ms after. 

I can live with that.  Spot-check the other typed array tests in that directory, just to be sure?

> But, honestly, I'm pretty confused about what compartment mObj is "supposed" to be in
> here.

Undefined, sadly.

The main reason mObj is there at all is for consumers who want to hold on to the object and return it later.  And for those consumers it doesn't matter too much what compartment they store it in, since when they return it it will get wrapped into the compartment it's being returned to.

I say "main" because in practice some of the WebGL code does operate on mObj because it was written pre-webidl bindings.  When it does, it expects mObj to be the right sort of typed array object, possibly inside a transparent cross-compartment wrapper, but does not expect it to be in any particular compartment: it just calls JS_GetArrayBufferViewType/JS_GetTypedArrayByteLength on the object, which don't have any compartment asserts (and no JSContext argument, even).
(Assignee)

Comment 35

5 years ago
Okay, I think this preserves the previous semantic meaning of mObj while deferring length/data computation til all arguments have been processed.  Stuff is generally inlined, except when the inlined method is rather crazy.

I haven't audited users at all.  I would be somewhat inclined to do that by taking this patch, then doing a separate patch atop it to change the API to require an explicit ComputeLengthAndData call on each typed-array-like argument before Data()/Length() can be called without asserting.  Seem reasonable?

In the meantime it would be nice to checkpoint this as itself ready to go, if not necessarily a final state.  This macro-pasting garbage is fugly, and I don't want to have to think about it again.  :-)
Attachment #8409980 - Attachment is obsolete: true
Attachment #8411393 - Flags: review?(bzbarsky)
(Assignee)

Comment 36

5 years ago
Oh, the numbers still vary on this new WebIDL patch, but they seem much more in the ballpark of pre-patch performance numbers.  There's probably some loss for the ArrayBufferView and ArrayBuffer cases, which I hope we can just eat as there's so much variation in the layouts for those sorts of objects.
status-firefox31: --- → affected
tracking-firefox31: --- → +
Attachment #8410672 - Flags: review?(josh) → review+
Attachment #8410671 - Flags: review?(josh) → review+
(Assignee)

Comment 37

5 years ago
Another fun testcase, which Luke sez is benign-ish, but still represents a real issue along the lines of all the other stuff here:

  function f(stdlib, foreign, heap)
  {
    "use asm";
    var h = new stdlib.Uint8Array(heap);
    function setElement(value)
    {
      value = value|0;
      h[5] = value;
    }
    return setElement;
  }
  var buf = new ArrayBuffer(4096);
  var setelem = f(this, {}, buf);
  setelem({ valueOf: function() { neuter(buf, "change-data"); return 0; } }, 42);

Argument coercion on entry into an asmjs-ified function occurs *after* the is-heap-neutered check, so you can run against a neutered buffer.  The buffer's data pointer doesn't change, so you're just acting on data that "should" have been observably transferred out of the ArrayBuffer.  Which is apparently not an *actual* problem in terms of exploitability/crashing/etc. but still "just ain't right".

<luke> so fortunately i think that is benight
<luke> *benign
<luke> i guess that can happen even now
<luke> so we should move the neutering check down, just for good hygeine
<luke> and also fix this attempt to setNewOwnedData when isAsmJSArrayBuffer
<luke> iiuc
(Assignee)

Comment 38

5 years ago
(Oh, the testcase in comment 37 hits bug 1002864 right now on trunk/aurora, but the issue here is applicable on all branches even with that bug fixed [i.e. with bug 999755 backed out, or done right-er].)
Comment on attachment 8411393 [details] [diff] [review]
Fix all WebIDL, without having audited uses to be sure no reentry

mObj doesn't need to be mutable, since ComputeData doesn't write to it.

Please do file a followup on having inline length/data accessors on typed arrays so we can nix this local cache?  We'd need to measure a bit, since these inline things involve a bunch of computation, but I suspect it would be doable.  Then we would reduce our attack surface to "spec bugs", at least.  :(

>+// with a dozen-plus classes and varying slot layouts.

The slot layouts for all the typed arrays are the same; I assume it's just the DataViews that are different?  We should consider making them all the same.

>+// This one isn't inlined because there are a bunch of different ArrayBuffer
>+// classes that would have to be individually handled here.

Again, seems like we should fix this as needed, but I don't understand js::AsArrayBuffer: ArrayBufferObject is a superclass of SharedArrayBufferObject, so it seems like we could use the same cast for both, no?  Followup fodder is fine.

r=me
Attachment #8411393 - Flags: review?(bzbarsky) → review+
What is left to do here? Things ready to go in?

I assume this issue pre-dates 30?
status-firefox32: --- → affected
tracking-firefox-esr24: --- → +
(Assignee)

Comment 41

5 years ago
What remains to be done is to finish auditing all the uses of any of byte length, byte offset, data pointer, and probably buffer as pertains to any ArrayBuffer or view of one, making sure computed values are not used after a potentially invalidating operation might occur.  I *think* the JS engine part of that fixing is done (well, done except with respect to TypedObject code, which I ignored in the first audit).  No guarantees about anything else.  And, until the audit is done, no guarantees that I haven't missed some other issue in all this.

All branches, releases, etc. back to the introduction of neutering are affected, just as with bug 982974.
(Assignee)

Comment 42

5 years ago
JS_GetStableArrayBufferData looks increasingly like a bad API.  The intent of it made sense, when it was only dealing with the potential of moving GC moving an object around, potentially invalidating exposed pointers to data.  But once neutering comes into play, it's fundamentally impossible in general to expose a "stable" data pointer.  The name gives a false sense of security that makes it even more likely that mistakes re neutering will happen.  On trunk, at least, I think we should probably remove it.

Technically they're not security holes because you need chrome code that behaves "just so" to do anything, but there seem to be a bunch of places in streams-related code that try to read into the data pointer of an ArrayBuffer.  That's fine if the ArrayBuffer isn't exposed, or isn't exposed at any point during that read.  But because streams are often proxied, and streams are scriptable, technically every Read((uint8_t*) JS_GetArrayBufferData(buf), ...) could be dangerous.  I'm not entirely sure yet what to do about that.  Sometimes the ArrayBuffer was just created in the code, so can't be exposed yet, so things are fine.  But not always.

Still sorting through uses of this stuff in Gecko right now, but I think I'm down to the last few left now.  They look tricky but manageable.  Hopefully I'll have them wrapped up by end of day today.
(Assignee)

Comment 43

5 years ago
Running up against some very gnarly ArrayBuffer uses in Gecko, that are not obviously correct even setting aside neutering concerns.  (!)  Will try to ping people in the relevant areas (webaudio, ctypes) on Monday, seeing as I missed them today.  Right now mach-build-looping indicates those are the only remaining issues, although it's plausible there are more hiding past those compilation failures (which the build-looping tries to evade, but only with partial success).
(Assignee)

Comment 44

5 years ago
I filed bug 1005960 for a lack of clarity in one particular bit of code implicated in the typed array/ArrayBuffer guts-accessor methods renames audit.  The concern mentioned there is not strictly relevant to this bug, except in what was touched.

The concern about those parts as concerns this bug, was that SetRawChannelContents writes into elements that in theory might be neutered.  However, all the callers right now are all setting contents of a freshly created AudioBuffer, that's never escaped to script.  So I think neutering isn't a concern for them.  Is that accurate?  (And might there be some sort of way to structure the code, such that this precondition is clearer, that the channel typed arrays haven't escaped?)

One or the other of the WebAudio people just needinfo'd, please comment to verify this, and to maybe consider a nicer/obviously-safer API here (but for another bug, of course).  :-)
Flags: needinfo?(paul)
Flags: needinfo?(ehsan)
I _think_ that's accurate, but you really want ehsan's and roc's opinion on this. Don't count this reply as being and answer please.
Flags: needinfo?(paul)

Comment 46

5 years ago
Filed bug 1006229 to MOZ_ASSERT the precondition suggested in comment 44.
Flags: needinfo?(ehsan)
(Assignee)

Comment 47

5 years ago
I'm looking into all the WebIDL hits, at the lower level of forcing every user to manually/explicitly compute length/data at just the right time, to get side effects in the right observable order.  (Specs will need to be clear when they compute data/length information, where reentry is possible.  Probably none do so right now because this issue is still rather under-explored in the specs.)

There seem to be a bunch of cases where we extract this data, then create instances of XPCOM components and stuff using user-overridable contracts -- say, for example, NS_UNICODEDECODER_CONTRACTID_BASE "ISO-8859-2" or so within the TextDecoder/TextEncoder stuff.  Now, we have builtin support for most of these.  But someone could override the builtin stuff, or provide other contract IDs and visibly change the APIs in question.  In the long run we want to get rid of that.  In the short run, I guess sometimes we just run the very-slight risk of someone doing something dumb and overriding?  I'm not really satisfied doing this (and in some cases I think a workaround is warranted, if the overridden thing is sufficiently general), but I guess perfect shouldn't be the enemy of good along those lines.

Even if these cases invoke no untrusted JS code, there's still the potential for untowardness if somehow a GC occurs.  GC could move stuff, and all that.  But, good news everyone!  Terrence tells me this *doesn't* happen right now, because of a recent bhackett change.  So, life here is a whole lot simpler as neutering is the *only* reason we need to worry about data pointer invalidation.

In the long run, of course, we're going to have to do something about this.  But for now, phew.
(Assignee)

Comment 48

5 years ago
Good news, everyone!  Aside from bug 1007223 ("other than that, Mrs. Lincoln..."), it looks like all the WebIDL users of this stuff are safe, in some cases assuming XPCOM component creation doesn't go off into the weeds as alluded to in comment 47.  I'm going to split up that patch into a part that makes the computation of data/length explicit, plus a part that renames length/data to zlength/data.  The second part, I'm going to try to split up into separate pieces, then parcel out for review of them wrt whether each place is neutering-safe or not.  We'll see if I can get some of those patches up today before a dental appointment or not, otherwise it'll be tomorrow.
(Assignee)

Comment 49

5 years ago
My understanding of the code is that SIMD values' underlying array buffers are never directly exposed.  If this is so, these assertions should all be good, and they should document that the data being accessed will never have disappeared out from under us.
Attachment #8420394 - Flags: review?(benj)
(Assignee)

Comment 50

5 years ago
The buffer can still be neutered underneath running code.  But it seems there's no good reason not to push this test as far along as we can -- after argument coercion, then.
Attachment #8420417 - Flags: review?(luke)
(Assignee)

Comment 52

5 years ago
Presumably fixing bug 1004767 will at some point make the current argument conversion code for TypedObject side-effectful.  At that point, the is-neutered check has to start happening later.  No reason not to move the check later right now, so we can't accidentally forget to do that later.

No test here, because the current argument conversion code can't reenter to make this into an actual bug.
Attachment #8420422 - Flags: review?(nmatsakis)
(Assignee)

Comment 53

5 years ago
It's certainly the case that nsIInputStream::Read is unscriptable.  But that doesn't guarantee that the method won't reenter JS, or perturb the JS engine in such a way that the buffer data would be un-inlined, or (eventually) that a GC could happen (perhaps through a memory-pressure event) and move the data.  So it seems like we need to have an intervening copy to properly defend against this.

The is-neutered test would be better expressed with a JS_IsArrayBufferNeutered method, but I'm loathe to scope-creep this bug any more than necessary (already far too much).  I believe a bug was filed just today to add such a method; I'll make sure the patch there changes this code when it lands.
Attachment #8420435 - Flags: review?(benjamin)
(Assignee)

Comment 54

5 years ago
Manual CSE helps readability a lot here.  Plus it made renaming byteLength->zbyteLength when auditing (I changed every accessor's name, then checked/changed each use to be sure it didn't use a stale value) much easier.
Attachment #8420437 - Flags: review?(luke)
(Assignee)

Comment 55

5 years ago
Another bit of manual CSE, for the same reason.

Conceivably a compiler could figure this one out on its own, but extracting a length() from every arm of a large switch with loops in every arm seems like a heroic optimization to expect the compiler to perform.  This is much better.
Attachment #8420440 - Flags: review?(sphink)
(Assignee)

Comment 56

5 years ago
Somehow I had this tweak made lower in the patch-stack than my patches to rename every accessor method.  It's super-small, and it's easier to just get it reviewed/landed, than to extricate it from underneath that change (and then deal with the renaming patch not being something that would/should ever land).
Attachment #8420442 - Flags: review?(sphink)

Updated

5 years ago
Attachment #8420437 - Flags: review?(luke) → review+
Comment on attachment 8420417 [details] [diff] [review]
Test for asm.js ArrayBuffer heap neutering during arguments coercion

This throws now as an impl detail, but we'll eventually fix this not to throw.  Can you change the test to accept throwing or not (really, it's just testing for lack of asserts/crashes).
Attachment #8420417 - Flags: review?(luke) → review+
Comment on attachment 8420418 [details] [diff] [review]
...and the patch to move the neutering test after argument coercion

Thanks!
Attachment #8420418 - Flags: review?(luke) → review+
Comment on attachment 8420394 [details] [diff] [review]
Add assertions to SIMD code that various buffers aren't neutered

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

Looks good to me.

::: js/src/builtin/SIMD.cpp
@@ +280,4 @@
>  #define STORE_LANES(_constant, _type, _name)                                  \
>        case _constant:                                                         \
>        {                                                                       \
> +        MOZ_ASSERT(!result->owner().isNeutered());                            \

nit: you could hoist this above the switch?

@@ +426,4 @@
>      if (!result)
>          return nullptr;
>  
> +    MOZ_ASSERT(!result->owner().isNeutered());

In this case, result is created just a few lines earlier so I can't see how it could be neutered at this point (even in a multi-threaded context, that seems unlikely as the object is entirely controlled by one single thread all the way). However, this works very well as a sanity check for newly created typed objects, so it seems fair to keep it.
Attachment #8420394 - Flags: review?(benj) → review+
(Assignee)

Comment 60

5 years ago
Okay, so I *think* the last bit of auditing is done, as far as the WebIDL users go.

The WebIDL-directed patch here defers data/length extraction from array buffers and views as long as possible -- to the point where the first need for data/length occurs.  This is pretty haphazard, so it seemed best to make the length/data computation explicit, via a ComputeLengthAndData method, that *must* be called before Length/Data can be called.  That forces at least a little bit of care about when these values are computed, and it makes that computation explicit.

That still leaves the problem of this computed data being invalidated through reentry or similar operations prior to use.  I approached this by renaming Length/Data in dom/bindings/TypedArray.h to ZLength and ZData.  Then I renamed every caller -- conveniently also auditing for ComputeLengthAndData use at the same time.

But of course we don't want to rename these methods -- we just want CLAD added, and we want any reentry issues fixed.  So after writing that patch, I split it into one patch to add CLAD, and one patch to do renames.  The renames patch won't land, the CLAD one will.

But, because we want to be sure the audit was comprehensive, the patch we want reviewed is the combination of those two patches -- adding CLAD *and* renaming at the same time.  So this is going to be the slightly weird case of my posting patches for review that, in that form, I have no intention of ever landing -- only the kernel of them, minus the renames.  Confusing, I guess, but it seems the best path here.

WebIDL-targeted patch splitup coming shortly.  There's one other large-scale audit in need of similar patch split-up before posting, and with that done I think this is good to go as far as trunk is concerned.  (No idea how well this will all backport to branches.  :-\ )
(Assignee)

Comment 62

5 years ago
Jeff: right now length/data are computed, at argument processing time in source order.  That's no good, because a later argument can invalidate the data/length computed by an earlier one.  Attachment 8411393 [details] [diff] delays length/data computation until either Length() or Data() is called.  That fixes that immediate problem, but it's non-obvious when the computation happens.

This patch (atop that other one) makes it so that length/data computation only happens when explicitly requested.  (See attachment 8421278 [details] [diff] [review] for the changes to make this happen.)

An additional worry is that after this computation occurs, code will do *something* that might cause a computed length/data to be invalidated.  Any sort of reentry into JS could conceivably do the trick.  So I need you to double-check that every place where I did s/Data/ZData/ or s/Length/ZLength/, no reentry can have happened between the ComputeLengthAndData call, and that later access.  It looks like GL is largely fine because it's so low-level, but I need a double-check on that from someone who knows the code here.  Note that there are places here where potentially neutered pointers/length/data are passed into methods, and then *those* methods do extra validation checks that catch this neutering.  It's kind of scary, but it seems to check out.

The plan is to land all this change, *without* the Z-renaming bit.  But the Z-renaming bit is a demonstration that all relevant places were examined, and it's an easy way to flag them for a double-check from someone closer to the code.
Attachment #8421282 - Flags: review?(jgilbert)
(Assignee)

Comment 63

5 years ago
Policy decision time: do we care that it's permissible for someone to override various builtin contract IDs for core stuff, in ways that could cause this to break?  I think we have to say no in the short run.  (And in the long run, none of this stuff should go through XPCOM at all, if it's web-exposed.)

The DOMParser thing spins up a component from a contract ID to do parsing.

The crypto thing has similar sorts of issues wrt the source of random bytes used to write into the buffer.

The Decoder case seems to spin up a "@mozilla.org/foo/blah/?type=" + type component.  That seems to hit builtin stuff, but conceivably a JS component could override it.
Attachment #8421291 - Flags: review?(bzbarsky)
(Assignee)

Comment 64

5 years ago
See comment 62 for background on what this patch is supposed to do, more or less (ignoring the GL-specific details).

As far as I can tell, the WebAudioUtils::ConvertLinearToDecibels sort of stuff can't/doesn't reenter -- an assumption worth verifying.

AudioBuffer::Copy{From,To}Channel have JS_GetTypedArrayLength calls that verify that mJSChannels[aChannelNumber] hasn't been neutered.  I have a separate patch that renames that to JS_ZGetTypedArrayLength, to be posted for review later.  With that knowledge considered, I *think* things are okay in those instances.

The changes to AudioContext::DecodeAudioData are not an implication of correctness there -- that code's buggy per bug 1007223, discovered during this audit (woo!).  I'll have to redo things there after the fix there lands.

Otherwise there seems to be a lot of passed-in data that's immediately copied, which should be safe.  Double-check 'em for me, please!
Attachment #8421296 - Flags: review?(ehsan)
(Assignee)

Comment 65

5 years ago
See comment 62 for background here (ignoring the GL bits).

All the particular cases in play here, we take the incoming typed array(buffer) data and immediately copy it into something else, so they should be safe.

The LockedFile case also pulls in the possible worry about XPCOM contractids being overridden, see comment 63.  I'm assuming that again we're not worrying about that.
Attachment #8421300 - Flags: review?(bugs)
(Assignee)

Comment 66

5 years ago
This case was kind of ugly.  Off the top of my head setData doesn't immediately imply a copy, and if you descend into nsStringInputStream you find it devolves into a |mData.Assign(data, dataLen)| where |mData| is an |nsDependentCSubstring|.  That apparently happens to be a copy (verified by stepping into it in a debugger, because no mortal man can read our string code to figure out where any particular string's implementation lies), so we're safe -- the correct amount is copied, and a subsequent neuter/whatever won't affect things.
Attachment #8421303 - Flags: review?(benjamin)
(Assignee)

Comment 67

5 years ago
These should be okay assuming NS_NewByteInputStream with NS_ASSIGNMENT_COPY specified doesn't reenter.  Again, the same sort of policy decision from comment 63.
Attachment #8421309 - Flags: review?(bugs)

Comment 68

5 years ago
Comment on attachment 8421296 [details] [diff] [review]
WebIDL 4 - WebAudio

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

So I double checked everything as you requested.  What's going to keep these invariants non-violated once this lands?

::: content/media/webaudio/AudioContext.cpp
@@ +443,5 @@
>  AudioContext::DecodeAudioData(const ArrayBuffer& aBuffer,
>                                DecodeSuccessCallback& aSuccessCallback,
>                                const Optional<OwningNonNull<DecodeErrorCallback> >& aFailureCallback)
>  {
> +  aBuffer.ComputeLengthAndData();

Note that bug 1007223 is touching this code, so please rebase if Paul wins the push race.
Attachment #8421296 - Flags: review?(ehsan) → review+
Comment on attachment 8421282 [details] [diff] [review]
WebIDL 2 - WebGL: make ComputeLengthAndData explicit, rename Length/Data

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

Non of this is re-entrant right now, but it would help to leave a reminder, as this isn't an obvious requirement.

It would be awesome to lean on the compiler a bit, and make this behavior assert in DEBUG, if it doesn't already.

::: content/canvas/src/WebGLContextGL.cpp
@@ +2144,5 @@
>  
>      if (!checked_neededByteLength.isValid())
>          return ErrorInvalidOperation("readPixels: integer overflow computing the needed buffer size");
>  
> +    pixbuf.ComputeLengthAndData();

Can you leave a comment on at least one of these lines, stating that this is only safe if we're non-reentrant between here and Length()/Data() and their use?
Attachment #8421282 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 70

5 years ago
On first readthrough I thought the strings passed down to the secondary Send() would be copies, so this would be safe.  Poking in a debugger, that's not the case.  Aarrghh!

It *appears* the websocket code will hit WebSocketChannel::SendMsgCommon with |!aStream|, which will do an immediate copy of the data.  So that seems okay.

The datachannel bits, I'm a bit less sure of.  I'm not sure where we have a test in the tree that I could examine in a debugger to figure things out, to be fully confident in that code.  Examining DataChannel.cpp, I *think* the data's either sent immediately, or copied into BufferedMsg structs (and therefore safe).  Could you verify this understanding for me, and perhaps point me at an existing test of this so I can double-check in a debugger?
Attachment #8421349 - Flags: review?(bugs)
(Assignee)

Comment 71

5 years ago
ImageData is really a bit weird about neutering, in that I *think* current code is nonsense but actually safe.

In the current world without any patches from this bug, you can create an ImageData from a neutered-during-argument-coercions ArrayBuffer, and that's just peachy so long as when initially computed, length/data were consistent with the width/height values passed in after coercions.  The result would be an ImageData with non-zero .width/.height, backed by a neutered array.

Now, that's weird, but ImageData is really only consumed by putImageData.  And there, there's code that checks the length of the data corresponds to width*height*4 passed along from the ImageData object itself.  So I think there's no distinct security issue in ImageData/putImageData as they stand, just some oddness.
Attachment #8421375 - Flags: review?(bzbarsky)
(Assignee)

Comment 72

5 years ago
And now that all the combined-patches are posted, here's the patch I'm actually going to land, to deal with dom/bindings/TypedArray.h getting a ComputeLengthAndData method.
(Assignee)

Comment 73

5 years ago
...and the combined patch that renames all the length/data users to Z-prefixed names.  Not for landing, just to say that:

  this attachment
  +
  attachment 8421379 [details] [diff] [review]

is the same as all the "WebIDL * - *" patches just posted, combined into one -- posted for additional clarity.
Comment on attachment 8421375 [details] [diff] [review]
WebIDL 9 - ImageData stuffs

r=me.

Note that you can take a perfectly good ImageData, then neuter its typed array's buffer, then pass it to putImageData, so putImageData definitely needs to deal with an incoming ImageData whose typed array doesn't match its size...  I'm glad ours does.

I reported a spec bug at https://www.w3.org/Bugs/Public/show_bug.cgi?id=25672 because the spec doesn't handle this.
Attachment #8421375 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 75

5 years ago
(In reply to :Ehsan Akhgari (@work week, needinfo? me!) from comment #68)
> What's going to keep these invariants non-violated once this lands?

Nothing.  There's no coherent way to expose length/data that completely avoids any possible way for values to get out of sync in bad ways.  The second you get either value, we're in a zone where it's unsafe to touch the underlying buffer in particular ways.  And it's not safe to just make the original object always get consulted, because then you might have a pre-reentry length paired with post-reentry data.  I see no solution to this problem other than constant vigilance.  :-\

Oh, things are going to get worse if we ever decide to allocate typed arrays as non-tenured/in the nursery, because then data can move around even if the buffer *isn't* neutered, in response to a JS GC, which could be in response to simple XPCOM-observed memory pressure.  In theory that can be sort of addressed by exposing typed array/arraybuffer/view data/length through a handle system, or so.  But ultimately, code asking for this stuff *wants* a raw pointer, or a raw uint32_t.  The handle *must* boil away at some point.  Once it does, you're back in the danger zone again.

Fortunately, non-tenured typed arrays are a ways off, so out of sight, out of mind right now.  When we make that change, this issue will be *very* prominent in the minds of the people dealing with it, I assure you!

Unless the standards process comes up with something here (like some mad idea about "locking" array buffers so they temporarily can't be neutered, while some other operation is in flight -- but beware reentrancy, behavior in case of early exits, and all that), I have nothing here.  (And the standards process can't do anything about the tenured/nursery issue, at the point we look to move forward with that.)
(Assignee)

Comment 76

5 years ago
(In reply to Jeff Gilbert [:jgilbert] from comment #69)
> Non of this is re-entrant right now, but it would help to leave a reminder,
> as this isn't an obvious requirement.

I sort of left something, ish, locally.  It's sort of messy, tho, because in the short run we don't want to point this out as an issue to people watching our incoming commits.  :-\

> It would be awesome to lean on the compiler a bit, and make this behavior
> assert in DEBUG, if it doesn't already.

Well, it's spec-permissible (as much as any spec handles this problem; some don't).  Things that can validly happen, can't trigger assertions.  If we could distinguish properly-anticipated neuters from unanticipated ones, I'd make the latter ones MOZ_CRASH() in a heartbeat.  But there isn't without spec changes to "lock" objects or similar as noted in comment 75.

> Can you leave a comment on at least one of these lines, stating that this is
> only safe if we're non-reentrant between here and Length()/Data() and their
> use?

Done, a little vaguely wrt consequnces:

    // Compute length and data.  Don't reenter after this point, lest the
    // precomputed go out of sync with the instant length/data.
(Assignee)

Comment 77

5 years ago
Er, precomputed *values*.
Attachment #8420440 - Flags: review?(sphink) → review+
Attachment #8420442 - Flags: review?(sphink) → review+
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25672 has been fixed in the spec.  Do we want to go ahead and update our impl to that change here or in a separate bug?  Basically, we need to change which exception we throw for the neutered case.
I'll review the patches here one attachment 8421278 [details] [diff] [review] has been reviewed.
(But it is not quite clear to me why we want ComputeLengthAndData() + Data() + Length()
and why not just GetLengthAndData)
(Assignee)

Comment 80

5 years ago
Okay, last bit of auditing done.  (...for trunk.  Wheeeeeeeee!)  (Unless I'm missing some other way any of this could possibly go awry.  People should think creatively about whether/how length/data/byteOffset/byteLength might be exposed by some other ways I haven't audited somehow!)  The final strategy here is like the WebIDL z-renaming thing -- just applied to every method exposing a view/buffer's length/byte length/byte offset/data/buffer.

At this point I think all the actual issues have been fixed by patches here, so this is just a long series of patches to Z-rename, that will never land.  Really tedious, really detail-oriented, easy to make mistakes here.  But it seems like it had to be done.  And it's found a handful of different bugs, so it's sadly been worthwhile.

Anyway.  Full renaming-patch here, patch-bombing to split it up to commence.
(Assignee)

Comment 81

5 years ago
People reviewing subsequent renaming patches may potentially be interested in this patch as a way to see what's been renamed, in the public-outside-the-engine sense.
Attachment #8421967 - Flags: review?(sphink)
(Assignee)

Comment 82

5 years ago
If any of this stuff is changing in a GC, we've got bigger problems than neutering.
Attachment #8421969 - Flags: review?(terrence)
(Assignee)

Comment 83

5 years ago
Hmm.  My initial runthrough on this apparently missed the JS_CODEGEN_X86 and JS_CODEGEN_ARM cases.  This patch adds the appropriate renaming to them.  Still, it suggests that possibly I've missed stuff in platform-specific code, potentially.  :-\  Hopefully not too much else, if any, but try server (when I feel safe doing it) could end up being enlightening.  :-\
Attachment #8421976 - Flags: review?(luke)
(Assignee)

Comment 84

5 years ago
I spent zero time actually looking at these on the assumption they were trusted code, not doing anything stupid.
Attachment #8421979 - Flags: review?(sphink)
Comment on attachment 8421976 [details] [diff] [review]
JS 3 - asm.js renaming

Ah, comment 80 says this will never land, it's just to show you've hit all the places.  These places look fine.
Attachment #8421976 - Flags: review?(luke) → review+
Comment on attachment 8421969 [details] [diff] [review]
JS 2 - GC renaming

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

rs=me
Attachment #8421969 - Flags: review?(terrence) → review+
(Assignee)

Comment 87

5 years ago
Welp!  I kind of trailed off there after that fourth patch, with something like eighteen to go.  Turns out the next renaming patch I was going to post had incorrectly audited one of the places it had changed, as I noticed when doing a skim of it to see what helpful comments I could make on it when posting it.  Filed bug 1009952 on that, with test/patch.  Woo!  \o/ /o\

Now, back to posting ongoing patches in a moment, after I deal with rebasing the renaming patch through that bug's changes.  Although, it's close to end of day and there's a barbecue at the office, so I may not get far.  ;-)
(Assignee)

Comment 88

5 years ago
The *offset things for JITs probably didn't need auditing, but better safe than sorry.  Most of them then (naturally) consult the instantaneously-canonical values in memory and so are almost obviously, trivially safe.

The DoSetElemFallback hit is a little interesting, because it happens after the fallback stub code executes.  So the length we get there could be 0 because neutering, and all.  That seems fine to me, just a slight touchiness worth noting specifically.

The MIR.cpp changes were the part I mentioned in the previous comment as having been incorrectly audited.  See the referenced bug there fro details of how this stuff was actually wrong, and how it got fixed.  Because the correctness of this depended on the code that actually created the MLoadTypedArrayElementStatic or MStoreTypedArrayElementStatic, that bug's changes don't actually conflict with anything in this patch.  Spooky interactions at a distance.  :-\
Attachment #8422541 - Flags: review?(jdemooij)
(Assignee)

Comment 89

5 years ago
Posted patch JS 6 - CTypesSplinter Review
If these don't make you at least a little scared, something's wrong:

       // Just as with C arrays, we make no effort to
       // keep the ArrayBuffer alive.

       // Convert TypedArray to pointer without any copy.
       // Just as with C arrays, we make no effort to
       // keep the TypedArray alive.

This is kind of a pre-existing, separate issue, tho, so I am more or less ignoring them for now.  Not to mention, extensions/chrome code would have to go to some effort to pass an ArrayBuffer into a ctypes method, then reenter enough to neuter that ArrayBuffer, for things to go truly wrong with this.

The other hunk is simple queries whose results are used almost immediately, so relatively obviously safe.
Attachment #8422544 - Flags: review?(jorendorff)
(Assignee)

Comment 90

5 years ago
Posted patch JS 7 - SIMDSplinter Review
bbouvier, I think you looked at most/all of these hits when adding the not-neutered assertions earlier, so this should be a trivial review.
Attachment #8422545 - Flags: review?(benj)
(Assignee)

Comment 91

5 years ago
This is one of the two messy splitups.  Someone's gotta take 'em.  :-\

fun_slice code is safe *only* because it delegates to createSlice(), which makes it safe.  It's perhaps reasonable at some later time to get rid of createSlice and just do it in fun_slice fully, maybe.

The refactoring to where js::GetArrayBufferViewLengthAndData is defined, and having it be used in JS_GetObjectAsArrayBufferView, should probably be done at some point, but I'm running out of patience to make that change in an earlier patch in this sequence.  Doesn't hurt to not do it for now.
Attachment #8422553 - Flags: review?(sphink)
Attachment #8422545 - Flags: review?(benj) → review+
(Assignee)

Comment 93

5 years ago
La la la, another issue missed during the initial audit...
Attachment #8422633 - Flags: review?(sphink)
(Assignee)

Comment 94

5 years ago
The other big fun part here.

Again, the safety for fun_subarray_impl is found in the createSubarray that it calls, and the two probably should be unified at some point.

The set() code could probably use extra care, given I found another missed bug in it just now.

DataViewObject::construct looks screwy, but it too is handled by a backstop check in DataViewObject::create.  Careful looking there appreciated too.
Attachment #8422646 - Flags: review?(sphink)
(Assignee)

Comment 95

5 years ago
I am somewhat accepting on faith that structured cloning handles midflight neutering attempts, because if we don't all this code is way unsafe.  Is that the case?  Do we have tests for structured cloning while you structured clone?  Two reviews for double the confidence (hopefully, at least -- given this whole saga's track record optimism seems never warranted here).
Attachment #8422652 - Flags: review?(sphink)
Attachment #8422652 - Flags: review?(jorendorff)
(Assignee)

Comment 96

5 years ago
After all the madness here, these renames should only implicate fixed code.
Attachment #8422653 - Flags: review?(bzbarsky)
(Assignee)

Comment 97

5 years ago
Most of these are just exposing numbers that will be instantaneously accurate but aren't used in dangerous code.  A few are accessing raw data, then doing things that never recur into the engine.  Nothing too tricky about any of the code being renamed here, seems to me, so globbed up into one patch.
Attachment #8422655 - Flags: review?(sphink)
(Assignee)

Comment 98

5 years ago
The self-hosted code is really hard to follow through for understanding of how callers and stuff are defined.  I am mostly *presuming* that if we're dealing with raw memory ranges here, this code is working with newborn, not-yet-exposed objects such that there's no concern with any of these renamed places.  Double-check me, please!
Attachment #8422660 - Flags: review?(shu)
Attachment #8422660 - Flags: review?(nmatsakis)
Comment on attachment 8421967 [details] [diff] [review]
JS 1 - Rename all the friendapi methods

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

The only other thing I can think of is the newly-added JS_IsNeuteredArrayBufferObject, but it doesn't currently appear anywhere in the tree, so never mind.
Attachment #8421967 - Flags: review?(sphink) → review+
Attachment #8421979 - Flags: review?(sphink) → review+
Comment on attachment 8422553 [details] [diff] [review]
JS 8 - ArrayBuffer.{cpp,h}

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

This is missing isNeutered() and therefore hasStealableContents(). But they're unlikely to do harm; isNeutered() could only get stale if it started out false and became true. I checked all users of both of those functions and it was trivially safe, except for some uses in TypedObject.cpp that are doing isInt32/toInt32 that look like they could become ToInt32 in the future. But even then, they're followed by range checks, which would prevent problems.
Attachment #8422553 - Flags: review?(sphink) → review+
Comment on attachment 8422632 [details] [diff] [review]
Test for typedArray.set(arraylike, offset) working when arraylike.length invokes a neutering getter

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

Nice.

::: js/src/tests/ecma_6/TypedArray/set-object-funky-length-neuters.js
@@ +4,5 @@
> + */
> +
> +var gTestfile = "set-object-funky-length-neuters.js";
> +//-----------------------------------------------------------------------------
> +var BUGNUMBER = 9999999;

991981
Attachment #8422632 - Flags: review?(sphink) → review+
Comment on attachment 8422633 [details] [diff] [review]
Handle screwy arraylike objects passed to %TypedArray.set correctly

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

Ouch
Attachment #8422633 - Flags: review?(sphink) → review+
Comment on attachment 8422646 [details] [diff] [review]
JS 9 - TypedArrayObject.{cpp,h}

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

Wow, your life has really sucked recently, hasn't it?
Attachment #8422646 - Flags: review?(sphink) → review+
Attachment #8422652 - Flags: review?(sphink) → review+
(Assignee)

Comment 105

5 years ago
So I went back and *actually* reviewed the TypedObject-related bits of the auditing patch, and it seems like my "audit" was...not really one, and I'd sort of forgotten this by convenient loss of memory.  Running through it again a few issues, of which this is the first.

Given I've really only audited this, well, a single time (versus the effective two for everything else -- once before, and again during scanning before upload now), it's quite reasonable to expect I missed something.  Extra caution warranted!
Attachment #8422817 - Flags: review?(nmatsakis)
(Assignee)

Comment 107

5 years ago
Attachment #8422819 - Flags: review?(nmatsakis)
(Assignee)

Comment 108

5 years ago
Eric in particular here (you can ignore the rest of the patch): the TypedObject::obj_enumerate bit has me wondering if we ever expose any sequence points between repeated calls to an enumerate hook.  I don't *think* we do any more -- we just enumerate the entire thing at once to create a list of ids to expose.  If that's the case, this is good.  If it's not the case, then |JS_ASSERT(index == typedObj->length());| will fail, but we'll cut off enumeration early as the very next step, so we'll be safe even if we assert.

Either way it's at safe in release builds, so I'm not going to think too hard about it.

Probably typedMem() should include a !isNeutered() assertion in it at some point.  Maybe in a followup bug conceivably (but I guess the in-the-middle-of-GC callers and the like probably complain if you just add it).  My energy for anything but bug-fixing on point here wanes (although all of the patches I've gotten to write today, versus just mind-numbing auditing, have kind of acted as a shot in the arm to my enthusiasm level right now :-) ).  (Why in the world would a shot in the arm be a good thing?  The mind digresses....)
Attachment #8422825 - Flags: review?(nmatsakis)
Attachment #8422825 - Flags: review?(efaustbmo)
(Assignee)

Comment 109

5 years ago
(Rationale for patch, if you haven't seen it already: we totally screw up accessing typed array/arraybuffer/etc. data and/or length in cases.  We compute the value, then we do something that causes the structure to be neutered, and then we go use a stale length/data pointer.  To fix this everywhere I'm auditing every place that accesses this stuff, fixing every one to handle the case where the data gets neutered.  The easiest way to find all of them was to add "Z" to the names, then individually fix every place, checking along the way for buggy usage.  You get to review that auditing patch and double-check me to be sure I audited correctly and didn't misconstrue unsafe code as safe.  :-) )

We bounds-checked for CheckTargetAndPopulate near the start of the one caller, XPCConvert::JSTypedArray2Native -- which is the other rename in this patch.  Agreed?
Attachment #8422833 - Flags: review?(bobbyholley)
(Assignee)

Comment 110

5 years ago
These ones are trivial -- accessing data of a newly-created ArrayBuffer, not exposed in the scope of the method.  Obviously safe.
Attachment #8422838 - Flags: review?(sphink)
(Assignee)

Comment 111

5 years ago
AppendVoidPtr and InsertElementsAt copy into an array or so, so are safe.  SendAudioImpl copies into fresh memory, so is safe as well.
Attachment #8422843 - Flags: review?(bzbarsky)
Comment on attachment 8422646 [details] [diff] [review]
JS 9 - TypedArrayObject.{cpp,h}

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

::: js/src/vm/TypedArrayObject.cpp
@@ +1328,5 @@
>  
>      }
>  
>      NewObjectKind newKind = DataViewNewObjectKind(cx, byteLength, proto);
>      obj = NewBuiltinClassInstance(cx, &class_, newKind);

If NewBuiltinClassInstance could neuter, then DataViewObject::create could end up with a nonzero length with a neutered buffer. We are assuming that it cannot, which seems reasonable, though Waldo made me more nervous by pointing out that this could conceivably call self-hosting code. Still, we probably have bigger problems if self-hosting execution escapes.

I wonder if we should have a AssertCannotCallNeuteringJS RAII class... (or more generally, cannot call JS except for self-hosted JS?)

@@ +1374,5 @@
>          return false;
>      }
>  
>      Rooted<ArrayBufferObject*> buffer(cx, &AsArrayBuffer(bufobj));
> +    uint32_t bufferLength = buffer->zbyteLength();

Er, check me on this one. We're in DataViewObject::construct. It looks like this byteLength() can return nonzero, then the byteOffset fetch neuters the buffer, then DVO::create gets a nonzero length which goes straight into the byteLength slot.

On a related note, InitArrayBufferViewDataPointer currently does MOZ_ASSERT(buffer->dataPointer() != nullptr). I bet it could assert !buffer->isNeutered() too.
Attachment #8422646 - Flags: review+ → review-
(Assignee)

Comment 113

5 years ago
See comment 109 for what this patch is trying to do, and why you're looking at it, even tho it's never expected to land.

I claim mArrayBuffer is only exposed when the state of the reader reaches "done" or so.  Before then the buffer is purely internal, so nothing can possibly neuter it, or cause its data to be un-inlined by JS_GetStableArrayBuffer data or so.
Attachment #8422847 - Flags: review?(jonas)
(Assignee)

Comment 114

5 years ago
See comment 109 for the rationale for this patch, and what you're supposed to be considering when reviewing it, when it obviously isn't something we ever want to land.

We're writing into a fresh, unexposed ArrayBuffer here.  The data pointer of an ArrayBuffer can *only* change by neutering.  Typed arrays are allocated tenured, so the data pointer will never be something that could change during GC (if the typed array were hypothetically moved from nursery to tenured generation).  So |stream->Read((char*)arrayBuffer, bufferLength, &numRead);| has no chance of using a stale pointer at any point during execution.
Attachment #8422851 - Flags: review?(bent.mozilla)
(Assignee)

Comment 115

5 years ago
These renames are for things previously changed -- I think in attachment 8410672 [details] [diff] [review] and attachment 8420435 [details] [diff] [review] if I'm reading attachment titles correctly on the new-attachment page.  I have no idea if both those patches have gotten review yet (or even been denied) because I'm not really keeping up with bugmail on this bug amidst progress in attaching all these patches, so consider this review conditional on those patches having been r+'d already.  :-)
Attachment #8422853 - Flags: review?(sphink)
Comment on attachment 8422553 [details] [diff] [review]
JS 8 - ArrayBuffer.{cpp,h}

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

Marking the rename patches r- if I find anything that doesn't seem to be addressed in another patch.

::: js/src/vm/ArrayBufferObject.cpp
@@ +199,1 @@
>      uint32_t begin = 0, end = length;

This looks to me like it has a benign failure. We fetch length=100, begin=0, and end=0. The ToClampedIndex of end neuters the buffer. We call createSlice(0,100). In an opt build, the arrayBuffer->hasData() check makes this safe -- it gives us a zero-length buffer. In a debug build, we get an assert. That's not good for fuzzing.

It'd be better if both gave a RangeError. I haven't looked at the spec.
Attachment #8422553 - Flags: review+ → review-
(In reply to Steve Fink [:sfink] from comment #112)
> Er, check me on this one. We're in DataViewObject::construct. It looks like
> this byteLength() can return nonzero, then the byteOffset fetch neuters the
> buffer, then DVO::create gets a nonzero length which goes straight into the
> byteLength slot.

Ugh, I'm an idiot. https://bugzilla.mozilla.org/attachment.cgi?id=8409935 fixed this.
Attachment #8422646 - Flags: review- → review+
(In reply to Steve Fink [:sfink] from comment #116)
> @@ +199,1 @@
> >      uint32_t begin = 0, end = length;
> 
> This looks to me like it has a benign failure. We fetch length=100, begin=0,
> and end=0. The ToClampedIndex of end neuters the buffer. We call
> createSlice(0,100). In an opt build, the arrayBuffer->hasData() check makes
> this safe -- it gives us a zero-length buffer. In a debug build, we get an
> assert. That's not good for fuzzing.
> 
> It'd be better if both gave a RangeError. I haven't looked at the spec.

Fixed already in https://bugzilla.mozilla.org/attachment.cgi?id=8409937 with a TypeError. From IRC, we're going to wait for the spec to settle on this stuff before worrying about the details.
(Assignee)

Comment 119

5 years ago
AudioBuffer::RestoreJSChannelData: writing into a new array created with the right length is safe.

AudioBuffer::CopyFromChannel: the PodMove writes into |aDestination|'s data the number of elements in |aDestination|.  The source data in the !mSharedChannels case has mLength length if I remember correctly how mSharedChannels is allocated.  And per the |end.value() > mLength| check earlier, where end is >= length, that means the source data has enough in it to not move memory that's not within its purview.

AudioBuffer::CopyToChannel: again mJSChannels[i] being neutered is an early exit.  If not neutered, we're copying in |aSource.Length()| elements, consistent with copying from |aSource.Data()|.  The destination has length mLength per the last comment (if it's right).  And we know the start location plus |length| is <= mLength per the |end.value() > mLength| check above.  So this is safe.

AudioBuffer::SetRawChannelContents: per bug 1005690 and bug 1006229 I am assuming this is correct.  I've largely not even attempted to verify this myself, due to the large complexity involved in how |aContents| can be defined, and in how |mLength| could have been set.  Extra-special care warranted here.

StealJSArrayDataIntoThreadSharedFloatArrayBufferList: not accessing data/length, so safe.  (I only changed the get-buffer methods out of paranoia mostly, shouldn't really be problems with uses of them.  Or so I hoped, and if memory serves I've been proven right.)

AudioBuffer::GetThreadSharedChannelsForRate: handling neutering by design, good code, have a dog biscuit!
Attachment #8422857 - Flags: review?(paul)
Attachment #8422857 - Flags: review?(ehsan)
(Assignee)

Comment 120

5 years ago
TCPSocketChild DeserializeArrayBuffer: writing into new array, lengths pretty obviously consistent.

SendSend: InsertElementsAt will copy, and the math to be in-range looks right to me.

UnixSocketRawData makes a copy of its data.  RunTask copies into a fresh ArrayBuffer of the right size, obviously, so safe again.

I am mildly curious how it is that Nfc.* and Ril.* seem to be almost identical in these relevant respects.  Discussion from above applies again.
Attachment #8422861 - Flags: review?(bzbarsky)
(Assignee)

Comment 121

5 years ago
In only compiling on 64-bit I missed a few places.  Most got folded into the patches uploaded after that point.  This would have been in an already-uploaded patch.  This entire bug is hairy as all get-out, so woo, one last asm.js review for this, for x86-32 code only as I remember.

I don't entirely truly exactly understand this code, so you should be extra-cautious about dealing with it.  *Particularly* as whatever is being done here, applies *only* to x86-32 specifically, so you haven't looked at corresponding code for some other platform.  Does this handle neutering correctly?
Attachment #8422863 - Flags: review?(luke)
(Assignee)

Comment 122

5 years ago
And with that, the entire swath of Z-rename patchwork is uploaded!  I'll start going back through reviews and dealing with any comments that have come in in the last couple days, tomorrow.  (Oh, and rebasing against current tip, since my tree's a week or two old at this point.  :-) )

Hopefully this mess will all backport to branches without too much trouble....
Attachment #8422833 - Flags: review?(bobbyholley) → review+
Comment on attachment 8422553 [details] [diff] [review]
JS 8 - ArrayBuffer.{cpp,h}

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

Ok, everything I found is fixed.
Attachment #8422553 - Flags: review- → review+
Comment on attachment 8422660 [details] [diff] [review]
JS 13 - ParallelJS stuffs

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

I can't navigate the absurd number of patches here to figure out what's going on, so I'll ask the stupid question here: when are length/data computed and cached for the ArrayBuffers and when is the cache invalidated?
Comment on attachment 8422655 [details] [diff] [review]
JS 12 - Relatively simple/obvious renames in JS code

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

Marking r- for the minor self-hosting issue.

::: js/src/jsinfer.cpp
@@ +1614,5 @@
>      bool invalidateOnNewType(Type type) { return false; }
>      bool invalidateOnNewPropertyState(TypeSet *property) { return false; }
>      bool invalidateOnNewObjectState(TypeObject *object) {
>          TypedArrayObject &tarray = object->singleton()->as<TypedArrayObject>();
> +        return tarray.zviewData() != viewData || tarray.zlength() != length;

I am totally unfamiliar with this code. But should there also be || tarray.isNeutered() in here? I mean, it barely makes a difference -- it would only matter for zero-length typed arrays that get neutered (and are one of the flavor that keeps its original data pointer when neutered.) Are we supposed to throw an exception when you tried to do something to a neutered typed array anywhere? That would be observable.

::: js/src/jsobj.cpp
@@ +3952,5 @@
>      // of bounds accesses.
>      if (obj->template is<TypedArrayObject>()) {
>          uint64_t index;
>          if (IsTypedArrayIndex(id, &index)) {
> +            if (index < obj->template as<TypedArrayObject>().zlength()) {

I have never seen this syntax. Bizarro!

::: js/src/vm/SelfHosting.cpp
@@ +424,5 @@
>          uint32_t idx = args[idxi].toInt32();
>  
>          if (arrobj->is<TypedArrayObject>() || arrobj->is<TypedObject>()) {
> +            JS_ASSERT(!arrobj->is<TypedArrayObject>() || idx < arrobj->as<TypedArrayObject>().zlength());
> +            JS_ASSERT(!arrobj->is<TypedObject>() || idx < uint32_t(arrobj->as<TypedObject>().zlength()));

These really ought to use JS_ASSERT_IF (or MOZ_ASSERT_IF).

And they seem very slightly wrong. In an opt build, this seems fine -- if one of the typed arrays gets neutered during this call, setElement() will fail safely. But in a debug build, you'll get an assert.
Attachment #8422655 - Flags: review?(sphink) → review-
Comment on attachment 8422838 [details] [diff] [review]
JS 16 - Cases where a fresh ArrayBuffer never exposed to script is being accessed

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

::: content/canvas/src/CanvasRenderingContext2D.cpp
@@ +3878,5 @@
>  
>    uint8_t* src = data;
>    uint32_t srcStride = aWidth * 4;
>    if (readback) {
>      srcStride = readback->Stride();

Heh. A little further down is:

      // XXX Is there some useful swizzle MMX we can use here?

As it turns out: yes, yes there is. I came up with it in grad school a millenium ago. I didn't tell anyone but the class professor; still, it's obvious enough that I'd be surprised if image processing libraries don't already use it or something better. (Actually, I'd also assumed that newer MMX instruction sets must've added a way to do it more directly.)
Attachment #8422838 - Flags: review?(sphink) → review+
Comment on attachment 8422853 [details] [diff] [review]
JS 20 - A few things already fixed earlier in this bug

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

Not sure what my review here means. Yes, it looks totally unsafe. Do you want me to look at the fix too?
Attachment #8422853 - Flags: review?(sphink) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #95)
> Created attachment 8422652 [details] [diff] [review]
> JS 10 - Structured cloning
> 
> I am somewhat accepting on faith that structured cloning handles midflight
> neutering attempts, because if we don't all this code is way unsafe.  Is
> that the case?

I think so, though the reasons aren't straightforward.

When writing a typed array, imagine you wrote a length into the clone buffer and then neuter the underlying ArrayBuffer. If the ArrayBuffer was neutered before it was written out itself, then it will be written into the clone buffer as a zero-length buffer. When reconstituting the typed array, it will call eg JS_NewInt8ArrayWithBuffer and throw an error if the saved length or offset is nonzero. If the ArrayBuffer is neutered after it was written, the data will have been copied into the clone buffer already so the ArrayBuffer itself will be fine, though the typed array offset will end up being written in as zero, possibly resulting in the typed array being a view of the wrong portion of the ArrayBuffer. :-(

Except that none of this will ever happen, because we don't write out any properties of either typed arrays or ArrayBuffers other than byteLength, byteOffset, and the raw ArrayBuffer data. So writing a typed array is atomic with respect to JS that could neuter things. But I don't know if that might change.

> Do we have tests for structured cloning while you structured
> clone?

I think there's a basic one. Probably not enough.

I have a patch that cleans up the eventual potential offset issue. I'll need to add a test of some sort, though the test will be bogus for now. And I'll probably just sit on the patch for the time being, to avoid interfering.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #119)
> Created attachment 8422857 [details] [diff] [review]
> JS 21 - Web Audio
>
> AudioBuffer::SetRawChannelContents: per bug 1005690 and bug 1006229 I am
> assuming this is correct.  I've largely not even attempted to verify this
> myself, due to the large complexity involved in how |aContents| can be
> defined, and in how |mLength| could have been set.  Extra-special care
> warranted here.

The first bug number is probably wrong, and I can't access the second one.

I double checked the other call sites, and they are fine, but this is the one that needs the most attention.

Updated

5 years ago
Attachment #8422857 - Flags: review?(paul)
Attachment #8422857 - Flags: review?(ehsan)
Attachment #8422857 - Flags: review+
Comment on attachment 8422541 [details] [diff] [review]
JS 5 - JIT changes

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

This looks fine, but there's still a JIT hazard with typed array neutering. Will file a sec bug in a sec.

::: js/src/jit/BaselineIC.cpp
@@ +5075,5 @@
>          {
>              return true;
>          }
>  
> +        uint32_t len = tarr->zlength();

Yeah this could be 0 after neutering but it doesn't really matter.
Attachment #8422541 - Flags: review?(jdemooij) → review+

Updated

5 years ago
Attachment #8422863 - Flags: review?(luke) → review+
(In reply to Jan de Mooij [:jandem] from comment #130)
> there's still a JIT hazard with typed array neutering.
> Will file a sec bug in a sec.

Bug 1011007.
Comment on attachment 8420435 [details] [diff] [review]
Make nsBinaryStream::ReadArrayBuffer deal with the passed-in buffer being neutered mid-read

Somebody who knows JSAPI should review this.
Attachment #8420435 - Flags: review?(benjamin)
I don't think comment 109 was clear enough.

Instructions to reviewers:

This bug has two main types of patches. One patch renames various ArrayBuffer/TypedArray-related accessors to nonsense names, eg byteLength() -> zbyteLength() or JS_GetTypedArrayLength -> JS_ZGetTypedArrayLength. When reviewing these patches, the renaming is just pointing out every use of these accessors. Your task is to check the code that uses these values to verify that it does not end up depending on something that could get invalidated. For example:

  uint8_t* data = JS_ZGetTypedArrayData();
  uint32_t len = JS_ZGetTypedArrayByteLength();
  int32_t number = ToInt32(cx, args[1]);
  data[len-1] = number; // <-- buffer overrun

What could happen is that data and len are fetched from the typed array, then ToInt32 ends up invoking JS due to valueOf or something, and that JS code neuters the arraybuffer. Now data[len-1] is totally invalid, both because of 'data' and 'len' referring to a previous state of the world.

The invalidatable accessors include retrieving the length, byteLength, byteOffset, data pointer, and neutered-ness of ArrayBuffers or views of ArrayBuffers.

Waldo went through the entire code base and audited every occurrence of these accessors. Most of the *other* patches in this bug are fixes to the problems he found, though some have been fixed in other bugs. Your review task is to double-check that audit, or the portion of the audit represented by the patch (he's giving review to someone familiar with the area of the code in question.)

There are also review requests for the fixes themselves. Those are regular review requests -- does it fix the problem?
Whiteboard: [reviewers: read comment 133]
Attachment #8422851 - Flags: review?(bent.mozilla) → review+

Updated

5 years ago
Attachment #8421303 - Flags: review?(benjamin)
Attachment #8422863 - Attachment is patch: true

Updated

5 years ago
Attachment #8422660 - Flags: review?(shu) → review+
(Assignee)

Comment 134

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #78)
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=25672 has been fixed in the
> spec.  Do we want to go ahead and update our impl to that change here or in
> a separate bug?  Basically, we need to change which exception we throw for
> the neutered case.

Another bug seems better to me, given this bug's going to remain hidden for a bit after fixing, I would expect.  Plus people observing comments might look askance a little at a minor spec update being processed in a security bug, without particularly good reason to do so.
(Assignee)

Comment 135

5 years ago
(In reply to Steve Fink [:sfink] from comment #127)
> Not sure what my review here means. Yes, it looks totally unsafe. Do you
> want me to look at the fix too?

Yeah, more or less.  That is, are the uses of these functions, that are renamed in this patch, in code that is safe against the issues this bug implicates?

Perhaps this patch showing the code after the fixes in the two attachments noted in comment 115, with a whole lot more context included, is more reviewable in this sense.
Attachment #8422853 - Attachment is obsolete: true
Attachment #8423499 - Flags: review?(sphink)
(Assignee)

Comment 136

5 years ago
(In reply to Paul Adenot (:padenot) from comment #129)
> > AudioBuffer::SetRawChannelContents: per bug 1005690 and bug 1006229 I am
> > assuming this is correct.  I've largely not even attempted to verify this
> > myself, due to the large complexity involved in how |aContents| can be
> > defined, and in how |mLength| could have been set.  Extra-special care
> > warranted here.
> 
> The first bug number is probably wrong, and I can't access the second one.

Blah.  Bug 1005960 for the first number.  And I see ehsan's poked you for review/CC'd you on the second one now.
I filed bug 1011574 on the putImageData exception type thing.
(Assignee)

Comment 138

5 years ago
Comment on attachment 8422655 [details] [diff] [review]
JS 12 - Relatively simple/obvious renames in JS code

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

Retry, with additional explanatory comments.

::: js/src/jsinfer.cpp
@@ +1614,5 @@
>      bool invalidateOnNewType(Type type) { return false; }
>      bool invalidateOnNewPropertyState(TypeSet *property) { return false; }
>      bool invalidateOnNewObjectState(TypeObject *object) {
>          TypedArrayObject &tarray = object->singleton()->as<TypedArrayObject>();
> +        return tarray.zviewData() != viewData || tarray.zlength() != length;

The point of this code is to cause JITted code that assumes a particular typed array's element range is valid, to be thrown away, when that typed array is neutered.  (Or, at a future time, when its elements get moved, if the object is allocated in the nursery and uses inline slot storage for its elements.)  This constraint guards places that compute a typed array's data or length, where the JIT treats the typed array as a constant and assumes it isn't neutered.

We can then burn into the JIT code the exact data pointer, or the length, and rely on its never changing.  Should it ever change, the JIT code is thrown out.  This happens because one of the MarkObjectStateChange calls in ArrayBufferObject::neuter will resolve to this constraint.  Testing the constraint via this method compares the new values against the old, cached, depended-upon values.  If this method says to invalidate, the JIT code gets thrown out.

Neutering reduces the length to zero, so the second condition is almost always true if the underlying buffer has been neutered.  It's not true when the typed array had length 0 to start.  When neutering a zero-length array, the first condition may or may not be true, depending on "change-data" "same-data" semantics.  But in either case, the constraint was guarding an empty elements range.  We can't have been guarding an access, because such access would be out of bounds.  And if we were guarding a length computation, well, 0 is 0 in either event, so again no harm.

::: js/src/jsobj.cpp
@@ +3952,5 @@
>      // of bounds accesses.
>      if (obj->template is<TypedArrayObject>()) {
>          uint64_t index;
>          if (IsTypedArrayIndex(id, &index)) {
> +            if (index < obj->template as<TypedArrayObject>().zlength()) {

http://stackoverflow.com/questions/610245/where-and-why-do-i-have-to-put-the-template-and-typename-keywords

::: js/src/vm/SelfHosting.cpp
@@ +424,5 @@
>          uint32_t idx = args[idxi].toInt32();
>  
>          if (arrobj->is<TypedArrayObject>() || arrobj->is<TypedObject>()) {
> +            JS_ASSERT(!arrobj->is<TypedArrayObject>() || idx < arrobj->as<TypedArrayObject>().zlength());
> +            JS_ASSERT(!arrobj->is<TypedObject>() || idx < uint32_t(arrobj->as<TypedObject>().zlength()));

UnsafePutElements is used only in self-hosted code, and like it says on the box it's unsafe to use on an array (of any sort) that's been exposed to script and gone beyond the confines of self-hosted code.  (Had it done so, neutering, defining of non-configurable properties, and all sorts of other nonsense could have happened.)  I went back and double-checked all calls of it, and all of them are acting upon a fresh NewDenseArray or typed array that hasn't been exposed to script yet.

So, this should be fine as well.  Fragile, sure.  But it says that on the box already.
Attachment #8422655 - Flags: review- → review?(sphink)
Comment on attachment 8421278 [details] [diff] [review]
WebIDL 1 - Add mandatory ComputeLengthAndData, rename Length/Data

r=me on the assumption that we plan to backport this.

But please file a followup for a better way.  Doing a single GetLengthAndData() would be slightly better, handles even better.  Keeping the old data alive while we're on the stack + handles even better...
Attachment #8421278 - Flags: review?(bzbarsky) → review+
Attachment #8421300 - Flags: review?(bugs) → review+
Comment on attachment 8421291 [details] [diff] [review]
WebIDL 3 - Several safe places if various XPCOM contractids aren't overridden

This seems fine, yes.
Attachment #8421291 - Flags: review?(bzbarsky) → review+
Attachment #8421309 - Flags: review?(bugs) → review+
Attachment #8421349 - Flags: review?(bugs) → review+
Comment on attachment 8420422 [details] [diff] [review]
Move the is-neutered check a little later in TypedObject constructor code

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

I have exactly this change in a branch-- although I moved the "callee->opaque()" test later too.

I guess the correct behavior will depend on precisely how the spec winds up being written.
Attachment #8420422 - Flags: review?(nmatsakis) → review+
Attachment #8422660 - Flags: review?(nmatsakis) → review+
Attachment #8422819 - Flags: review?(nmatsakis) → review+
Comment on attachment 8422653 [details] [diff] [review]
JS 11 - TypedArray.h renames

r=me
Attachment #8422653 - Flags: review?(bzbarsky) → review+
Attachment #8422814 - Flags: review?(nmatsakis) → review+
(Assignee)

Comment 143

5 years ago
Comment on attachment 8420435 [details] [diff] [review]
Make nsBinaryStream::ReadArrayBuffer deal with the passed-in buffer being neutered mid-read

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

I really kind of hate to keep piling on you (and I've been trying pretty hard not to).  But realistically intersection of streams-knowledge and JSAPI-knowledge is a fairly small set, and we're starting to get lower on time this cycle.  So, bleh.

See comment 53 for details on this.  There's a JS_IsNeuteredArrayBuffer now on trunk, but I'm somewhat loathe to make that additional change and have to propagate it through the rest of my patch series.
Attachment #8420435 - Flags: review?(bzbarsky)
(Assignee)

Comment 144

5 years ago
Comment on attachment 8421303 [details] [diff] [review]
WebIDL 6 - A non-obvious case

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

See comment 66 for background on this.
Attachment #8421303 - Flags: review?(bzbarsky)
Comment on attachment 8422843 [details] [diff] [review]
JS 17 - Places where data is immediately copied after data/length lookup, fragile bits never used after that

r=me
Attachment #8422843 - Flags: review?(bzbarsky) → review+
Comment on attachment 8422861 [details] [diff] [review]
JS 22 - Miscellanea

r=me
Attachment #8422861 - Flags: review?(bzbarsky) → review+
Comment on attachment 8421303 [details] [diff] [review]
WebIDL 6 - A non-obvious case

r=me
Attachment #8421303 - Flags: review?(bzbarsky) → review+
Group: core-security
(Assignee)

Comment 148

5 years ago
(In reply to Olli Pettay [:smaug] from comment #79)
> (But it is not quite clear to me why we want ComputeLengthAndData() + Data()
> + Length()
> and why not just GetLengthAndData)

Mostly that was how it was done before, and it seemed best to touch as little code as possible given the scale of this.  In hindsight that's conceivably not the right analysis, given that you have to know those locations to determine whether CLAD was called in the right place or not.  Oh well.  For the followup bug.
Comment on attachment 8422825 [details] [diff] [review]
JS 14 - TypedObject renames

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

I believe your claims about the enumerate hook. jsiter.cpp seems to just call the hook in a tight loop without possiblility of other JS running. Even scripted proxies won't do it, because they can't get between, either.
Attachment #8422825 - Flags: review?(efaustbmo) → review+
Comment on attachment 8422544 [details] [diff] [review]
JS 6 - CTypes

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

::: js/src/ctypes/CTypes.cpp
@@ +2417,3 @@
>        if (!p)
>            return false;
>        *static_cast<void**>(buffer) = p;

Of course it's impossible to vouch for this, really, since its safety depends on js-ctypes *users* not to pass in a TypedArray that's going to be neutered somehow while this raw pointer is being used. ...I imagine it's OK.
Attachment #8422544 - Flags: review?(jorendorff) → review+
(In reply to Steve Fink [:sfink] from comment #128)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #95)
> > Created attachment 8422652 [details] [diff] [review]
> > JS 10 - Structured cloning
> > 
> > I am somewhat accepting on faith that structured cloning handles midflight
> > neutering attempts, because if we don't all this code is way unsafe.  Is
> > that the case?

Everything Steve said is right. The sequence of events under WriteStructuredClone is:

  - Figure out which transferables we're going to transfer
    (-> init -> parseTransferables);
    make a note of them and add them to the history
    (-> init -> writeTransferMap)
    but don't write down the arraybuffer length or buffer pointer yet

  - Serialize everything else, including, potentially:

      - writing one or more TypedArrays that point into that ArrayBuffer

      - neutering the ArrayBuffer

  - Transfer ownership of memory from transferables to the TransferMap
    which is just a part of the serialized data
    (-> write -> transferOwnership)
    This is where the length and buffer pointer are written.

The two potential events described in the middle there can happen in either order, of course. Both are safe in the code as written.

The only place I disagree with sfink is actually good news (well, if I'm right). I claim it doesn't matter that we don't write any properties of TypedArrays. It would still be safe.

writeArray() calls startWrite() for the ArrayBuffer, but in the case we're talking about, where that buffer is being transferred, startWrite() finds the ArrayBuffer in `memory`, and we get a backreference. In short, serialization will never touch that ArrayBuffer again until transferOwnership() is called at the very end.
Comment on attachment 8422652 [details] [diff] [review]
JS 10 - Structured cloning

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

Still have not actually done the audit here yet. Monday. Sorry :(
Comment on attachment 8422847 [details] [diff] [review]
JS 18 - Audit nsDOMFileReader

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

what does JS_ZGetArrayBufferData do? It's not in the tree and there's no comments in this bug about it.

Given that this seems like a tree-wide change, don't think this needs to be reviewed by a DOM peer. But feel free to re-ask for review if you want me to review it, but some explanation of what JS_ZGetArrayBufferData does would help.
Attachment #8422847 - Flags: review?(jonas)
Comment on attachment 8420435 [details] [diff] [review]
Make nsBinaryStream::ReadArrayBuffer deal with the passed-in buffer being neutered mid-read

nsAutoArrayPtr might be more local style than ScopedDeleteArray.

Apart from that, looks great.  r=me
Attachment #8420435 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 155

5 years ago
Comment on attachment 8422847 [details] [diff] [review]
JS 18 - Audit nsDOMFileReader

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

(In reply to Jonas Sicking (:sicking) from comment #153)
> Given that this seems like a tree-wide change, don't think this needs to be
> reviewed by a DOM peer. But feel free to re-ask for review if you want me to
> review it, but some explanation of what JS_ZGetArrayBufferData does would
> help.

Does comment 133 clarify things at all here, in concert with my original request in comment 113?  The point is to get a second set of eyes familiar with each bit of code in question, to double-check my auditing of all of it.  Domain-specific knowledge is more or less valuable for this purpose, depending on the complexity of the code.  (Plus it makes more people aware of this issue, which is going to be a persistent one for some time.  Even doing something handle-based won't solve it, not without awareness of what the handles are meant to defend against.)

To restate my argument for safety in more detail, which you are to double-check: it looks to me like nsDOMFileReader creates mArrayBuffer, fills it in, then once reading is finished exposes it to script.  It's never passed to anything that might neuter, until reading has finished.  Therefore, the data pointer computed here, into which Read(...) writes, will *never* go stale.

So no matter what hijinks |aInputStream->Read(...)| performs, they will never cause JS_GetArrayBufferData(mArrayBuffer) to ever evaluate to a pointer which becomes invalid while that single Read() call is in progress.

> what does JS_ZGetArrayBufferData do? It's not in the tree and there's no
> comments in this bug about it.

It's identical to the existing JS_GetArrayBufferData.  If comment 133 and this still aren't clear, poke me and I can attempt to explain again.  (I've been in this for so long I have extreme tunnel vision re just how clear any of this bug is to anyone not already intimately familiar with it.  :-\ )
Attachment #8422847 - Flags: review?(jonas)
(Assignee)

Comment 156

5 years ago
Trunk patchwork is in a holding pattern right now while I figure out how to run tests on it without spilling the beans too much.  Maybe some sort of push-a-merge-to-try shenanigans or somesuch.  Once I get that tested, and once I have all the reviews in here, I'll post some sort of rollup patch for sec-approval.

I have a backport to aurora of all patches related to this bug and all bugs spawned as a result of it.  It compiles, but I haven't tested anything yet.

I also have partial backporting completed to beta.  Everything up to the final rename-all-JS-methods-with-Z patchwork here very nearly compiles -- looks like one or two issues remaining to finish everything up to that point.  Importing the rename-js-to-z patch hits a solid screenful or two of patch failures that I'll have to sort through manually.

No backporting work started yet on on b2g26 or any other b2g branch, or upon esr24.
(Assignee)

Comment 157

5 years ago
I guess we removed this in aurora/trunk, but it's a thing on beta (and earlier) in need of adjustment.  HTMLAudioElement::MozWriteAudio converts the incoming raw data into a new AudioDataValue[], and that's just a bunch of math, so no reentry possibilities there.
Attachment #8424628 - Flags: review?(ehsan)
(Assignee)

Comment 158

5 years ago
This method appears, as the code reads, to do everything synchronously without reentry (excepting the omnipresent concern about someone overriding a critical XPCOM contract ID).  It squirrels away a length and raw pointer, but it fully consumes them before returning and letting the world go crazy.  So, no problems here.

This patch may somewhat conflict with bug 1007223 patchwork as far as backporting goes, depending on the extent of the backport wrt WebAudioDecodeJob changing interface.  I imagine either of us can adapt without too much trouble.
Attachment #8424636 - Flags: review?(ehsan)
Comment on attachment 8422652 [details] [diff] [review]
JS 10 - Structured cloning

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

It is certainly weird to trigger a runtime error on the deserialize end when a transferable buffer is neutered during serialization, after a view has been written. But then it's a weird case.
Attachment #8422652 - Flags: review?(jorendorff) → review+
Comment on attachment 8422825 [details] [diff] [review]
JS 14 - TypedObject renames

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

::: js/src/builtin/TypedObject.cpp
@@ +2510,5 @@
>          int32_t maximumLength = bytesRemaining / elemSize;
>  
>          int32_t length;
>          if (args.length() >= 3 && !args[2].isUndefined()) {
>              if (!args[2].isInt32()) {

This area looks ok, but only because we call isInt32() rather than toInt(). But we'll just have to be careful when we fix this, I think.
Attachment #8422825 - Flags: review?(nmatsakis)
Attachment #8422825 - Flags: review?(efaustbmo)
Attachment #8422825 - Flags: review+
Attachment #8422817 - Flags: review?(nmatsakis) → review+
Attachment #8422818 - Flags: review?(nmatsakis) → review+
(Assignee)

Comment 161

5 years ago
Comment on attachment 8422825 [details] [diff] [review]
JS 14 - TypedObject renames

(resetting efaust's flag to r+)
Attachment #8422825 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 162

5 years ago
Comment on attachment 8421282 [details] [diff] [review]
WebIDL 2 - WebGL: make ComputeLengthAndData explicit, rename Length/Data

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

::: content/canvas/src/WebGLContextGL.cpp
@@ +3681,5 @@
> +    uint32_t length;
> +    int jsArrayType;
> +    if (pixels.IsNull()) {
> +        const ArrayBufferView& view = pixels.Value();
> +        view.ComputeLengthAndData();

This...is not quite right.  :-)  Fixed locally (subsequent Z-renaming too, for good measure, even if it's not going to land), flagged by a manual WebGL conformance smoketest test run prior to posting an aurora patch for fuzzification.
(Assignee)

Comment 163

5 years ago
Here's an aurora rollup patch for fuzzing.  It's not what's going to land -- it has all the added tests in it, for one -- but it's something that can be banged on.
Attachment #8425192 - Flags: feedback?(gary)
(Assignee)

Comment 164

5 years ago
...and, finished up the beta backport.  More potential for fuzzing.

Aside from esr24, what specific branches do I also need to backport this to?  I have no idea these days what the set of b2g branches is, that are in active development wrt security fixes.
Attachment #8425194 - Flags: feedback?(gary)
Flags: needinfo?(dveditz)
Attachment #8425192 - Flags: feedback?(choller)
Attachment #8425194 - Flags: feedback?(choller)
Depends on: 1013031
status-b2g18: --- → wontfix
status-b2g-v1.1hd: --- → wontfix
status-b2g-v1.2: --- → affected
status-b2g-v1.3: --- → affected
status-b2g-v1.3T: --- → affected
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-firefox29: --- → wontfix
status-firefox30: --- → affected
status-firefox-esr24: --- → affected
(Assignee)

Comment 165

5 years ago
RyanVM tells me I want b2g26 and b2g28.  And if it were a chemspill situation, b2g18, but as far as we know this isn't being actively exploited, so that doesn't apply.  (Not that we have any real knowledge that it's not.  :-\ )
Flags: needinfo?(dveditz)
Comment on attachment 8422655 [details] [diff] [review]
JS 12 - Relatively simple/obvious renames in JS code

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

Ok, fair enough.
Attachment #8422655 - Flags: review?(sphink) → review+
Attachment #8423499 - Flags: review?(sphink) → review+

Comment 167

5 years ago
Comment on attachment 8424628 [details] [diff] [review]
Beta-targeted: Make computation of the length/data pointer in MozWriteAudio explicit

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

FWIW the moz audio data API which uses this is disabled on Beta and removed from newer versions as you noted.
Attachment #8424628 - Flags: review?(ehsan) → review+

Updated

5 years ago
Attachment #8424636 - Flags: review?(ehsan) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #151)
> The only place I disagree with sfink is actually good news (well, if I'm
> right). I claim it doesn't matter that we don't write any properties of
> TypedArrays. It would still be safe.
> 
> writeArray() calls startWrite() for the ArrayBuffer, but in the case we're
> talking about, where that buffer is being transferred, startWrite() finds
> the ArrayBuffer in `memory`, and we get a backreference. In short,
> serialization will never touch that ArrayBuffer again until
> transferOwnership() is called at the very end.

(None of this matters, since we can't hit the problematic part right now anyway. But to be complete:)

That wasn't the case I was thinking of. Pretend that we write out properties added to ArrayBuffers. (Currently, ABs are extensible, but structured clone ignores any added properties.) You clone a typed array view of an array buffer that has a sneaky property that goes back and neuters that array buffer. No transferables anywhere (at least in the outer clone.) Something like:

  B = new ArrayBuffer(100);
  A = new Int8Array(B, 20);
  Object.defineProperty(B, 'foo', { get: () => { neuter(B) } });
  serialize(A);

Now structured clone writes out the length of A (80) and then writes out B. Let's say the data gets inserted into the clone buffer before the extra properties are written. While writing B, it puts a complete copy of B's data into the clone buffer, then neuters B. Then to finish up writing A, it writes out the new offset (0, instead of the original 20). We now have a totally valid clone buffer, except it's going to get deserialized into an Int8Array with length 80 at offset 0 instead of the original length 80 offset 20.
(Assignee)

Comment 169

5 years ago
sfink pointed out a new es-discuss thread about ArrayBuffer neutering and its effect on other typed array semantics.

https://mail.mozilla.org/pipermail/es-discuss/2014-May/037214.html

The thread's about --><-- close to making the leap to finding this class of ES6 bugs, at which point this bug's existence (if not the exact set of places we think are broken) is public knowledge.

Hopefully this draft mail can head off unwitting comments from other vendors, to make this problem public knowledge.  Anyone with comments on wording, etc. feel free to comment on it.  Given the thread's new and active, I'd like to send this today to head off as much dangerous discussion as possible, so if you have suggestions to make, make them quickly.

(Note that plenty of non-browser people are reading that thread, occasionally even commenting.  If someone we don't have good relations with makes the leap quickly enough, well, having to chemspill is a real possibility.)
Attachment #8426609 - Flags: feedback?(sphink)
Comment on attachment 8426609 [details]
Draft mail to other vendors about this concern

Feedback sent via email.
Attachment #8426609 - Flags: feedback?(sphink)
Attachment #8425192 - Flags: feedback?(gary) → feedback+
Attachment #8425194 - Flags: feedback?(gary) → feedback+
The aurora patch doesn't compile for me:

/srv/repos/mozilla-aurora/js/src/jsapi-tests/testArrayBufferView.cpp: In member function ‘bool cls_testArrayBufferView_type::TestViewType(JSContext*)’:
/srv/repos/mozilla-aurora/js/src/jsapi-tests/testArrayBufferView.cpp:122:44: error: no matching function for call to ‘cls_testArrayBufferView_type::Create(JSContext*&)’
/srv/repos/mozilla-aurora/js/src/jsapi-tests/testArrayBufferView.cpp:122:44: note: candidate is:
/srv/repos/mozilla-aurora/js/src/jsapi-tests/testArrayBufferView.cpp:109:1: note: template<JSObject* (* CreateTypedArray)(JSContext*, unsigned int), long unsigned int Length> static JSObject* cls_testArrayBufferView_type::Create(JSContext*)


The beta patch doesn't seem to apply cleanly to tip, can you specify the revision I should use for testing?
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 172

5 years ago
Trunk patch tryserver run:

https://tbpl.mozilla.org/?tree=Try&rev=81356d3c1646

Once that smoketests, aurora/beta and perhaps b2g28 and b2g26 (backport just completed!) will follow after.  (Although of course few of the later ones are likely to build on try, but oh well.)  Also, I still need to (for non-trunk) do a run-through of the patches to verify each test busts without the corresponding patch, passes with it.  And I need to update the trunk patch for bug 995385 still, although there's a claim it's trivial, so.

I'm in the process of adjusting the aurora/beta patches to repost them.  The aurora issue looks like a compiler name-scoping bug, easily worked around.  (Wasn't biting me with a semi-recent clang locally.)  The beta thing I still need to look up.  Will do later in the evening.
(Assignee)

Comment 173

5 years ago
Turns out that ArrayBufferInputStream patch is just wrong.  I sort of misunderstood the meanings of the member fields -- mBufferLength isn't the length of the ArrayBuffer, it's the length of the data to write out.  Not too difficult to fix, just check for the ArrayBuffer having length zero with more data expected.
(Assignee)

Updated

5 years ago
Attachment #8410647 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8410672 - Attachment is obsolete: true
(Assignee)

Comment 174

5 years ago
Trunk patch rerun with that, and a jstest not marked as requiring neuter(), fixed:

https://tbpl.mozilla.org/?tree=Try&rev=9748a4967c1e
(Assignee)

Comment 175

5 years ago
Trunk try looks good so far, good enough to risk further exposure of branch-targeting of patches for an adversary watching the try firehose:

Aurora try run: https://tbpl.mozilla.org/?tree=Try&rev=0cf6f3f08024
Beta try run: https://tbpl.mozilla.org/?tree=Try&rev=f8020ba659e7
Quixotic b2g28 try run: https://tbpl.mozilla.org/?tree=Try&rev=cc550794d9a2
Just for giggles b2g26 try run: https://tbpl.mozilla.org/?tree=Try&rev=c8fcafad08cd

ESR24 backporting is in progress.  This is particularly fun because TypedArrayObject.* is not a thing on that branch, so every change to the JS engine's aspects of typed arrays/ArrayBuffers requires manual poting.  (Then again, ArrayBufferObject.* wasn't a thing in at least some of the backporting, so I had to merge those changes in by hand as well, but I managed, so this is really just more of the same.)

decoder, here are your aurora and beta rollups, assuming try doesn't end up rejecting them:

Aurora: https://hg.mozilla.org/try/raw-rev/0cf6f3f08024
Beta: https://hg.mozilla.org/try/raw-rev/f8020ba659e7

I can always upload the patches here if we want a permanent record, but I suspect it's probably not worth it for these only-transiently-important rollups.  When the fixes ultimately land, we'll have permanent records via the usual mozilla-central revisions.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 176

5 years ago
I plan to do the last bit of bug 995385-related fixing later this morning, at which point I'll post the trunk rollup patch for sec-approval.  I expect to land this patch, and patches for several other typed array bugs in my queue underneath all these patches (bug 1003997, bug 999651, bug 995679, bug 1009952, and bug 1011007), all at once -- those each as separate revisions, and this bug's patches as a single combined revision.  Or perhaps I may be paranoid enough to push a single revision for all of them.  Not sure quite yet.  But given all the fairly-obvious out-of-bounds considerations that a massive typed array patch like this must be contemplating, I really don't think we can be too paranoid about any of this, as far as watching adversaries go, and reducing window of exposure as far as possible.
(Assignee)

Comment 177

5 years ago
[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Pretty easily if they think a little.  But, hey, maybe wall-o-text will help confuse things.  Or something.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Patch doesn't contain tests.  Might be one or two comments that could raise hackles, mostly I tried to avoid adding such comments, tho.  Checkin comment will be bland, except for the swath of reviewers in it.  :-)  (Not adding til I'm ready to push, because it's mostly wasted effort creating a rollup now that won't actually be what lands, given congestion.  And it's useful to preserve the patch splits as long as possible.)

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
Everything.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Mostly I do, now.  Still working on b2g28/b2g26 (...I think, not entirely sure) and esr24.  But the end is well in sight.

> How likely is this patch to cause regressions; how much testing does it need?
I wrote it once, I reviewed it before posting and found more issues, everyone in the world reviewed everything else, hopefully it's scrutinized enough now.  But honestly we could easily have missed something still.  I'm not so worried about regressions, as about incompleteness.  I don't think real-world testing is going to be much help at all with sussing out any issues in this, or still in it after all this time.
Attachment #8428125 - Flags: sec-approval?
Attachment #8428125 - Flags: review+
(Assignee)

Comment 178

5 years ago
By "Mostly I do, now" I meant I have aurora/beta done (or a try-run away from verification), and the b2gs are about the same except with less certainty about try run success, and esr24 is in progress.  Just to clarify.
Comment on attachment 8428125 [details] [diff] [review]
Trunk rollup patch (atop various other typed array bugs as noted before)

sec-approval+ (oh my) for trunk.

If we want this on Beta and Aurora, and we do, we should get those patches nominated as soon as this is green on trunk so it makes the Beta beta (hmm) next week.
Attachment #8428125 - Flags: sec-approval? → sec-approval+
tracking-firefox30: --- → +
tracking-firefox32: --- → +
Depends on: 1015919
Comment on attachment 8425192 [details] [diff] [review]
Aurora rollup patch for fuzzing

Nothing bad found after 24 hours of testing.
Attachment #8425192 - Flags: feedback?(choller) → feedback+
Comment on attachment 8425194 [details] [diff] [review]
Beta rollup for fuzzing

Nothing here either :)
Attachment #8425194 - Flags: feedback?(choller) → feedback+
(Assignee)

Comment 182

5 years ago
Rollup patch (with a bunch of related typed array bugs folded in, for mild obfuscatory purposes) landed:

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

Backports in progress, hoping to have aurora/beta by EOD.
(Assignee)

Comment 183

5 years ago
Backed out, then relanded with CLOBBER:

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

Comment 184

5 years ago
Posted patch Aurora rollup patch (obsolete) — Splinter Review
Once this gets approval, I'd rather not have help landing this.  It's pretty fragile, and I don't want to bust the separated patches I'm maintaining locally, if I lose a push race or similar.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dates back to introduction of neutering, ish, so ~forever
User impact if declined: sec-critical
Testing completed (on m-c, etc.): landed on m-c, lotsa tests written and run against it (but won't land with this patch, note)
Risk to taking this patch (and alternatives if risky): I bust something in the process of fixing?  that or I miss something, or the risk assessment on trunk is different from that on various branches
String or IDL/UUID changes made by this patch: N/A
Attachment #8429702 - Flags: review+
Attachment #8429702 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 185

5 years ago
Actually, I probably should keep this bug's changes in one patch, and get approvals of the other changes in the other bugs.  Approval comments on previous patch apply verbatim here.
Attachment #8429704 - Flags: review+
Attachment #8429704 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #8429702 - Attachment is obsolete: true
Attachment #8429702 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 186

5 years ago
[Approval Request Comment]
All the same as for the aurora backport.

Again, please leave landing to me.
Attachment #8429893 - Flags: review+
Attachment #8429893 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/57b0932e2f06
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-b2g-v2.0: affected → fixed
status-firefox32: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
tracking-firefox-esr24: + → 30+
Comment on attachment 8429704 [details] [diff] [review]
Aurora patch with just this bug's changes, and no others, in it

Given the fuzzing hasn't turned up issues at this point, we'll take the uplift.  Is there anything else that can be done for verification/peace of mind before we ship?
Attachment #8429704 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #186)
> Created attachment 8429893 [details] [diff] [review]
> Beta backport of only this bug's patches
> 
> [Approval Request Comment]
> All the same as for the aurora backport.
> 
> Again, please leave landing to me.

See ni? in comment 188 - this is arriving pretty late in the FF30 cycle.  What would regressions to this work manifest as?  What, if any, user impact is there to waiting one more cycle to land this much change?
(Assignee)

Comment 190

5 years ago
(In reply to Lukas Blakk [:lsblakk] from comment #188)
> Is there anything else that can be done for verification/peace of
> mind before we ship?

Not really.

(In reply to Lukas Blakk [:lsblakk] from comment #189)
> See ni? in comment 188 - this is arriving pretty late in the FF30 cycle.

Hey, I wanted this done *last* cycle, but it was all detail/audit-oriented enough it didn't happen.  :-(  I don't think we ever had a choice about when to land this in the cycle.  It's too blatant an issue to allow the window of public visibility be any wider than it must be.  Heck, I didn't even start tryservering any of this til about a week ago, because of that same paranoia.

> What would regressions to this work manifest as?

Most of the changes are simple enough to not have much regression potential.  Add an extra arithmetic check here, and bail immediately if it fails -- strictly narrowing behavior.  The big concern would be that somehow the audit missed something.  But not landing any changes at all, clearly does nothing at all to address that concern.

> What, if any, user impact is there to waiting one more cycle to land
> this much change?

Well, it landed on trunk and pretty clearly has security implications, and someone slightly familiar with typed arrays could probably recognize the implications pretty easily, and probably (with some reliance on memory allocation behavior, but that's controllable) exploit them from there.  That can't be good, right?

I have b2g28/b2g26 patches at the point of being as done as I think they can be without an actual green landing.  esr24 backporting is almost done, just in the final phases of Z-renaming to verify I've found and audited/fixed everything that needs it.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 191

5 years ago
These two methods appear not to exist in any of b2g26/b2g28/beta/aurora/trunk.  They look safe to me -- these methods pass along raw pointer and length into methods that ultimately *copy* that data/length.  See:

https://hg.mozilla.org/releases/mozilla-esr24/annotate/default/content/canvas/src/WebGLBuffer.cpp
https://hg.mozilla.org/releases/mozilla-esr24/annotate/default/content/canvas/src/WebGLBuffer.h
https://hg.mozilla.org/releases/mozilla-esr24/annotate/default/content/canvas/src/WebGLElementArrayCache.cpp
https://hg.mozilla.org/releases/mozilla-esr24/annotate/default/content/canvas/src/WebGLElementArrayCache.h

which ultimately bottom out in WebGLElementArrayCache::BufferData and WebGLElementArrayCache::BufferSubData.

But please double-check my logic.  :-)  Again, as before, the Z-renaming isn't part of what will land -- it's just to point out all the places that require ComputeLengthAndData, and where the length/data that are computed will be used, and when.

This appears to be the only particular significant difference between esr24 and newer branches, in terms of the TypedArray struct usage.  I still need to backport the Z-renaming of all the JS methods, to be sure I've covered all the bases before I have a complete esr24 backport ready.
Attachment #8430293 - Flags: review?(jgilbert)
Attachment #8430293 - Flags: review?(jgilbert) → review+
(Assignee)

Comment 192

5 years ago
Woo, esr24 backport is done.  Haven't verified tests fail, then pass with appropriate patch applied, but other than that things are good.  b2g patches and esr24 backports to be posted tomorrow, and aurora/beta patches to be pushed probably tomorrow afternoon or evening.
Attachment #8429893 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 193

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

b2g/esr24 backports shortly.
status-firefox30: affected → fixed
status-firefox31: affected → fixed
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4fd82c282519
status-b2g-v1.4: affected → fixed
Flags: in-testsuite?
status-b2g-v1.3T: affected → fixed
(Assignee)

Comment 196

5 years ago
So.  It happens that there's no attempt made to handle neutering a DataView on ESR24 -- we just fall into typed array neutering code.  That quickly asserts.  (And it caused probably both of the DataView tests I had to fail without my patches, *and* with them, hence how I found this.)

The consequences?  DataView and typed arrays largely share structure and slot layout, so the byte-length and byte-offset slot writes are fine.  The setPrivate(NULL) is fine, because both use the private slot, and setPrivate will properly refer to the actual object class to determine where to store the NULL.

The only tricky bit is the undesired write to LENGTH_SLOT for DataViews.  DataView::RESERVED_SLOTS is BufferView::NUM_SLOTS is 5.  That rounds up to the size class 8.  LENGTH_SLOT is NUM_SLOTS is 5.  So the write will go to a fixed slot.  It doesn't appear to me that that fixed slot can be used any other way -- added properties will use non-reserved slots, the private slot is one after the last fixed slot (of which there are 7, for a class that has a private) which won't ever be 5 (it'll be 7 here).  So I *think* this is just an assertion failure, coupled with some dodgy punning that happens not to be a critical issue.  But please check me on this!

It appears serialize() in 24 hasn't been updated to handle transfers, so it's not possible to write a test for this, unless it's a browser-based test.  Probably not worth the trouble for an apparent assertion-only bug.
Attachment #8432038 - Flags: review?(sphink)
(Assignee)

Comment 197

5 years ago
I'm testing the esr24 backport patches now (as comment 196 may have suggested).

Some of the tests aren't effective against esr24 because of esr24 missing stuff.  For example, the tests for bug 999651 and bug 995679 don't work against esr24, because esr24 appears to treat objects as constants less readily than even b2g26 does.  It's not clear to me how to fix that test, because I don't know enough about how we decide to treat an object as a constant on esr24 (or even in newer branches, I'm just trying stuff and it happens to be constant there :-) ).  In that case, the code change being tested is simple enough (removal of some fast-paths in favor of falling back on obviously correct code) that I'm not too worried about lack of test.

Others, like for bug 1009952, required a little tweaking to still invoke the same paths, but weren't too bad.  (Who'd have thought |ta[0]| would *not* invoke an optimization but only |var index = 0; ta[index >> 0]| would?)

Still working through the rest of it past the DataView patches, things looking pretty good, tho, so I expect to have approval requests for ESR24 going pretty shortly.
(Assignee)

Comment 198

5 years ago
[Approval Request Comment]
User impact if declined: sec-critical
Fix Landed on Version: everything newer than ESR24, at this point, 32 (?) originally
Risk to taking this patch (and alternatives if risky): again, might have missed something, but that's far preferable to not landing this at all, given it's on trunk and everywhere else now and clearly is a security fix, and with a little reading it's clear how to attack it
String or UUID changes made by this patch: N/A

As usual for all this, no help wanted landing this or any of the other typed array backports.

Unlike aurora/beta or even the b2gs, I haven't try-servered this patch, because I assume given age there's no possible way I'll get meaningful data back.  Will just have to watch and be careful.  Given lack of surprises in previous landings, excepting the weird build bug that hit the trunk landing, I think we're in reasonable shape for this.  And as always, I'll keep an eye on the tree.
Attachment #8432047 - Flags: review+
Attachment #8432047 - Flags: approval-mozilla-esr24?
Comment on attachment 8432038 [details] [diff] [review]
ESR24: Properly neuter DataView objects

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

Well, at least that seems safe.
Attachment #8432038 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #199)
> Comment on attachment 8432038 [details] [diff] [review]
> ESR24: Properly neuter DataView objects
> 
> Review of attachment 8432038 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Well, at least that seems safe.

(And I believe the logic in comment 196 is sound.)
Comment on attachment 8432047 [details] [diff] [review]
ESR24 backport

ESR24 approval granted.
Attachment #8432047 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
(Assignee)

Comment 202

5 years ago
https://hg.mozilla.org/releases/mozilla-esr24/rev/8c406adf76a0
status-firefox-esr24: affected → fixed
(Assignee)

Comment 203

5 years ago
In b2g26 generated code for, say, XHR.send is so:

          RootedTypedArray<ArrayBuffer > arg0(cx);
          if (!arg0.Init(&args[0].toObject())) {
            break;
          }

          ErrorResult rv;
          self->Send(Constify(arg0), rv);
          rv.WouldReportJSException();
          if (rv.Failed()) {
            return ThrowMethodFailedWithDetails(cx, rv, "XMLHttpRequest", "send");
          }
          args.rval().set(JSVAL_VOID);
          return true;

In ESR24 it's so:

          NonNull<ArrayBuffer> arg0;
          Maybe<ArrayBuffer> arg0_holder;
          arg0_holder.construct(&args.handleAt(0).toObject());
          if (!arg0_holder.ref().inited()) {
            break;
          }
          arg0 = arg0_holder.addr();
          ErrorResult rv;
          self->Send(arg0, rv);
          rv.WouldReportJSException();
          if (rv.Failed()) {
            return ThrowMethodFailedWithDetails<true>(cx, rv, "XMLHttpRequest", "send");
          }
          args.rval().set(JSVAL_VOID);
          return true;

The key difference is in ESR24, we don't call Init() after construction.  And the TypedArray ctor is just dumb (I guess we're not calling it, ever, without an Init after, in b2g26 or later!), and puts the obj into mObj.  Hilarity ensues when, say, an XMLDocument object appears where a typed array was supposedly expected.

This just makes the ctor call Init with the provided object.  Seems to do the trick as far as the test failures from tbpl I could run locally were concerned.  We can fix the ctor/Init thing on trunk later.
Attachment #8432780 - Flags: review?(bzbarsky)
Comment on attachment 8432780 [details] [diff] [review]
Init TypedArray structs from WebIDL correctly in ESR24

r=me
Attachment #8432780 - Flags: review?(bzbarsky) → review+
Group: javascript-core-security
(Assignee)

Comment 206

5 years ago
https://hg.mozilla.org/releases/mozilla-esr24/rev/5917a170c6bd
https://hg.mozilla.org/releases/mozilla-esr24/rev/f2e036f80683

And with that, hopefully we're done here.  Heading home for dinner, watching the tree from there...
> I guess we're not calling it, ever, without an Init after, in b2g26 or later!

Seems plausible: that ctor is there for use from C++ only, not from bindings.  Please do make sure that on trunk we either remove that ctor or make it call Init or something.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 208

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #207)
> Please do make sure that on trunk we either remove that ctor or
> make it call Init or something.

Bug 1019334.
Flags: needinfo?(jwalden+bmo)
Based on comment 190, it appears that there is not much verification that can be done outside of what we've already done. Marking [qa-].
Whiteboard: [reviewers: read comment 133] → [reviewers: read comment 133][qa-]
(Assignee)

Updated

5 years ago
Depends on: 1020034
Whiteboard: [reviewers: read comment 133][qa-] → [reviewers: read comment 133][qa-][adv-main30+][adv-esr24.6+]
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.
status-seamonkey2.26: --- → wontfix
Waldo, those tests are failing for me in the shell:

    ecma_6/extensions/ArrayBuffer-slice-arguments-neutering.js
    ecma_6/extensions/DataView-construct-arguments-neutering.js
    ecma_6/extensions/DataView-set-arguments-neutering.js
    ecma_6/extensions/TypedArray-set-object-funky-length-neuters.js
    ecma_6/extensions/TypedArray-subarray-arguments-neutering.js

## ecma_6/extensions/TypedArray-subarray-arguments-neutering.js: rc = 4, run time = 0.037687
dbg64opt/js/src/js -f shell.js -f ecma_6/shell.js -f ecma_6/extensions/shell.js -f ecma_6/extensions/TypedArray-subarray-arguments-neutering.js
can't open ecma_6/extensions/shell.js: No such file or directory
REGRESSION - ecma_6/extensions/TypedArray-subarray-arguments-neutering.js
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 213

5 years ago
Apparently js/src/tests/ecma_6/extensions/{shell,browser}.js are in my tree but aren't in the patches I landed.  :-(  jorendorff also pointed this out earlier today, and I landed the stupid addition of both (empty) files, so things should be good again.

("It worked for me locally...")
Flags: needinfo?(jwalden+bmo)
Group: core-security
You need to log in before you can comment on or make changes to this bug.