It acts upon mObj, then it writes the new wrapped value into mObj. But ComputeLengthAndData acts upon mObj, *after* that wrapping has happened, in the case of methods called via xrays. Apparently we have no tests at all that exercise a WebIDL-implemented method, on an xray-wrapped object, that passes it a typed array or ArrayBuffer. Joy. Madness. Inconceivable.

Comment on attachment 8433769 [details] [diff] [review] Possible patch (requires patch from bug 1019334 first) lgarner, does this patch fix things for you? I can almost guarantee it should, in concert with the patch in bug 1018068 and the patch in bug 1019334.

> Apparently we have no tests at all that exercise a WebIDL-implemented method, on an > xray-wrapped object, that passes it a typed array or ArrayBuffer. It'd have to be a WebIDL constructor, not just any method. And it would have to be passing a typed array that's from the chrome scope, as opposed to one from the content scope. I'm not actually sure what semantics we want here. We can use the attached patch to allow the callee to extract the data, but if they want to hold on to the object itself they end up exposing a COW to the web page. So I would argue that the real bug here is the chrome code passing a chrome typed array object to a content ctor.

(In reply to Boris Zbarsky [:bz] from comment #3) > I'm not actually sure what semantics we want here. We can use the attached > patch to allow the callee to extract the data, but if they want to hold on > to the object itself they end up exposing a COW to the web page. So I would > argue that the real bug here is the chrome code passing a chrome typed array > object to a content ctor. Agreed. How should we fix it? Make it implicitly clone when calling over Xrays? Make it throw, and force the caller to explicitly pass the result of Cu.cloneInto()? The latter sounds better, and it matches the semantics I'm implementing for Xray TypedArrays (where indexed access just throws because it'd be slow, and the message tells the caller to use Cu.cloneInto).

What behavior do we end up with here for WebIDL objects? I think we just go ahead and use them as-is, right? Maybe what we should do is instead of doing the argument conversions for a ctor in the caller compartment and then entering the compartment we'll construct in we should just enter the latter immediately, wrap all the args into it, then start the arg conversions. This ought to automagically give us the behavior we want, I'd think.

(In reply to Boris Zbarsky [:bz] from comment #5) > What behavior do we end up with here for WebIDL objects? I think we just go > ahead and use them as-is, right? > > Maybe what we should do is instead of doing the argument conversions for a > ctor in the caller compartment and then entering the compartment we'll > construct in we should just enter the latter immediately, wrap all the args > into it, then start the arg conversions. This ought to automagically give > us the behavior we want, I'd think. That would put us right back where we started in bug 861493, right?

I think the best thing to do is for the C++ TypedArray helper to handle this. When it's first constructed, it makes sure that it can CheckedUnwrap the buffer, and throws if it can't. When it's asked to spit a JSObject back out, it checks whether the new compartment is same-origin with the old one, and, if it isn't, creates a clone.

> That would put us right back where we started in bug 861493, right? Hrmph. Yes. :( > it makes sure that it can CheckedUnwrap the buffer It does that already. > When it's asked to spit a JSObject back out, it checks whether the new compartment is > same-origin with the old one Hmm. I guess this in addition to waldo's patch?

But also, now we have to worry about Obj() OOMing, or just inadvertently creating a large allocation (e.g. webgl randomly calls Obj() in some cases, though I expect webgl from chrome to a content canvas to be rare).

(In reply to Boris Zbarsky [:bz] from comment #9) > But also, now we have to worry about Obj() OOMing, or just inadvertently > creating a large allocation (e.g. webgl randomly calls Obj() in some cases, > though I expect webgl from chrome to a content canvas to be rare). I would also be fine with making it throw.

Sounds bad. Please adjust if needed.

It looks like only this MozNDEFRecord constructor, and the ImageData constructor, are affected by this. (Going by a skim of the trunk patch. I don't remember having seen any other constructors affected, so this result comports with my pre-skim understandings, as a sort-of double-check.) If it were only the first, I wouldn't be worried about branches or anything along those lines. But the second is somewhat worrisome. Granted, it seems to require the help of chrome code to make bad stuff happen. But it's plausible. So, I don't know where this leaves us as far as branches go, and landing this on them. And particularly, about what to do for esr24 and the release about to happen in a week. Guess we should act as fast as possible in getting patches ready in case we think respinning (ugh) is warranted. As soon as we figure out what the patch actually should be, at least.

> And particularly, about what to do for esr24 The ImageData ctor was added in Fx29. I have no idea about MozNDEFRecord, since it was landed in a non-core bug which therefore doesn't have the right target milestone flags. :( So we don't need to do anything for 24.

Comment on attachment 8433769 [details] [diff] [review] Possible patch (requires patch from bug 1019334 first) I applied prerequisites {1018068, 1019334}, and there is no assertion failure anymore in Debug builds when ComputeLengthAndData() is called. F/MOZ_Assert( 1000): Assertion failure: GetObjectClass(obj) == detail::Uint8ArrayClassPtr, at ../../dist/include/jsfriendapi.h:1360 When this is done chrome side to pass to NFC application: rec = new this._window.MozNDEFRecord(rec.tnf, rec.type, rec.id, rec.payload); The last 3 arguments are Uint8Arrays (TypedArray). The patch appears to fix the development code I am working on.

OK, thinking about this some more, I like this option from comment 4: Make it throw, and force the caller to explicitly pass the result of Cu.cloneInto() I think the simplest wait to implement that would be to just make TypedArray_base::WrapIntoNewCompartment throw. Possibly only if it can't CheckedUnwrap after wrapping, but I expect that's always the case for Xrays anyway.

(In reply to Boris Zbarsky [:bz] from comment #15) > I think the simplest wait to implement that would be to just make > TypedArray_base::WrapIntoNewCompartment throw. Possibly only if it can't > CheckedUnwrap after wrapping, but I expect that's always the case for Xrays > anyway. CheckedUnwrap will succeed for a chrome->content Xray.

> CheckedUnwrap will succeed for a chrome->content Xray. I said _after_ wrapping. So after we've entered the callee compartment and wrapped everything into it. At that point for a chrome->content Xray we'll have a COW for the typed array object. But it's simpler to just throw from TypedArray_base::WrapIntoNewCompartment no matter what and fix consumers.

Comment on attachment 8433769 [details] [diff] [review] Possible patch (requires patch from bug 1019334 first) r=me because this restores the old behavior we had, but let's file a followup to make this all go away per comment 15.

Comment on attachment 8433769 [details] [diff] [review] Possible patch (requires patch from bug 1019334 first) [Security approval request comment] > How easily could an exploit be constructed based on the patch? Requires cooperation from chrome code or an addon passing a chrome typed array into a content DOM method (and a constructor, at that -- this seems to be only ImageData and MozNDEFRecord -- the former, nobody's likely to do, the latter is b2g so much less important), so very not so. (We decided not to rush on a complete fix before the recent releases, because of this difficulty.) > Do comments in the patch, the check-in comment, or tests included > in the patch paint a bulls-eye on the security problem? Not really. You'd have to understand bindings code and various contortions to even get the code to be hit. And then you need chrome code as accomplice. And you're going to have some trouble getting the chrome code to do just the right thing. and all. Seems pretty improbable. > Which older supported branches are affected by this flaw? All of them to an extent, but the chrome requirement makes it pretty much ignorable on branches. Except to the extent bug 1021180 or so might require it. > If not all supported branches, which bug introduced the flaw? Bug 991981. > Do you have backports for the affected branches? If not, how > different, hard to create, and risky will they be? Haven't bothered, we can let this slide on branches. But it *does* affect everything, so I guess s-a still needed. > How likely is this patch to cause regressions; how much > testing does it need? Unlikely. The fix is slightly heavyweight duplication of the object field in the struct, but there are few enough users, each wanting exactly one sense of this, that it's pretty clear and relatively obviously correct in what it promises.

Comment on attachment 8433769 [details] [diff] [review] Possible patch (requires patch from bug 1019334 first) Sec-approval+ for trunk. It isn't really clear to me whether you're suggesting taking this on Aurora and Beta or not. You seem to hedge... :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e024afb9f1d9 The question of whether aurora/beta need it is mostly a question of whether the current MozNDEFRecord uses in b2g stuff are a concern there. If they are, we'll need this there. (Either that, or the MozNDEFRecord uses will need to be changed using the Cu.cloneInto() thing.) If they aren't, we can leave those branches alone.

MozNDEFRecord is sort of android only currently, and only if compiled in. The MozNDEFRecord usage landing isn't imminent (yet). However, I was attempting to follow how the version of NFC in W3C was delivering some data objects via event callback parameter lists. ontagfound = function (jsObj_With_array_of_MozNDEFRec){} Is an explicit clone now suggested after these fixes?

(In reply to Garner Lee from comment #22) > MozNDEFRecord is sort of android only currently, and only if compiled in. --> "android" AOSP base as defined by the config file.

Landed on b2g32 with Waldo's blessing because it was blocking bug 1019668. https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e21a5384b520

Gaaah. We should have taken this on ESR31.

Marking [qe-verify-] in absence of test or STR. If that changes, please let me know and I'll test this.

(In reply to Al Billings [:abillings PTO 10/23-11/7] from comment #27) > Gaaah. We should have taken this on ESR31. Can we take this for our next ESR31 release?

Yes, we can. We should get an ESR31 patch ASAP made and nominated.

Does the current patch work on the ESR31 branch or will it require some work to back-port?

Comment 26's patch doesn't apply to 31 for me locally. The backport seems straightforward enough, but frankly my memory of the typed/wrapped split that was introduced there isn't good, so I hesitate to just throw up that patch and be happy with it. Building locally, probably need to page the entire patch back into memory again. :-( Should be done soon (today), tho, with a backport patch.

I am not particularly convinced we need this on 31. The reason it wasn't really backported, which may or may not be clear in comments (well, it is to me if I reread them, but no one else inhabits my brain, so...), is that this only bites MozNDEFRecord (b2g-only) and ImageData, created in chrome code, passed (in that chrome code) a typed array created within a content page. That level of interaction is unlikely to happen except deliberately, and it can't be triggered by a malicious site. And if it's done, it's somewhat dubious that the content object has enough control over itself to manage to do anything but crash rather quickly. Still, a backport is doable if really desired, it's just that nearly all details of this are out of my mind now, and as I stare at this backport-patch I somewhat worry about getting some detail wrong along the way. (And we don't have an automated test I could lean on for this, because of its being moderately esoteric and all. And constructing one now is not really that different from re-grokking the code again.)

Following Jeff's comments in comment 33, I don't think we should take this in ESR31. I'm marking this as "won't fix" for that.