1352681 - SharedArrayBuffers: refcount leak during serialization leads to potential use-after-free

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Samuel Groß]

Attachment: Proof-of-concept code to trigger the bug

Summary 
-------
A refcount leak is present in the handling of SharedArrayBuffers during structured cloning. It can be abused to trigger a refcount overflow and subsequent use-after-free of a SharedArrayBuffer. 
SharedArrayBuffers are not yet enabled in the release version of Firefox (v52). However, the current beta (v53) has them enabled.


Detailed description
--------------------
SharedArrayBuffers make use of a custom reference counting mechanism:

    class SharedArrayRawBuffer
    {
      private:
        mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
        // ...

They can also be transferred to a web worker, in which case this refcount will be incremented by one. The following code is responsible for handling SharedArrayBuffers that are transferred during structured cloning:

    bool
    JSStructuredCloneWriter::writeSharedArrayBuffer(HandleObject obj)
    {
        if (!cloneDataPolicy.isSharedArrayBufferAllowed()) {
            JS_ReportErrorNumberASCII(context(), GetErrorMessage, nullptr, JSMSG_SC_NOT_CLONABLE,
                                      "SharedArrayBuffer");
            return false;
        }

        Rooted<SharedArrayBufferObject*> sharedArrayBuffer(context(), &CheckedUnwrap(obj)->as<SharedArrayBufferObject>());
        SharedArrayRawBuffer* rawbuf = sharedArrayBuffer->rawBufferObject();

        // Avoids a race condition where the parent thread frees the buffer
        // before the child has accepted the transferable.
        rawbuf->addReference();                 // [[ 1 ]]

        intptr_t p = reinterpret_cast<intptr_t>(rawbuf);
        return out.writePair(SCTAG_SHARED_ARRAY_BUFFER_OBJECT, static_cast<uint32_t>(sizeof(p))) &&
               out.writeBytes(&p, sizeof(p));
    }

At [[ 1 ]] the refcount of the SharedArrayBuffer is (artificially) increased to keep it alive during the transfer. It will later be decremented again during JSStructuredCloneReader::readSharedArrayBuffer. However, if the structured clone algorithm fails after the refcount has been increased, it is never decreased again, leading to a refcount leak. This in turn directly leads to a refcount overflow after 0x100000000 increments.

The structured clone algorithm checks for duplicates during serialization, thus `serialize([ab, ab]);` will only increment the refcount once. However, since different SharedArrayBufferObjects can point to the same SharedArrayRawBuffer instance, it is still possible to increment the refcount multiple times per invocation (which is much faster than incrementing once per invocation).
The attached POC implements this: first it creates 0x100000 copies of a SharedArrayBufferObject, then triggers the refcount leak to overflow the refcount. In a JavaScript shell on my 2015 MacBook Pro this takes about 35 minutes. It may be faster with more copies.

The POC makes use of some functions that are only available in the js shell: serialize(), unserialze(), and gc(). In the browser, serialize() and unserialize() can be replaced with window.postMessage(), while garbage collection can be triggered through other means (e.g. https://github.com/saelo/foxpwn/blob/master/code.js#L297).

Here is the output of executing poc.js:

    > time ./obj-ff-release/dist/bin/js poc.js
    1/4095 - 0.02%
    2/4095 - 0.05%
    3/4095 - 0.07%
    4/4095 - 0.10%
    5/4095 - 0.12%
    ...
    4091/4095 - 99.90%
    4092/4095 - 99.93%
    4093/4095 - 99.95%
    4094/4095 - 99.98%
    4095/4095 - 100.00%
    [2]    22920 segmentation fault  ./obj-ff-release/dist/bin/js poc.js
    ./obj-ff-release/dist/bin/js poc.js  2057.79s user 71.22s system 99% cpu 35:37.80 total

The backtrace at the time of the crash (at engine shut down in this case):

    * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x103a00fe8)
      * frame #0: 0x000000010054f080 js`js::SharedArrayRawBuffer::dropReference() [inlined] std::__1::__atomic_base<unsigned int, true>::fetch_sub(__op=1, __m=memory_order_acq_rel) at atomic:936 [opt]
        frame #1: 0x000000010054f07b js`js::SharedArrayRawBuffer::dropReference() [inlined] mozilla::detail::IntrinsicAddSub<unsigned int, (mozilla::MemoryOrdering)1>::sub(aVal=1) at Atomics.h:259 [opt]
        frame #2: 0x000000010054f07b js`js::SharedArrayRawBuffer::dropReference() [inlined] mozilla::detail::IntrinsicIncDec<unsigned int, (mozilla::MemoryOrdering)1>::dec(std::__1::atomic<unsigned int>&) at Atomics.h:291 [opt]
        frame #3: 0x000000010054f07b js`js::SharedArrayRawBuffer::dropReference() [inlined] mozilla::detail::AtomicBaseIncDec<unsigned int, (mozilla::MemoryOrdering)1>::operator--() at Atomics.h:610 [opt]
        frame #4: 0x000000010054f07b js`js::SharedArrayRawBuffer::dropReference(this=0x0000000103a00fe8) at SharedArrayObject.cpp:179 [opt]
        frame #5: 0x000000010054f4aa js`js::SharedArrayBufferObject::Finalize(fop=<unavailable>, obj=0x00000001025821c0) at SharedArrayObject.cpp:310 [opt]
        frame #6: 0x00000001004145e9 js`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] js::Class::doFinalize(this=<unavailable>, fop=<unavailable>) const at Class.h:881 [opt]
        frame #7: 0x00000001004145df js`unsigned long js::gc::Arena::finalize<JSObject>(js::FreeOp*, js::gc::AllocKind, unsigned long) [inlined] JSObject::finalize(js::FreeOp*) at jsobjinlines.h:87 [opt]
        frame #8: 0x00000001004145b8 js`unsigned long js::gc::Arena::finalize<JSObject>(this=<unavailable>, fop=<unavailable>, thingKind=<unavailable>, thingSize=<unavailable>) at jsgc.cpp:465 [opt]
        frame #9: 0x00000001004142d2 js`bool FinalizeTypedArenas<JSObject>(fop=<unavailable>, src=0x00007fff5fbfdb38, dest=0x00007fff5fbfdb60, thingKind=<unavailable>, budget=0x00007fff5fbfdb40, keepArenas=<unavailable>) at jsgc.cpp:523 [opt]
        frame #10: 0x00000001003f647d js`FinalizeArenas(fop=<unavailable>, src=0x00007fff5fbfdb38, dest=0x00007fff5fbfdb60, thingKind=<unavailable>, budget=0x00007fff5fbfdb40, keepArenas=KEEP_ARENAS) at jsgc.cpp:557 [opt]
        frame #11: 0x00000001003f627d js`js::gc::ArenaLists::backgroundFinalize(fop=<unavailable>, listHead=<unavailable>, empty=0x00007fff5fbfeba0) at jsgc.cpp:2780 [opt]
        frame #12: 0x00000001003f94e5 js`js::gc::GCRuntime::sweepBackgroundThings(this=<unavailable>, zones=<unavailable>, freeBlocks=<unavailable>) at jsgc.cpp:3178 [opt]
        frame #13: 0x000000010040179e js`js::gc::GCRuntime::endSweepingZoneGroup(this=0x0000000102373460) at jsgc.cpp:5258 [opt]
        frame #14: 0x00000001004026ee js`js::gc::GCRuntime::sweepPhase(this=<unavailable>, sliceBudget=<unavailable>, lock=<unavailable>) at jsgc.cpp:5471 [opt]
        frame #15: 0x000000010040496b js`js::gc::GCRuntime::incrementalCollectSlice(this=<unavailable>, budget=<unavailable>, reason=<unavailable>, lock=<unavailable>) at jsgc.cpp:6066 [opt]
        frame #16: 0x0000000100405561 js`js::gc::GCRuntime::gcCycle(this=0x0000000102373460, nonincrementalByAPI=<unavailable>, budget=0x00007fff5fbfeee0, reason=DESTROY_RUNTIME) at jsgc.cpp:6351 [opt]
        frame #17: 0x00000001004064f8 js`js::gc::GCRuntime::collect(this=0x0000000102373460, nonincrementalByAPI=<unavailable>, budget=<unavailable>, reason=DESTROY_RUNTIME) at jsgc.cpp:6500 [opt]
        frame #18: 0x0000000100406881 js`js::gc::GCRuntime::gc(this=<unavailable>, gckind=<unavailable>, reason=<unavailable>) at jsgc.cpp:6566 [opt]
        frame #19: 0x0000000100538457 js`JSRuntime::destroyRuntime(this=0x0000000102373000) at Runtime.cpp:308 [opt]
        frame #20: 0x00000001003b85c4 js`js::DestroyContext(cx=0x0000000102341000) at jscntxt.cpp:226 [opt]
        frame #21: 0x0000000100007f57 js`main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) at js.cpp:8471 [opt]
        frame #22: 0x0000000100001954 js`start + 52

Comment 1

[:Gijs (out for now; he/him)]

Lars, looks like you know about this stuff? Please feel free to forward as appropriate.

Comment 2

[Samuel Groß]

I suggest adding an overflow check to the refcount as part of the fix. 2^32 "legit" references would currently require 2^32 * 32B = 128GB, but with e.g. page compression (on macOS) and swapping, this might not be too unrealistic in the near future...

Comment 3

[Lars T Hansen [:lth]]

Nice find.

Comment 4

[Lars T Hansen [:lth]]

A fun Monday morning all around.  There are at least four related problems here:

- no overflow check on the reference count [as reported above]
- if the SC algorithm fails, the added reference should be dropped, or the SAB will leak [as reported above]
- if the SC buffer is dropped on the floor (not deserialized), the added reference is not dropped, so the SAB will leak
- if the SC buffer is deserialized multiple times we drop the reference multiple times, but it was only added once, so we crash

Additional test cases coming up.

Comment 5

[Lars T Hansen [:lth]]

Attachment: deserialize-multi.js

Deserialize a buffer multiple times and we crash because we drop the SAB reference too many times.

Comment 6

[Lars T Hansen [:lth]]

Attachment: leak-sab.js

Fail to deserialize and we leak the SAB.  This test case is interactive; see instructions in the comments.

Comment 7

[Lars T Hansen [:lth]]

The proper fix for handling the reference counts in the structured clone algorithm is likely to make SAB participate in the clear/copy/steal/adopt regime that the SC algorithm now uses, and not simply dropReference the SAB on deserialize().  If the finalizer for an SC datum (whether in the shell or in the browser) properly calls clear(), and clear() performs the appropriate dropReference, and deserialize() does not assume that it can take ownership of the addReference from serialize() but instead increments the reference count for the new object, then we should properly handle both leaks and multiple deserializations.  I'm still unclear on whether this handles aborted serialization or whether that requires an additional mechanism.

Comment 8

[Lars T Hansen [:lth]]

Attachment: bug1352681-no-sab-rc-oflo.patch

Part 1: Catch refcount overflow on the SharedArrayRawBuffer.  Also promotes two assertions to MOZ_RELEASE_ASSERT; this code is not remotely hot.

Comment 9

[Lars T Hansen [:lth]]

Attachment: bug1352681-sab-rc-accounting.patch

Part 2: Reengineer the refcount accounting as follows:

- when SC sees a SAB it ups the refcount and stuffs the SAB rawBuffer into a
  list in the SC writer
- when the data are extracted from the SC writer the list is moved to the SC
  data container
- when the SC data container is cleared, copied, stolen, or adopted, either
  clear, copy, or move the list of owned rawbuffers, incrementing and
  decrementing refcounts as appropriate
- when the SC writer, or the SC data container, is destroyed, then decrement
  the refcounts
- increment the refcount on a buffer that is extracted into a new SAB object

This fixes my test cases and should fix the original test case on this bug (haven't tested yet, need to set up a run).

This needs careful vetting, I'm not sure I've caught all the cases.  Also see the TODO I left in this patch re a possible missing OOM check.

Unlike the transfer list, the list of rawbuffers does not fit in with the SC buffer data, so there are additional arrays hanging off the SC writer and the SC data.  Not ideal but it was the best fix I could find.

Comment 10

[Marcia Knous [:marcia]]

Tracking 53/54/55+ for this sec issue.

Comment 11

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

Comment on attachment 8853950 [details] [diff] [review]
bug1352681-no-sab-rc-oflo.patch

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

::: js/src/shell/js.cpp
@@ +5520,5 @@
>      sharedArrayBufferMailboxLock->lock();
>      SharedArrayRawBuffer* buf = sharedArrayBufferMailbox;
>      if (buf) {
> +        if (!buf->addReference()) {
> +            JS_ReportErrorASCII(cx, "Reference count overflow on SharedArrayBuffer");

Why not JS_ReportErrorNumberASCII in this file? (I know it's just a test file, but it still seems preferable.)

@@ +5525,2 @@
>              rval = false;
> +        } else {

Barely related to this patch, but it seems like it would be safer to use RAII to manage the locking here. If there isn't an RAII class for these locks already, at least use MakeScopeExit to do manual RAII.

::: js/src/vm/SharedArrayObject.cpp
@@ +178,5 @@
> +            return false;
> +        if (this->refcount_.compareExchange(old_refcount, new_refcount))
> +            break;
> +    }
> +    return true;

Unfortunate. But I see the need.

Comment 12

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

Comment on attachment 8853951 [details] [diff] [review]
bug1352681-sab-rc-accounting.patch

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

::: js/public/StructuredClone.h
@@ +206,5 @@
>  
>      const JSStructuredCloneCallbacks* callbacks_;
>      void* closure_;
>      OwnTransferablePolicy ownTransferables_;
> +    js::Vector<js::SharedArrayRawBuffer*, 0, js::SystemAllocPolicy> refsHeld_;

Given the number of places that manipulate this in various ways, I'd feel much better if you buffer ref ownership were encapsulated in a class. SharedArrayRawBufferRefs or SharedArrayRawBufferOwnership or something. I think it could do the error reporting, even.

class SharedArrayRawBufferRefs {
    js::Vector<js::SharedArrayRawBuffer*, 0, js::SystemAllocPolicy> refs_;
  public:
    ~SharedArrayRawBufferRefs() { release(); }
    void release() {
        for (auto ref : refs_)
            ref.dropReference();
        refs_.clear();
    }
    bool acquire(JSContext*cx, js::SharedArrayRawBuffer*) ...include error reporting...
    void takeOwnership(SharedArrayRawBufferRefs&&) {
        MOZ_ASSERT(refs_.empty()); ... // or release(); ...
    }
    bool copy(JSContext* cx, const SharedArrayRawBufferRefs& srcRefs) {
        if (!refs_.reserve(refs_.length() + srcRefs.refs_.length()))
           report oom;
        for (auto ref : srcRefs) {
            if (!addReference)
                report overreffed;
            refs_.append(ref);
        }
    }
};

I *think* that's more or less correct.

::: js/src/vm/StructuredClone.cpp
@@ +407,5 @@
>  
>      bool extractBuffer(JSStructuredCloneData* data) {
>          bool success = out.extractBuffer(data);
>          if (success) {
> +            data->refsHeld_.swap(refsHeld);

I'd prefer a Move assignment (or explicit function call that transfers ownership) instead of swap(), which makes me think both sets of refs are meaningful.

@@ +980,5 @@
>      }
> +
> +    // If we still own the addReference()s for these SABs, drop them again.
> +    for ( uint32_t i=0; i < refsHeld.length(); i++ )
> +        refsHeld[i]->dropReference();

I don't understand the "again". This is the case where you never gave up those references, right?

I guess you meant "again" statically? I'd rather omit it from the comment.

@@ +1177,5 @@
> +    if (!refsHeld.append(rawbuf)) {
> +        ReportOutOfMemory(context());
> +        return false;
> +    }
> +

Ugh, two different fallible operations to transfer ownership. It looks right to me. But I think I'd rather encapsulate this in a class.

@@ -1897,5 @@
> -        // The sending side performed a reference increment before sending.
> -        // Account for that here before leaving.
> -        if (rawbuf)
> -            rawbuf->dropReference();
> -

Good riddance. That ref counting logic strikes me as extraordinarily fragile. Who reviewed that, anyway?... oh. Darn.

@@ +2633,5 @@
>      clear();
>  
>      auto iter = srcData.Iter();
>      while (!iter.Done()) {
> +        // TODO: are we dropping an OOM on the floor here?

Can you file a bug for this and put the bug number in here? WriteBytes and friends should all be MOZ_MUST_USE.

@@ +2650,5 @@
> +                data_.refsHeld_[j]->dropReference();
> +            data_.refsHeld_.clear();
> +            return false;
> +        }
> +    }

Move this into the ref-holding struct.

Comment 13

[Lars T Hansen [:lth]]

Attachment: bug1352681-sab-rc-accounting-v2.patch

This introduces a container object as suggested.  It is pretty clean overall, definitely an improvement.

That said, I struggled mightily to get the Move semantics right, so a careful check of that logic would be most welcome.

(As for the OOM result that was not checked, I fixed it here and filed bug 1353767 for the work item of marking the methods MOZ_MUST_USE.)

Comment 14

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

Comment on attachment 8854920 [details] [diff] [review]
bug1352681-sab-rc-accounting-v2.patch

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

::: js/src/vm/StructuredClone.cpp
@@ +274,5 @@
> +    // increment fails, decrement the ones we incremented, reset the array to
> +    // its previous state, and return failure.
> +
> +    uint32_t len = refs_.length();
> +    for ( uint32_t i=before; i < len; i++ ) {

Spacing style (looks like house style conflicts with yours, sorry).

for (uint32_t i = before; i < len; i++)

@@ +277,5 @@
> +    uint32_t len = refs_.length();
> +    for ( uint32_t i=before; i < len; i++ ) {
> +        if (!refs_[i]->addReference()) {
> +            for ( uint32_t j=before; j < i; j++ )
> +                refs_[j]->dropReference();

Do you prefer to do this dropReference stuff? I thought it was nicer to leave the state consistent pretty much all the time, so you wouldn't have extra non-owned references in the list. Then you can just bail out, and rely on it getting cleaned up in the destructor.

if (!reserve) reportOOM;
for (auto ref : that.refs_) {
  if (!ref.addReference)
    report error and return false;
  _refs.append(ref); // infallible because of the reserve

No more dropReference, and the contents of refs_ never gets mismatched with what we have a reference to, so it's safe to bail at any time.

@@ +291,5 @@
> +void
> +SharedArrayRawBufferRefs::takeOwnership(SharedArrayRawBufferRefs&& other)
> +{
> +    MOZ_ASSERT(refs_.empty());
> +    refs_ = Move(other.refs_);

I'm not sure whether the Move here is necessary or not. I had always thought that it was only needed for plain identifiers, but Waldo explained to me once why that was wrong, and I have now forgotten the explanation.

At any rate, please leave it here. I'm just talking to myself.

Comment 15

[Lars T Hansen [:lth]]

(In reply to Steve Fink [:sfink] [:s:] from comment #14)
> Comment on attachment 8854920 [details] [diff] [review]
> bug1352681-sab-rc-accounting-v2.patch
> 
> Review of attachment 8854920 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/StructuredClone.cpp
> 
> @@ +291,5 @@
> > +void
> > +SharedArrayRawBufferRefs::takeOwnership(SharedArrayRawBufferRefs&& other)
> > +{
> > +    MOZ_ASSERT(refs_.empty());
> > +    refs_ = Move(other.refs_);
> 
> I'm not sure whether the Move here is necessary or not. I had always thought
> that it was only needed for plain identifiers, but Waldo explained to me
> once why that was wrong, and I have now forgotten the explanation.

It's necessary because there's some trickiness having to do with overflow resolution.  The Move() is necessary to select the move-assignment operator, which is otherwise unselected because there's no automatic coercion from the lvalue other.refs_ to the required rvalue reference.

Comment 16

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c63f2de2f15f4a2638a96f5b885bfe9e990120e3

Comment 17

[Lars T Hansen [:lth]]

Attachment: bug1352681-no-sab-rc-oflo-v2.patch

Final patch #1 (m-c): overflow checking.  Carrying sfink's r+.

Comment 18

[Lars T Hansen [:lth]]

Attachment: bug1352681-sab-rc-accounting-v3.patch

Final patch #2 (m-c): proper reference counting.  Carrying sfink's r+.

Comment 19

[Lars T Hansen [:lth]]

Attachment: bug1352681-no-sab-rc-oflo-v2-BETA.patch

This is patch #1, adapted to the BETA branch.

Comment 20

[Lars T Hansen [:lth]]

Attachment: bug1352681-sab-rc-accounting-v3-AURORA-BETA.patch

This is patch #2, adapted to the AURORA and BETA branches.

Comment 21

[Lars T Hansen [:lth]]

Comment on attachment 8855190 [details] [diff] [review]
bug1352681-no-sab-rc-oflo-v2.patch

DO NOTE that the feature in question - SharedArrayBuffer and Atomics - is off by default in all FF release versions through FF52 and that by default no FF users are vulnerable.  The feature may be enabled in FF53, TBD.

DO NOTE there are two patches here that have to land together.

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

Exploit unclear.  The original PoC runs flat-out for 35 minutes before managing to crash the JS shell, the browser would take at least as long as that.  Other errors that I found (multiple deserialization, see comment thread) are not available in the browser I believe, only in the shell.

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

The check-in comment mentions reference count overflow, which is at least a hint.

Which older supported branches are affected by this flaw?

Aurora and Beta are affected.
ESR52 is vulnerable in the same way that 53/54/55 are.
ESR45 has very different code but it likely vulnerable to the same overflow bug.

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

There are several flaws.  A failure to drop the reference count if the Structured Clone later fails has been there since the beginning.  The failure to properly account for multiple references to buffers was arguably introduced with bug 1264642.

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

I have patches for Aurora and Beta.  I could easily construct a patch for ESR52.  I think ESR45 we should just leave alone.

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

It is not without risk and we have scant test cases but it will not *regress* anything much because the feature has been disabled by default so far.

Comment 22

[Lars T Hansen [:lth]]

Comment on attachment 8855191 [details] [diff] [review]
bug1352681-sab-rc-accounting-v3.patch

[Security approval request comment]
See comment on the other patch.

Comment 23

[Lars T Hansen [:lth]]

Comment on attachment 8855190 [details] [diff] [review]
bug1352681-no-sab-rc-oflo-v2.patch

Approval Request Comment
[Feature/Bug causing the regression]:
SharedArrayBuffer

[User impact if declined]:
Possible use-after-free error

[Is this code covered by automated tests?]:
Yes, though probably only superficially

[Has the fix been verified in Nightly?]:
No, pending for sec-approval, but time is getting short

[Needs manual test from QE? If yes, steps to reproduce]: 
No

[List of other uplifts needed for the feature/fix]:
There is a second patch on this bug labeled for Aurora that must also land

[Is the change risky?]:
Somewhat

[Why is the change risky/not risky?]:
Reference counting machinery is always hard to get right

[String changes made/needed]:
There's a new error message in this patch in js/src/js.msg but I don't know if we localize those

Comment 24

[Lars T Hansen [:lth]]

Comment on attachment 8855201 [details] [diff] [review]
bug1352681-sab-rc-accounting-v3-AURORA-BETA.patch

Approval Request Comment
[Feature/Bug causing the regression]:
SharedArrayBuffer

[User impact if declined]:
Possible use-after-free problem

[Is this code covered by automated tests?]:
Yes, but only superficially

[Has the fix been verified in Nightly?]:
Working on it.  That patch is pending sec-approval, but time is running out

[Needs manual test from QE? If yes, steps to reproduce]: 
Not really

[List of other uplifts needed for the feature/fix]:
There is another patch that must land (on this bug), one labeled for beta and the other for aurora

[Is the change risky?]:
Somewhat

[Why is the change risky/not risky?]:
Reference counting machinery is hard to get right

[String changes made/needed]:
None

Comment 25

[Lars T Hansen [:lth]]

Comment on attachment 8855200 [details] [diff] [review]
bug1352681-no-sab-rc-oflo-v2-BETA.patch

Approval Request Comment
[Feature/Bug causing the regression]:
SharedArrayBuffer

[User impact if declined]:
Possible use-after-free issue, certainly crashers

[Is this code covered by automated tests?]:
Yes, though only superficially

[Has the fix been verified in Nightly?]:
Working on it; other bugs are pending sec-approval

[Needs manual test from QE? If yes, steps to reproduce]: 
Not really

[List of other uplifts needed for the feature/fix]:
There's another patch on this bug labeled for Beta, it too must land

[Is the change risky?]:
Somewhat

[Why is the change risky/not risky?]:
Reference counting is always hard to get right

[String changes made/needed]:
There's a new error message here in js/src/js.msg

Comment 26

[Lars T Hansen [:lth]]

We aim to ship this feature in FF53.

Comment 27

[Al Billings [:abillings - ex-MoCo]]

You've missed the window for Firefox 53. We're making release builds in several days and there are no more betas.

I've giving this sec-approval+ for checking in on *May 2* and not before. This is two weeks into the next cycle. You're going to have to wait until then to go in.

If you want to try to make a case to land this with no beta builds, you'll need to convince release management to take it.

Comment 28

[Lars T Hansen [:lth]]

(In reply to Al Billings [:abillings] from comment #27)
> You've missed the window for Firefox 53. We're making release builds in
> several days and there are no more betas.
> 
> I've giving this sec-approval+ for checking in on *May 2* and not before.
> This is two weeks into the next cycle. You're going to have to wait until
> then to go in.

Can you clarify (because I've never been in this situation before) whether that restriction applies to all branches or just to the FF53 mozilla-beta branch?

Anyway, I was afraid of that.  In that case, SharedArrayBuffer will ship enabled-by-default in FF53 with this bug present.  That is not the end of the world because there is no known exploit and anyway shared memory is unmapped when freed so use-after-free is somewhat farfetched, but it makes me a little nervous.

> If you want to try to make a case to land this with no beta builds, you'll
> need to convince release management to take it.

I'll pass :)

Comment 29

[Samuel Groß]

In case it helps land the patch earlier, here's a short description on how to exploit the bug: once the shared memory is unmapped, one can allocate lots of ArrayBuffer instances of size 96 (largest size so the data is still stored inline). That will in turn create many new Arenas which will be allocated through mmap, which fills up existing holes (like the one from the shared data). At that point one can freely control the data of an ArrayBufferObject, leading to an arbitrary read/write in memory (similar to this exploit I wrote some time ago: https://saelo.github.io/posts/firefox-script-loader-overflow.html).
But yes, it does take quite a long time to trigger the bug.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

(In reply to Lars T Hansen [:lth] from comment #28)

> Can you clarify (because I've never been in this situation before) whether
> that restriction applies to all branches or just to the FF53 mozilla-beta
> branch?

Sec-approval is a mozilla-central flag. When you get sec-approval, that just determines whether a sec-high or sec-critical rated issue can land on central. It has no bearing on branches (other than the fact that you can't land a security bug fix that affects trunk on branches without landing it on trunk first). In order to land on branches, you must request specific branch approval for your patch via patch flags for those branches.

Comment 31

[Jeff Walden [:Waldo]]

Comment on attachment 8854920 [details] [diff] [review]
bug1352681-sab-rc-accounting-v2.patch

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

::: js/src/vm/StructuredClone.cpp
@@ +291,5 @@
> +void
> +SharedArrayRawBufferRefs::takeOwnership(SharedArrayRawBufferRefs&& other)
> +{
> +    MOZ_ASSERT(refs_.empty());
> +    refs_ = Move(other.refs_);

Move is definitely necessary here, because 1) refs_ is a Vector and Vector deliberately doesn't have a copy constructor or copy assignment operator, and 2) by accepting a SARBR&& you're purporting to possibly consume/take ownership of |other| -- and the name implies you definitely are taking ownership -- and so you want to move all of |other| into |*this|.  The only field in |this| is |refs_|, so you want to Move it out of |other| into |this|.  This is 100% correct style and implementation of the take-ownership idiom.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

After more discussion we are going to disable the feature for 53 (in bug 1354160) but would like to land this fix for the RC build.

Comment 33

[Daniel Veditz [:dveditz]]

If we're going to be disabling this in firefox 53 (bug 1354160) then there's no reason to hold up landing on central now.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 8855190 [details] [diff] [review]
bug1352681-no-sab-rc-oflo-v2.patch

sec-approval=dveditz
a=dveditz for aurora

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bcdca1c4da67cc705f3615160f5ee31feb044da
https://hg.mozilla.org/integration/mozilla-inbound/rev/d16bffb1859d7120bc82c0b39bb7d9fc664e8816

Comment 36

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8855200 [details] [diff] [review]
bug1352681-no-sab-rc-oflo-v2-BETA.patch

Let's take this for beta 53.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6b46c9a63b28
https://hg.mozilla.org/releases/mozilla-aurora/rev/f6712e7858f3

https://hg.mozilla.org/releases/mozilla-beta/rev/c1e1e7392f5d
https://hg.mozilla.org/releases/mozilla-beta/rev/ac4dedf08884

Comment 38

[Jeff Walden [:Waldo]]

jorendorff asked me to take a gander at this (even beyond the Move thing, which I had noticed in bugmail but hadn't gotten around to commenting on).

I can't say I see anything definitely affirmatively *wrong* with this, but 1) I don't think overflow-checking should be necessary if we properly drop refs on partial failure, 2) I think having overflow-checking is more than a bit confusing and I would remove it ASAP if it were me, 3) that vector really needs to contain RefPtr to avoid the manual refcounting.  But all of that can wait past now.

Also, JSMSG_SC_SAB_REFCNT_OFLO is a horrific name and we all ought to be ashamed of ourselves for allowing such an awful abbreviation to land.  :-)

Comment 39

[Jason Orendorff [:jorendorff]]

Yes. To clarify, Waldo sfink and I took another look at this patch, and we found no correctness issues. I'm glad this landed.

I agree with all Waldo's comments, though. In the unlikely case that there are any more bugs in the reference counting here, I think making these changes, particularly #3, would likely shake them loose. I'll file a follow-up bug (in the open, Monday).

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bcdca1c4da6
https://hg.mozilla.org/mozilla-central/rev/d16bffb1859d

Comment 41

[Lars T Hansen [:lth]]

Attachment: bug1352681-test-cases.patch

Shell test cases for the three central problems:

- failed serialization leaves the SharedArrayRawBuffer refcount inflated
- dropping a serialization buffer leaks the SharedArrayRawBuffer
- deserializing a serialization buffer multiple times drops the
  SharedArrayRawBuffer reference count below zero, frees the memory, and
  causes a crash

As somebody observed earlier, without these problems RC overflow would not be an issue (given our present 4G heap limit).

Comment 42

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

(In reply to Lars T Hansen [:lth] from comment #24)

> [Needs manual test from QE? If yes, steps to reproduce]: 
> Not really

Comment 43

[Jeff Walden [:Waldo]]

Comment on attachment 8856490 [details] [diff] [review]
bug1352681-test-cases.patch

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

::: js/src/builtin/TestingFunctions.cpp
@@ +2690,5 @@
> +    if (args.length() != 1 || !args[0].isObject()) {
> +        JS_ReportErrorASCII(cx, "Expected SharedArrayBuffer object");
> +        return false;
> +    }
> +    RootedObject obj(cx, ToObject(cx, args.get(0)));

&args[0].toObject();

::: js/src/tests/js1_8_5/extensions/clone-sab-leak.js
@@ +29,5 @@
> +// The serialization buffer hangs onto the SAB buffer, gc does not reap it even
> +// if the original variable does not reference it and the SAB object is dead and
> +// gone.
> +sab = null;
> +gc();

Possibly worth putting in two talismanic calls here, mimicking what other code does to deal with certain screwball GC edge cases at least in the browser.  e_e

Comment 44

[Lars T Hansen [:lth]]

Attachment: bug1352681-test-cases-v2.patch

Updated patch, carrying r+ from sfink and waldo.

Note, code fixes have landed on all branches, these are just test cases.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Cleverness is required but possible, see earlier comments.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test cases definitely point to the security problem, and one of them is clearly not dependent on the JS shell.

Which older supported branches are affected by this flaw?
None per se, these are test cases, meant for central only.

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

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

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

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Since we've disabled this on 53, I see no reason not to land tests now.

Comment 46

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/075aa2b7342add7abaa3d75f9b09fdc5be2482b1
Bug 1352681: Test cases for serialize/deserialize SAB. r=sfink, r=waldo

Comment 47

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d06374cddb9fb4c4c619bcda8a4ade3aac8bd479
Bug 1352681: Test cases for serialize/deserialize SAB, fix busted build.  r=me

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/075aa2b7342a
https://hg.mozilla.org/mozilla-central/rev/d06374cddb9f