Closed Bug 1003997 Opened 10 years ago Closed 10 years ago

JSObject::is<ArrayBufferViewObject>() and JS_IsArrayBufferViewObject are inconsistent with JS_GetObjectAsArrayBufferView/JS_GetArrayBufferViewType/JS_GetArrayBufferViewData/JS_GetArrayBufferViewBuffer/JS_GetArrayBufferViewByteLength

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox30 --- unaffected
firefox31 --- unaffected
firefox32 + verified
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- fixed

People

(Reporter: Waldo, Assigned: Waldo)

Details

(Keywords: regression, sec-high)

Attachments

(3 files, 1 obsolete file)

The former functions treat TypedObjects as ArrayBufferViews.  The latter functions do not.

But!  It looks like DataViews and typed arrays store byte length in the same slot, and the data pointer in getPrivate(), and the owner/underlying buffer in the same slot.  By inspection, I *think* typed objects store byte length in the same slot, and the data pointer is again in getPrivate().  So JS_GetObjectAsArrayBufferView, JS_GetArrayBufferViewData, JS_GetArrayBufferViewBuffer, and JS_GetArrayBufferViewByteLength by dumb luck should manage to have the right behavior, modulo assertions in as<TypedArrayObject>() or as<DataViewObject>().  If I'm reading the code right.  (Which is not necessarily guaranteed, and which someone else should verify as well.  If these all pun like this, we should have moved them all into ArrayBufferViewObject.  But, note that getPrivate() in that case would become "tricky" because it would consult a class pointer that I believe can encode different reserved-slot counts, so any use of getPrivate(LAST_SLOT) as a mini-optimization would have to be carefully removed in a move into ABVO.)

That leaves only JS_GetArrayBufferViewType:

JS_FRIEND_API(JSArrayBufferViewType)
JS_GetArrayBufferViewType(JSObject *obj)
{
    obj = CheckedUnwrap(obj);
    if (!obj)
        return ArrayBufferView::TYPE_MAX;

    if (obj->is<TypedArrayObject>())
        return static_cast<JSArrayBufferViewType>(obj->as<TypedArrayObject>().type());
    else if (obj->is<DataViewObject>())
        return ArrayBufferView::TYPE_DATAVIEW;
    MOZ_ASSUME_UNREACHABLE("invalid ArrayBufferView type");
}

Sigh.
Shu, TypedObjects are considered to be ArrayBufferViews by JS_IsArrayBufferViewObject right now.  This implies that JS_GetObjectAsArrayBufferView(JSObject*, uint32_t*, uint8_t**) has meaning for such objects.  That seems wrong to me.  Typed objects don't have a length, and they don't really have scalar array-ful data.  They do have a backing buffer and a byte offset, but those are the only real similarities.  Should we change this?  And if not (we probably can't change it on branches, at least, should we just ignore the byte length slot in typed objects and instead consult the owner for *its* byte length, for the purposes of that method?
Flags: needinfo?(shu)
That's a question more for Niko than me.
Flags: needinfo?(shu) → needinfo?(nmatsakis)
Niko has a patch that removes the redundant byteLength from typed arrays. (I remembered something like that, but assumed it had already landed.)

I believe the typed object spec wants typed objects to be ArrayBufferViews, though I don't have a pointer to it handy.

Why should ArrayBufferViews have scalar array-ful data? DataViews don't either. Assuming I'm understanding what you mean by "scalar array-ful"; Google translate couldn't handle it for me. Non-opaque typed objects do have a size, fwiw.

I believe it is intentional that a typed object can share backing data with DataViews and typed arrays. Heck, I guess that's how you'd do a reinterpret_cast.
Oh! Sorry, I ignored the previous sentence. *That's* what you meant by length and scalar array-ful data. Sorry.
I think the friend API will need to be updated to account for typed objects. Possibly the right behavior is just to have those functions return false for typed objects, since they share some of the properties of typed arrays but not all. (Perhaps they should also be renamed: I'm not sure how flexible we are with respect to that sort of thing.)

In particular:

1. Typed objects *do* have a size in bytes, which is basically the equivalent of byte length. I had a patch in bug 989276 to remove the BYTELENGTH slot. 

2. However, typed objects can also store references, and are not necessarily safe to interpret as just plain bytes.

3. Typed objects are views onto array buffers (and hence they extend ArrayBufferViewObject).
Flags: needinfo?(nmatsakis)
Attached patch ArrayBufferView JSAPI test (obsolete) — Splinter Review
Hmm.  Let's just make typed objects not observably ArrayBufferView objects.  The inheriting from ABVO bit is kind of messy/confusing, but I don't have better ideas.
Attachment #8418173 - Flags: review?(sphink)
Attachment #8418173 - Flags: review?(nmatsakis)
Attachment #8418174 - Flags: review?(nmatsakis)
What security rating should we give this, Jeff?  Is this an actual security problem?
So, conceivably you could fall off the end of the function, which would probably just quickly crash.  But it also seems reasonable that compilers could just eliminate the |else if (is a dataview)| check and just return the dataview type in that case.  Or perhaps invert the condition-checking, if the compiler's smart.  Either way, you'd get type-punning and probably according sec-critical badness.  Who knows.  In any case it sort of doesn't much matter, we should just fix this everywhere and not worry too much how it gets rated.
Keywords: sec-high
This seems pretty confusing to me. Typed objects *are* views of ArrayBuffers, so it kind of makes sense for them to be ArrayBufferViews. If the JSAPI is going to say they aren't, at least it needs prominent comments explaining why so people don't put it back that way.

But what if we left it and fixed things up?

JS_GetArrayBufferViewType doesn't make much sense even with just DataView. We could rename it to JS_GetTypedArrayElementType and make it fail if you pass in DataViews or typed objects.

Returning a pointer to the data for opaque typed objects makes me very nervous. I'd be tempted to make ABOV data accessors fail for opaque objects and add a separate JS_GetOpaqueTypedObjectData and friends instead. I think leaving the byte length accessor is ok, though; it can return the size of the typed object. (It has the same meaning: the length, in bytes, of the portion of the ArrayBuffer that the view is providing access to.)

I guess I'm saying that given that typed objects *do* have a byte length (aka size), they really are Views of ArrayBufferObjects and it's ok to treat them as such for anything but grabbing their data. That's special, because those data are just very different when they might contain GC pointers. You can't memcpy them, for example, without doing a bunch of manual fixup. In fact, maybe JS_GetOpaqueTypedObjectData should not refer to "data" at all. It's something like an immovable region that may contain GC heap pointers that you can't read or write without going through contortions. You can use the offset and byte length to figure out what portions of a buffer you have overlapping views for, say, but you can't take the pointed-at region out of an opaque typed object and construct a new buffer/view pair for it. Hm... it would be best if we just didn't have an API for accessing that.

I don't want to overengineer by creating an ABOV base class that doesn't expose the data and an ABOV subclass that does, but it's sort of what we're talking about here.
ArrayBufferView was a Khronos thing.  It's not in ES6 at all, so its status as a concept is kind of dubious, until TC39 says something.

ArrayBufferView in the Khronos spec is an interface exposing buffer, byteOffset, and byteLength properties.  It has nothing to do with the structural idea of an object that views an ArrayBuffer.  It's just a convenient fiction to add these properties to everything that wants them.

http://www.khronos.org/registry/typedarray/specs/latest/#6

Typed objects don't expose their underlying buffer to JS.  There's no .buffer property or anything like that, as far as I can tell, to expose a buffer.  The only way you can get one, is if you created a typed object with a provided buffer.  This seems suggestive to me, about what JSAPI should look like.

Typed objects also don't expose their byte length to JS.  Again, suggestive.

Finally, typed objects don't expose a byte offset.

Additionally, typed object data is weird.  Because typed objects can contain references per comment 6, exposing the raw data is kind of dangerous.  Even if exposed as const T*.

For all these reasons it seems to make sense to me to go with this patch.  To the extent DataView does or doesn't make sense here, we should defer that issue: DataViews expose all the above characteristics, and (unlike wrt typed objects) people may rely on this.  We need to go back to spec groups to get the status of ArrayBufferView as a concept solidified as part of ES6, if it is to keep living.  It seems best not to hold up a security fix for that inanity.
Comment on attachment 8418174 [details] [diff] [review]
Typed objects shouldn't appear to be ArrayBuffer views

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

::: js/src/vm/TypedArrayObject.h
@@ -367,5 @@
>  inline bool
>  JSObject::is<js::ArrayBufferViewObject>() const
>  {
> -    return is<js::DataViewObject>() || is<js::TypedArrayObject>() ||
> -           IsTypedObjectClass(getClass());

I'm not sure how I feel about this part of the patch. Perhaps what is needed is a new class. It seems to me that there are two notions:

1. Views onto an array buffer.
2. ArrayBufferView as specified in khronos spec

Typed objects are #1 but are probably not #2. Maybe we need a new base class to represent "things that view an array buffer and participate in the view list, can be neutered, and so forth".
Attachment #8418174 - Flags: review?(nmatsakis)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #11)
> ArrayBufferView was a Khronos thing.  It's not in ES6 at all, so its status
> as a concept is kind of dubious, until TC39 says something.

Oh, it never made it into ES6? Ok, that does change things.

> ArrayBufferView in the Khronos spec is an interface exposing buffer,
> byteOffset, and byteLength properties.  It has nothing to do with the
> structural idea of an object that views an ArrayBuffer.  It's just a
> convenient fiction to add these properties to everything that wants them.
> 
> http://www.khronos.org/registry/typedarray/specs/latest/#6
> 
> Typed objects don't expose their underlying buffer to JS.  There's no
> .buffer property or anything like that, as far as I can tell, to expose a
> buffer.  The only way you can get one, is if you created a typed object with
> a provided buffer.  This seems suggestive to me, about what JSAPI should
> look like.

Er, there was at some point. It was called .storage, I think?

http://wiki.ecmascript.org/doku.php?id=harmony:typed_objects had storage(obj).buffer, or just obj.buffer for array types. You can only get the buffer for non-opaque types, though.

nmatsakis says https://github.com/dslomov-chromium/typed-objects-es7 is "the most up to date bug also out of date". It seems kind of incomplete to me.

> Typed objects also don't expose their byte length to JS.  Again, suggestive.
> 
> Finally, typed objects don't expose a byte offset.

They do. byteOffset(), byteLength(), and buffer() (the last is for transparent types only), per nmatsakis on IRC.

> Additionally, typed object data is weird.  Because typed objects can contain
> references per comment 6, exposing the raw data is kind of dangerous.  Even
> if exposed as const T*.

I agree. I don't think we should provide an API to expose opaque typed objects' data. Forget about my JS_GetOpaqueTypedObjectData suggestion; I didn't like it much anyway.

> For all these reasons it seems to make sense to me to go with this patch. 
> To the extent DataView does or doesn't make sense here, we should defer that
> issue: DataViews expose all the above characteristics, and (unlike wrt typed
> objects) people may rely on this.  We need to go back to spec groups to get
> the status of ArrayBufferView as a concept solidified as part of ES6, if it
> is to keep living.  It seems best not to hold up a security fix for that
> inanity.

If the typed object spec hasn't been totally changed from what I linked above (which it possibly has; I can't keep up with it), then something internal *like* ArrayBufferView is a useful concept. If we want to disconnect it from the ill-defined ArrayBufferView by naming it something else, that's totally fine. We never did conform to the (webidl?) spec by making Uint8Array.prototype.prototype be an ArrayBufferView thing, so we are painted into no corners wrt JS-exposed stuff. But it seems like the spec is moving very much in the direction of having typed objects be views of ArrayBuffers like DataView and typed arrays, except that opaque typed objects do not expose the buffer.

I was looking at the current callers of JS_GetArrayBufferViewData, and I think they should or could all be JS_GetTypedArrayData (as in, some should be, and some would only lose unimportant functionality). So I don't think we need JS_GetArrayBufferViewData at all for now.
Collided with nmatsakis, but we seem to be saying the same thing.
Group: core-security
The spec questions really should be split off from this bug somehow. I'm kind of feeling like:

 - we should keep the ArrayBufferView concept, since as per bz's comment it's used frickin' everywhere

 - DataViews should continue to be ArrayBufferViews

 - transparent typed objects should also be ArrayBufferViews

 - opaque typed objects either should not be ArrayBufferViews, or every place that uses an ArrayBufferView has to be prepared for failure to access the buffer or buffer's data

Given the uncertainty involved with the latter two, I'm wondering if we could temporarily accept this patch as-is (making typed objects *not* be ArrayBufferViews), with the expectation that they would later become ABVs.

Then we'd also invent a new notion (ArrayBufferReference?) that is used internally for eg an ArrayBuffer's view list. I don't know if we're ok with an intermediate state where the external ArrayBufferView given by the explicit list in JS_IsArrayBufferViewObject differs from what inherits from ArrayBufferViewObject.

This waffling is easily externally visible, btw -- ES6 has ArrayBuffer.isView() and we return JS_IsArrayBufferViewObject() for it. (Not to mention that you currently cannot use a typed object blob.)
Attachment #8418173 - Flags: review?(sphink) → review+
Comment on attachment 8418174 [details] [diff] [review]
Typed objects shouldn't appear to be ArrayBuffer views

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

This makes JS_IsArrayBufferViewObject mismatch the class hierarchy. I've filed followup bug 1009678 to fix that. For now, I think we've decided to just go ahead with this approach.
Attachment #8418174 - Flags: review?(sphink) → review+
Comment on attachment 8418173 [details] [diff] [review]
ArrayBufferView JSAPI test

Typed objects are trunk-only right now, so this is technically trunk-only.  But this patch happens to have been developed underneath bug 991981's patches, so it may help me to backport it a bit.  Not clear yet.  If it's helpful on branches, I'll request approval for that then.

I don't think it matters if we land patch and test at the same time, given how little code is touched here.  If someone looks, it's straightforward to recognize the issue.  *Exploiting* is much less clear, without investigation that's just not worth it to me because of how compiler-specific it would end up being.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Unclear, depends on vagaries of the compiler in use.  Might or might not be possible, depending what the compiler does.

> Do comments in the patch, the check-in comment, or tests
> included in the patch paint a bulls-eye on the security problem?
No more than the code changes themselves do, tho I'll censor the patch summaries as a matter of course before landing.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
Nightly-only because typed objects are nightly-only.

> Do you have backports for the affected branches? If not, how
> different, hard to create, and risky will they be?
Haven't tried, probably easy to do if I find it helps me to do it.

> How likely is this patch to cause regressions; how much
> testing does it need?
Unlikely.  None beyond the test here.
Attachment #8418173 - Flags: review?(nmatsakis) → sec-approval?
Comment on attachment 8418174 [details] [diff] [review]
Typed objects shouldn't appear to be ArrayBuffer views

[Security approval request comment]
See comment 19 for approval request info.
Attachment #8418174 - Flags: sec-approval?
You don't need sec-approval for things that only really affect Nightly.
Attachment #8418173 - Flags: sec-approval? → sec-approval+
Comment on attachment 8418174 [details] [diff] [review]
Typed objects shouldn't appear to be ArrayBuffer views

sec-approval+ but if you want to backport to Aurora, you'll need an Aurora nomination and approval.
Attachment #8418174 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/e25785e174ff

Looks like I didn't need to get this in aurora, so we're good here.

Did not land test at same time, will give it a week or so for updates for people on nightly or something.  (Also my attention is kind of distracted ATM.  :-) )
https://hg.mozilla.org/mozilla-central/rev/e25785e174ff
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Group: javascript-core-security
Confirmed crash in Fx32, 2014-04-29.
Verified fixed in Fx32, 2014-07-16.
Status: RESOLVED → VERIFIED
Group: core-security
Keywords: regression
This broke a few times over the last few months, but the rebasing wasn't utterly horrendous.  Here's what I'd push now, but for bug 1082141 (regression from bug 1061404 -- only a few weeks ago, so only trunk/aurora are affected), which only landed this cycle.  Once that bug is fixed (should be, very soon), this can land on trunk, then once that patch lands on aurora (which it should) we can backport the patch and be okay everywhere about this.
Attachment #8418173 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0e6c1ac0391

This wants an uplift to aurora, but bug 1082141 needs to land there before that can happen.
You need to log in before you can comment on or make changes to this bug.