Christian Holler (:decoder) Reporter
	Description • 6 years ago

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.

	Fuzzing Team
	Comment 1 • 6 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 2 • 6 years ago

Duh, multiplication overflow here results in max value equal to 0. I've got a patch, but need to better evaluate security risk here.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 3 • 6 years ago

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).

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 4 • 6 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 5 • 6 years ago

/me forgot to update to tip

	Jan de Mooij [:jandem]
	Comment 6 • 6 years ago

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 :)

	Julian Seward [:jseward]
	Comment 7 • 6 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 8 • 6 years ago

With assertions, carrying forward r+ from jseward.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 9 • 6 years ago

(without typo in summary, sorry for the bug spam)

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 10 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 11 • 6 years ago

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.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 12 • 6 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad1f8237199ca9d233d3c6a3a48c2a9c975723d

	Fuzzing Team
	Comment 13 • 6 years ago

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 9849ea3937e2).

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 14 • 6 years ago

https://hg.mozilla.org/mozilla-central/rev/7ad1f8237199

	Fuzzing Team
	Comment 15 • 6 years ago

JSBugMon: This bug has been automatically verified fixed.

	Benjamin Bouvier [:bbouvier] (inactive)
	Comment 16 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 6 years ago

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.