Closed Bug 1452784 Opened 4 years ago Closed 4 years ago

Allocator mismatch involving StructuredCloneWriteCallback() and ~DataOwner()

Categories

(Core :: Storage: IndexedDB, defect, P2)

58 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mozillabugs, Assigned: baku)

Details

Attachments

(4 files)

101.59 KB, text/html
Details
101.29 KB, application/x-javascript
Details
43.93 KB, application/octet-stream
Details
987 bytes, patch
decoder
: review+
Details | Diff | Splinter Review
Attached file test2.html
StructuredCloneWriteCallback() (dom\indexedDB\IDBObjectStore.cpp) creates objects using new[] that MemoryBlobImpl::DataOwner::~DataOwner() (dom\file\MemoryBlobImpl.cpp/.h) later releases using free().

This mismatch could cause heap corruption, depending upon how new[] is implemented. In the debug build of FF 58.0.2, the source (mozalloc.h) implements it by calling moz_xmalloc(), and the compiler appears not to add any additional data to the allocation, so the mismatch appears safe. Other compilers (or VS with different switches) could, however, add additional data to the allocation that new[] and delete[] use internally. In that case, using free() on the pointer returned by new[] would cause deallocation of only part of the block allocated by new[], which would cause heap corruption and possibly other issues with internal data maintained by new[] and delete[].

Attached is a POC that demonstrates the issue. Use it by putting the POC files on a webserver, starting FF, and putting a BP on StructuredCloneWriteCallback() line 633:

477: bool
478: StructuredCloneWriteCallback(JSContext* aCx,
479:                              JSStructuredCloneWriter* aWriter,
480:                              JS::Handle<JSObject*> aObj,
481:                              void* aClosure)
482: {
...
633:   if (JS::IsWasmModuleObject(aObj)) {
634:     RefPtr<JS::WasmModule> module = JS::GetWasmModule(aObj);
635:     MOZ_ASSERT(module);
636: 
637:     size_t bytecodeSize = module->bytecodeSerializedSize();
638:     UniquePtr<uint8_t[]> bytecode(new uint8_t[bytecodeSize]);
639:     module->bytecodeSerialize(bytecode.get(), bytecodeSize);
640: 
641:     RefPtr<BlobImpl> blobImpl =
642:       new MemoryBlobImpl(bytecode.release(), bytecodeSize, EmptyString());
643: 
644:     RefPtr<Blob> bytecodeBlob = Blob::Create(nullptr, blobImpl);
645: 
646:     if (module->compilationComplete()) {
647:       size_t compiledSize = module->compiledSerializedSize();
648:       UniquePtr<uint8_t[]> compiled(new uint8_t[compiledSize]);
649:       module->compiledSerialize(compiled.get(), compiledSize);
650: 
651:       blobImpl =
652:         new MemoryBlobImpl(compiled.release(), compiledSize, EmptyString());
653:     } else {
654:       nsCOMPtr<nsIInputStream> stream(new WasmCompiledModuleStream(module));
655: 
656:       blobImpl = StreamBlobImpl::Create(stream, EmptyString(), UINT64_MAX);
657:     }
658: 
659:     RefPtr<Blob> compiledBlob = Blob::Create(nullptr, blobImpl);
660:
... [queue the blob for writing]
688: }

and on ~DataOwner() (in the .h file, line 89).

80:     ~DataOwner() {
81:       mozilla::StaticMutexAutoLock lock(sDataOwnerMutex);
82: 
83:       remove();
84:       if (sDataOwners->isEmpty()) {
85:         // Free the linked list if it's empty.
86:         sDataOwners = nullptr;
87:       }
88: 
89:       free(mData);
90:     }

Now load test2.html in FF. When the BP in StructuredCloneWriteCallback() fires, trace the allocation on line 638 and remember the pointer returned by new[]. Do the same for the allocation on line 648.

Now proceed. When the BP on ~DataOwner() fires, verify that |mData| is one of the values that you recorded above, and that it is released by calling free(). Proceed and do the same thing again for the other value.
Attached file test2.js
Attached file test2.wasm
Flags: sec-bounty?
I think these allocator mismatches are not exploitable in our codebase because we use jemalloc for both. But I would like to defer that question to someone who knows it much better than I do.
Flags: needinfo?(mh+mozilla)
:decoder is right. In Firefox, new[] and new and implemented in terms of (derivatives of) malloc, and delete and delete[] in terms of (derivatives of) free. Which makes allocator mismatch not a practical problem. In fact, in many cases, they're inlined, and you wouldn't even observe calls to new/new[]/delete/delete[]. This can sometimes cause the converse problem, where you can observe a mismatch between e.g. malloc/delete when the code actually correctly uses new and delete.
Flags: needinfo?(mh+mozilla)
I'll add, though, that it's always useful to fix those mismatches even when they're not a practical problem.
(In reply to Mike Hommey [:glandium] from comment #4)
> :decoder is right. In Firefox, new[] and new and implemented in terms of
> (derivatives of) malloc, and delete and delete[] in terms of (derivatives
> of) free. Which makes allocator mismatch not a practical problem. In fact,
> in many cases, they're inlined, and you wouldn't even observe calls to
> new/new[]/delete/delete[].

This appears to be true when new (new[]) is used to allocate a POD (or array of PODs) and free() is used to deallocate it. However, allocation of non-PODs via new (new[]), combined with deallocation by free(), often will not work correctly. For one thing, free() will not call a non-POD's destructor. This could, for example, leave a freed object like:

   struct Foo {
      Foo *m_flink;
      Foo *m_blink;
      ...
      virtual ~Foo() {
         Foo *flink = m_flink;
         m_blink -> m_flink = m_flink;  // remove from list
         flink -> m_blink = mblink;
         ...
      }
   };

present in a linked list, causing a use after free bug. Conversely, allocation of a non-POD by malloc(), followed by deallocation by delete (delete[]) will cause use of (and a destructor call for) an object for the constructor was never called, which is UB.
an object for the constructor was never called => an object for *which* the constructor was never called,
Most of the code we have on Gecko uses private destructors with refcounting to ensure that things are deleted properly, or is handled by garbage collection. I'm not sure if/where we actually call delete on non-POD structures.

In this particular case it doesn't matter though because all types involved are PODs. Nevertheless, like Mike said, we should fix this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
decoder asked me to make the bug public.
Group: core-security
Assignee: nobody → amarchesini
Priority: -- → P2
Want me to find a new owner for this bug?
Flags: needinfo?(amarchesini)
Ah! I completely ignored this bug. I don't receive notifications when a bug is assigned to me. Sorry for this delay.
I work on it.
Flags: needinfo?(amarchesini)
Attached patch allocation.patchSplinter Review
Attachment #8974268 - Flags: review?(choller)
Attachment #8974268 - Flags: review?(choller) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b78c2999400
Use malloc() instead of new[] in IDBObjectStorage to match the use of free() in memory BlobImpl, r=decoder
https://hg.mozilla.org/mozilla-central/rev/8b78c2999400
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Minusing for a bounty because this can't be a problem in our codebase (not even the theoretically-possible mismatch that can come up in the case of using system NSPR since that's not involved here).
Flags: sec-bounty? → sec-bounty-
You need to log in before you can comment on or make changes to this bug.