Closed Bug 1052582 Opened 10 years ago Closed 6 years ago

Store non-inline JS array buffer contents in their own malloc arena

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: benjamin, Assigned: molly)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

Bug 1052582 Part 1 - Support an arena parameter for js_pod_malloc and friends.
59 bytes, text/x-review-board-request
sfink
: review+
Details
Bug 1052582 Part 2 - Create and use a separate malloc arena for ArrayBuffer contents.
59 bytes, text/x-review-board-request
sfink
: review+
Details 
See bug 1052575 for overview: this bug tracks storing JS engine arraybuffers in the segregated data heap.
I started investigating this.  I'm not sure if this is correct, but I looked at Array Buffers and found 6 locations where we seem to be allocating data for them:


AllocateArrayBufferContents http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/js/src/vm/ArrayBufferObject.cpp#305
This seems like the most used vector. This goes into ZoneAllocPolicy and Zone, and eventually pod_calloc and moz_xcalloc: http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/memory/mozalloc/mozalloc.h#348

WasmArrayRawBuffer::Allocate http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/js/src/vm/ArrayBufferObject.cpp#623
WASM uses (on windows) VirtualAlloc and (elsewhere) mmap.  This leads me to believe this call site does not need to be changed, as we're not going to be able to allocate an arraybuffer over top any sort of DOM node with this method. I think.

ArrayBufferObject::createMappedContents http://searchfox.org/mozilla-central/rev/a984558fa2bbde6492d3fb918496fc0b0835b2ce/js/src/vm/ArrayBufferObject.cpp#828
This calls into AllocateMappedContent in Memory.cpp and eventually mmap. Same conclusion: don't need to do anything here.

JS_NewArrayBufferWithContents http://searchfox.org/mozilla-central/search?q=symbol:_Z29JS_NewArrayBufferWithContentsP9JSContextmPv&redirect=false
This is only used in the js shell and when a XMLHttpRequest's response type is of 'Moz_chunked_arraybuffer'. However, I cannot find anywhere this response type is actually set.

JS_NewArrayBufferWithExternalContents http://searchfox.org/mozilla-central/search?q=symbol:_Z37JS_NewArrayBufferWithExternalContentsP9JSContextmPv&redirect=false
This is only used in a jsapi-test

JS_NewMappedArrayBufferWithContents http://searchfox.org/mozilla-central/search?q=symbol:_Z35JS_NewMappedArrayBufferWithContentsP9JSContextmPv&redirect=false
This is used in:
 - FetchConsumer
 - Push Messages relating to Service Workers 
 - FileReader
 - IndexDB Cursors
 - Some stuff that appears related to IPC, in TCPSocket.cpp and TCPSocketChild.cpp
 - Alterately when a XMLHttpRequest's response type is of 'Moz_chunked_arraybuffer', instead of JS_NewArrayBufferWithContents
 - As part of the Streams spec, TransferArrayBuffer
 - In TestingFunctions.cpp, as part of getCloneBufferAsArrayBuffer
 - As part of the structured clone algorithm
 - As part of WebRTC packet dumping
 - As part of OS.File (in GetResult)
This seems like a tricky one to address, as most of these can be filled with attacker-controlled data. We'll need to investigate these more to see how the data gets allocated and if could be allocated in the same place as DOM objects.
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Note to reviewers: since this is my first SpiderMonkey patch, I want to say a) feel free to redirect if I haven't identified the best reviewers, and b) I will neither mind nor be surprised if this review turns into an ocean of red ink. Thanks!
Comment on attachment 8990404 [details]
Bug 1052582 Part 2 - Add dedicated AllocKinds just for ArrayBufferObjects.

https://reviewboard.mozilla.org/r/255492/#review262274


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: js/src/vm/ArrayBufferObject.cpp:1196
(Diff revision 1)
> +static inline AllocKind
> +GetArrayBufferGCObjectKind(size_t numSlots)
> +{
> +    if (numSlots <= 4) {
> +        return AllocKind::ARRAYBUFFER4;
> +    } else if (numSlots <= 8) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

    } else if (numSlots <= 8) {
      ^~~~~~~~
Comment on attachment 8990405 [details]
Bug 1052582 Part 1 - Support an arena parameter for js_pod_malloc and friends.

https://reviewboard.mozilla.org/r/255494/#review262276


Code analysis found 4 defects in this patch:
 - 1 defect found by clang-tidy
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/mochitest/leaks.py:186
(Diff revision 1)
>          self.maxNumRecordedFrames = 4
>  
>          # Don't various allocation-related stack frames, as they do not help much to
>          # distinguish different leaks.
>          unescapedSkipList = [
> -            "malloc", "js_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",
> +            "malloc", "js_malloc", "js_arena_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",

Error: Line too long (103 > 99 characters) [flake8: E501]

::: testing/mochitest/leaks.py:187
(Diff revision 1)
>  
>          # Don't various allocation-related stack frames, as they do not help much to
>          # distinguish different leaks.
>          unescapedSkipList = [
> -            "malloc", "js_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",
> -            "calloc", "js_calloc", "calloc_", "__interceptor_calloc", "moz_xcalloc",
> +            "malloc", "js_malloc", "js_arena_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",
> +            "calloc", "js_calloc", "js_arena_calloc", "calloc_", "__interceptor_calloc", "moz_xcalloc",

Error: Line too long (103 > 99 characters) [flake8: E501]

::: testing/mochitest/leaks.py:188
(Diff revision 1)
>          # Don't various allocation-related stack frames, as they do not help much to
>          # distinguish different leaks.
>          unescapedSkipList = [
> -            "malloc", "js_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",
> -            "calloc", "js_calloc", "calloc_", "__interceptor_calloc", "moz_xcalloc",
> -            "realloc", "js_realloc", "realloc_", "__interceptor_realloc", "moz_xrealloc",
> +            "malloc", "js_malloc", "js_arena_malloc", "malloc_", "__interceptor_malloc", "moz_xmalloc",
> +            "calloc", "js_calloc", "js_arena_calloc", "calloc_", "__interceptor_calloc", "moz_xcalloc",
> +            "realloc", "js_realloc", "js_arena_realloc", "realloc_", "__interceptor_realloc", "moz_xrealloc",

Error: Line too long (109 > 99 characters) [flake8: E501]
Comment on attachment 8990406 [details]
Bug 1052582 Part 2 - Create and use a separate malloc arena for ArrayBuffer contents.

https://reviewboard.mozilla.org/r/255496/#review262278


Code analysis found 4 defects in this patch:
 - 1 defect found by clang-tidy
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: js/src/vm/ArrayBufferObject.cpp:1197
(Diff revision 1)
>  static inline AllocKind
>  GetArrayBufferGCObjectKind(size_t numSlots)
>  {
>      if (numSlots <= 4) {
>          return AllocKind::ARRAYBUFFER4;
>      } else if (numSlots <= 8) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

    } else if (numSlots <= 8) {
      ^~~~~~~~
Comment on attachment 8990406 [details]
Bug 1052582 Part 2 - Create and use a separate malloc arena for ArrayBuffer contents.

https://reviewboard.mozilla.org/r/255496/#review262284

This patch seems good. I have questions/comments/concerns with the others.

::: dom/file/FileReaderSync.cpp:65
(Diff revision 2)
>    uint64_t blobSize = aBlob.GetSize(aRv);
>    if (NS_WARN_IF(aRv.Failed())) {
>      return;
>    }
>  
> -  UniquePtr<char[], JS::FreePolicy> bufferData(js_pod_malloc<char>(blobSize));
> +  UniquePtr<char[], JS::FreePolicy> bufferData(

It's not really that important, but it looks like this could avoid line wrapping by #including js/Utility.h and then using JS::UniqueChars here.
Attachment #8990406 - Flags: review?(sphink) → review+
Comment on attachment 8990404 [details]
Bug 1052582 Part 2 - Add dedicated AllocKinds just for ArrayBufferObjects.

https://reviewboard.mozilla.org/r/255492/#review262288

This increases fragmentation a bit and increases our minimum memory overhead, which is perhaps not an enormous cost. But it also doesn't seem like a huge benefit -- this segregates inline ArrayBuffer data into ArrayBuffer-specific arenas, which helps prevent ArrayBuffer overruns from accessing and potentially corrupting JSObject data from similar sized objects -- but the attacker would still have access to the ArrayBuffer metadata (including the length and offset, not to mention a data pointer, which means modifying it allows access anywhere in memory). So it doesn't seem worth much overhead to me.

If this is an important attack vector, then it seems like having inline data at all is a much bigger problem. (But simply removing inline data would be even more expensive in terms of performance.) If there is previous discussion about this that I'm missing that decided this was a good idea, please point me to it.

Allocating out of line ArrayBuffer data in a separate heap region *does* seem like a large security win to me, but that doesn't seem to require this patch as far as I can tell. (And without this patch, Part 1 is unnecessary, though it seems useful in its own right.)
Attachment #8990404 - Flags: review?(sphink)
Comment on attachment 8990404 [details]
Bug 1052582 Part 2 - Add dedicated AllocKinds just for ArrayBufferObjects.

https://reviewboard.mozilla.org/r/255492/#review262288

The security benefit of this patch is that it makes it more difficult for an attacker to gain the ability to write to the inline data by using some other exploit, like a more limited type of buffer overrun; it makes it less likely that such an exploit would be able to provide that access. I have to admit to not being familiar enough to fully understand the overheads involved, and I'm also not a security expert, so I can claim that this change is worth it, but not with a lot of confidence.

You're right that parts 1 and 2 strictly relate to the inline ArrayBuffer data, they're not needed for array contents. Maybe I could break these out into a separate bug, that we can discuss further and possibly end up WONTFIXing? I'm kind of stretching the intent of this one anyway.
(In reply to Steve Fink [:sfink] [:s:] from comment #18)
> but the attacker would still have access to the ArrayBuffer
> metadata (including the length and offset, not to mention a data pointer,
> which means modifying it allows access anywhere in memory). So it doesn't
> seem worth much overhead to me.
> 
> If this is an important attack vector, then it seems like having inline data
> at all is a much bigger problem. (But simply removing inline data would be
> even more expensive in terms of performance.) If there is previous
> discussion about this that I'm missing that decided this was a good idea,
> please point me to it.


I can confirm that short overwrite vulnerabilities to edit the length of an ArrayBuffer are extremely common. They're practically the playbook for most exploits for the past half a decade. So moving the array metadata away from array contents (effectively eliminating inline data) is ideal. Before that is WONTFIXed I'd want to see what we can do about the memory costs - and we can also review how common it is to have vulnerabilities that cause overwites or UAFs with ArrayBuffers themselves.


But although I'd love to see inline ArrayBuffers eliminated; moving the inline ArrayBuffers into their own arena is _definitely_ useful even if the metadata is next to it. It would prevent attacks from using UAFs or short overwrite vulnerabilities in other JSObjects from being able to manipulate ArrayBuffers.
Attachment #8990403 - Attachment is obsolete: true
Attachment #8990403 - Flags: review?(jwalden+bmo)
Attachment #8990404 - Attachment is obsolete: true
Comment on attachment 8990405 [details]
Bug 1052582 Part 1 - Support an arena parameter for js_pod_malloc and friends.

https://reviewboard.mozilla.org/r/255494/#review263262

I'm kind of tempted to ask for arena_id_t to be a class enum or something that *can* be overloaded on -- or better yet, require the arena parameter for the whole js_*alloc family of functions, so that the caller has to think about the arena. But for now, this seems to be the path of least resistance, and I wouldn't want to hold up the heap partitioning for that change.
Attachment #8990405 - Flags: review?(sphink) → review+
Comment on attachment 8990406 [details]
Bug 1052582 Part 2 - Create and use a separate malloc arena for ArrayBuffer contents.

https://reviewboard.mozilla.org/r/255496/#review262284

> It's not really that important, but it looks like this could avoid line wrapping by #including js/Utility.h and then using JS::UniqueChars here.

That gets the line down to 93 characters, so it really probably still needs to wrap. I would say that keeps this change from being worth it, unfortunately.
Pushed by mhowell@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b6b0d0fc267d
Part 1 - Support an arena parameter for js_pod_malloc and friends. r=sfink
https://hg.mozilla.org/integration/autoland/rev/1f5426580dec
Part 2 - Create and use a separate malloc arena for ArrayBuffer contents. r=sfink
https://hg.mozilla.org/mozilla-central/rev/b6b0d0fc267d
https://hg.mozilla.org/mozilla-central/rev/1f5426580dec
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1475544
Rewriting the summary to better describe what this bug ended up doing.
Summary: Store JS array buffers in the data heap → Store non-inline JS array buffer contents in their own malloc arena
Depends on: 1486444
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: