Closed Bug 1472639 Opened 2 years ago Closed 2 years ago

Assertion failure: numBytes <= maxSize.valueOr((4294967295U)), at js/src/vm/ArrayBufferObject.cpp:766 with wasm

Categories

(Core :: Javascript: WebAssembly, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- verified

People

(Reporter: decoder, Assigned: bbouvier)

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main63+])

Attachments

(1 file, 3 obsolete files)

The following testcase crashes on mozilla-central revision 23885c14f025 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

let mem = new WebAssembly.Memory({
    initial: 1,
    maximum: 65536
});


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000000000abb912 in js::WasmArrayRawBuffer::Allocate (numBytes=numBytes@entry=65536, maxSize=...) at js/src/vm/ArrayBufferObject.cpp:766
#0  0x0000000000abb912 in js::WasmArrayRawBuffer::Allocate (numBytes=numBytes@entry=65536, maxSize=...) at js/src/vm/ArrayBufferObject.cpp:766
#1  0x0000000000ac9b02 in CreateBuffer<js::ArrayBufferObject, js::WasmArrayRawBuffer> (maybeSharedObject=..., maxSize=..., initialSize=65536, cx=0x7ffff5f17000) at js/src/vm/ArrayBufferObject.cpp:809
#2  js::CreateWasmBuffer (cx=0x7ffff5f17000, memory=..., buffer=buffer@entry=...) at js/src/vm/ArrayBufferObject.cpp:909
#3  0x0000000000e37d80 in js::WasmMemoryObject::construct (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/wasm/WasmJS.cpp:1522
#4  0x00000000005ba1f7 in CallJSNative (cx=0x7ffff5f17000, native=native@entry=0xe37ba0 <js::WasmMemoryObject::construct(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/vm/Interpreter.cpp:443
[...]
#16 0x0000000000443815 in Shell (envp=<optimized out>, op=0x7fffffffda20, cx=<optimized out>) at js/src/shell/js.cpp:8983
#17 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9461
rax	0x0	0
rbx	0x10000	65536
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffc8f0	140737488341232
rsp	0x7fffffffc8d0	140737488341200
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x7fffffffc900	140737488341248
r13	0x7ffff5f17000	140737319628800
r14	0x7fffffffc9b0	140737488341424
r15	0x7fffffffc9f8	140737488341496
rip	0xabb912 <js::WasmArrayRawBuffer::Allocate(unsigned int, mozilla::Maybe<unsigned int> const&)+258>
=> 0xabb912 <js::WasmArrayRawBuffer::Allocate(unsigned int, mozilla::Maybe<unsigned int> const&)+258>:	movl   $0x0,0x0
   0xabb91d <js::WasmArrayRawBuffer::Allocate(unsigned int, mozilla::Maybe<unsigned int> const&)+269>:	ud2


Marking s-s because I don't know what happens if we exceed that maximum size specified in the assertion.
Component: JavaScript Engine → Javascript: Web Assembly
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/27a355cf6036
parent:      361999:5f944d9ffbe0
user:        Benjamin Bouvier
date:        Wed May 31 17:23:12 2017 +0200
summary:     Bug 1368844: Check WebAssembly.{Memory,Table} sizes against internal limits; r=luke

This iteration took 282.950 seconds to run.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Duh, multiplication overflow here results in max value equal to 0. I've got a patch, but need to better evaluate security risk here.
Attached patch fix.patch (obsolete) — Splinter Review
Easy fix: we have logic in DecodeLimits (called when creating a module that defines or imports a memory) to clamp the max memory size when it's too high, but this logic didn't exist in WasmMemoryObject::construct (called when a Memory object is created), so I factored out the clamping chunk into a function that gets called in WasmMemoryObject::construct.

Jan, I wanted to flag Julian as reviewer, but he's not in one of the sec lists. The patch is just a small refactoring, but let me know if you'd prefer somebody else in the wasm team to review it (luke/lars being on pto for some time).
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8989814 - Flags: review?(jdemooij)
For what it's worth, the security risk is ultra low:

- on WASM_HUGE_MEMORY platforms (i.e. where we use the signal tricks for memory accesses, viz. x64), the buffer will record a maximum memory size of 0, which isn't affecting anything, except when we try to grow the memory. In which case, it will just fail all the time, because any new size would be bigger. (note it generalizes nicely to other values of max that are < initial size, so we're safe on this side.
- on non-WASM_HUGE_MEMORY platforms (x86 / ARM), another assertion triggers later when allocating the array. If this assertion is removed (to see what's the worst that could happen), then the mprotect/second VirtualAlloc call will fail, and trigger a user-visible OOM.

So in the worst case, it's just unexpected behavior, no crashes, and I think no potential for exploitation.
Attached patch fix.patch (obsolete) — Splinter Review
/me forgot to update to tip
Attachment #8989814 - Attachment is obsolete: true
Attachment #8989814 - Flags: review?(jdemooij)
Attachment #8989816 - Flags: review?(jdemooij)
Comment on attachment 8989816 [details] [diff] [review]
fix.patch

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

Good catch. Looks fine but I'll forward to jseward just to be sure :)
Attachment #8989816 - Flags: review?(jdemooij) → review?(jseward)
Comment on attachment 8989816 [details] [diff] [review]
fix.patch

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

::: js/src/wasm/WasmValidate.cpp
@@ +1430,5 @@
> +void
> +wasm::ConvertMemoryPagesToBytes(Limits* memory)
> +{
> +    CheckedInt<uint32_t> initialBytes = memory->initial;
> +    initialBytes *= PageSize;

Please add comment/assertions explaining why this multiplication can never overflow.
Attachment #8989816 - Flags: review?(jseward) → review+
Attached patch fix.patch (obsolete) — Splinter Review
With assertions, carrying forward r+ from jseward.
Attachment #8989816 - Attachment is obsolete: true
Attachment #8990028 - Flags: review+
Attached patch fix.patchSplinter Review
(without typo in summary, sorry for the bug spam)
Attachment #8990028 - Attachment is obsolete: true
Attachment #8990029 - Flags: review+
Comment on attachment 8990029 [details] [diff] [review]
fix.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I don't think we can make one, see comment 4 for the worst that could happen. It will always result in either a safe crash situation (because of the MOZ_RELEASE_ASSERT), or unexpected errors (as detailed in comment 4).

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

Which older supported branches are affected by this flaw? I think all the branches since WasmMemoryObject was introduced. (probably bug 1284155)

If not all supported branches, which bug introduced the flaw?
(probably bug 1284155)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't have them, but they shouldn't be too hard. There's a small very-low risk refactoring in this patch, that could be avoided on other branches if needed, but it's not doing much. So branch refactorings could be easily done, with a very-low risk.

How likely is this patch to cause regressions; how much testing does it need?
Local testing on a WASM_HUGE_MEMORY platform and a non-WASM_HUGE_PLATFORM have been made, and all the tests passed. It won't cause performance regressions, and it's being more constraining than what was there before, so it is very not likely to cause any kind of regression.
Attachment #8990029 - Flags: sec-approval?
Keywords: sec-low
Comment on attachment 8990029 [details] [diff] [review]
fix.patch

Clearly sec-approval since Dan made this a sec-low issue. This can just go into trunk but will probably not be backported.
Attachment #8990029 - Flags: sec-approval?
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9849ea3937e2).
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
https://hg.mozilla.org/mozilla-central/rev/7ad1f8237199
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Flags: needinfo?(bbouvier)
Flags: in-testsuite+
Ryan, did you have a specific question in mind with this needinfo?

Since the patch has landed on Nightly, I suspect it might be about backporting; comment 11 says we should not backport. The patch is small enough and very low risk, so it could easily be backported, if we were to change our minds. That being said, it fixes a benign issue which will just safely crash or trigger unexpected errors in opt builds, so it doesn't look too high priority.
Flags: needinfo?(bbouvier) → needinfo?(ryanvm)
Sorry, that NI was accidental on my part. Given comment 11, I had assumed we didn't want to backport. If you want to do so, feel free to set 62 back to affected and nominate the patch.
Flags: needinfo?(ryanvm)
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main63+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.