"Assertion failure: view" with gcslice and neuter

RESOLVED FIXED in Firefox 27

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jruderman, Assigned: sfink)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla28
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 wontfix, firefox27 fixed, firefox28 fixed, firefox-esr24 wontfix, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed)

Details

(Whiteboard: [fuzzblocker:neuter][adv-main27+])

Attachments

(1 attachment, 1 obsolete attachment)

var a = new Uint8Array();
a.subarray();
gcslice(0);
gcslice(1781);
neuter(a.buffer);

Assertion failure: view, at js/src/vm/TypedArrayObject.cpp:824

Using the trick in bug 929786 comment 4, the first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0efef923cc17
user:        Steve Fink
date:        Tue Oct 15 23:48:01 2013 -0700
summary:     Bug 861925 - Allow grabbing data from ArrayBuffers and neutering them independently (in addition to Steal, which does both at the same time). r=Waldo
Blocks: 861925
We maintain a list of ArrayBuffers with more than view. Sadly, we do not remove them from the list when neutering.

"We", here, is pronounced "sfink".
Attachment #8334141 - Flags: review?(wmccloskey)
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

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

::: js/src/vm/TypedArrayObject.cpp
@@ +350,5 @@
>          if (!ArrayBufferObject::neuterAsmJSArrayBuffer(cx, *this))
>              return false;
>      }
>  
> +    // remove buffer from the list of buffers with > 1 view

Please capitalize and add a period at the end.
Attachment #8334141 - Flags: review?(wmccloskey) → review+
Keywords: regression
This needed a rating before it went in and since it affects Aurora and Trunk both, it needed security approval. 

See https://wiki.mozilla.org/Security/Bug_Approval_Process.

Can you suggest a security rating and answer the sec-approval? template questions?
Keywords: regression
Keywords: regression
Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

Requesting sec-approval, but on further reflection, I don't think this is sensitive at all.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It should be fairly easy to cause a near-NULL crash. I have no idea how to do anything more than that. I don't see how it's possible.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

It's pretty clear from the patch what the actual code flaw and how to construct a scenario that hits it. It's not very clear why it's a security problem.

Which older supported branches are affected by this flaw?

Everything since Firefox 18.

If not all supported branches, which bug introduced the flaw?

Bug 789295

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The code would be slightly different, but just because different helper functions were introduced at some point. The behavior would be the same.

How likely is this patch to cause regressions; how much testing does it need?

The situation where this arises is uncommon, so if it does introduce regressions, they won't hit very often in normal usage.

To be specific, to hit this bug you would need to:

1. Create an ArrayBuffer with multiple typed array views
2. Do a postMessage that transfers the ArrayBuffer
3. Timing it so that step #2 happens between incremental GC slices

The result is an ArrayBuffer with zero views on the list of "multiview" ArrayBuffers. Since the list is threaded through each buffer's first view, that means that when traversing the list, it'll dereference the address 0x8 or 0x10 or something like that. The exact offset is fixed; the user cannot interfere with it. (The list is only traversed during sweeping. So it would happen at the end of the current incremental GC.)
Attachment #8334141 - Flags: sec-approval?
Keywords: sec-low
Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

I agree with Andrew. This sounds like a sec-low so you're all good to go. Low and Medium rated issues don't need approval.
Attachment #8334141 - Flags: sec-approval?
landed on https://hg.mozilla.org/mozilla-central/rev/335ec0f81ead
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Per comment 5, this bug has been around since fx18
Steve, can you please nominate this for uplift?
Flags: needinfo?(sphink)
Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

Sorry for the premature landing.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 789295
User impact if declined: crashes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): similar crashes, if we missed something in the logic
String or IDL/UUID changes made by this patch: none
Attachment #8334141 - Flags: approval-mozilla-beta?
Attachment #8334141 - Flags: approval-mozilla-aurora?
For a sec-low, it may be a bit late in the cycle to take churn. I can see approving this for Aurora but probably not Beta.
Attachment #8334141 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8334141 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8334141 [details] [diff] [review]
Remove buffer from multiview list when neutered

NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 789295
User impact if declined: crashes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): similar crashes, if we missed something in the logic
String or IDL/UUID changes made by this patch: none

It might make sense to land this for b2g, since I think they use workers a bit more and may be more likely to encounter this issue? I don't really know though.
Attachment #8334141 - Flags: approval-mozilla-b2g26?
Attachment #8334141 - Flags: approval-mozilla-b2g18?
Flags: needinfo?(sphink)
blocking-b2g: --- → koi+
Preeti, do we want this on b2g18 as well?

Steve, can you please make at least a b2g26 patch?
Flags: needinfo?(sphink)
Flags: needinfo?(praghunath)
Ryan

This is not needed for 1.1HD, can track it for 1.1.1 though.
Flags: needinfo?(praghunath)
Attachment #8334141 - Attachment is obsolete: true
Attachment #8334141 - Flags: approval-mozilla-b2g26?
Attachment #8334141 - Flags: approval-mozilla-b2g18?
http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/9db448e74615 - backed out a87f1f662a6c because I missed an important clause in the original patch
http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/a87f1f662a6c - relanded
Followup fix https://hg.mozilla.org/releases/mozilla-b2g18/rev/7ef4780f267f (I seem to be incapable of doing trivial rebases)
Whiteboard: [fuzzblocker:neuter] → [fuzzblocker:neuter][adv-main27+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.