1620290 - Assertion failure: (static_cast<uint8_t>(utf8Chars[0]) & 0b1110'0000) == 0b1100'0000 (expected length-2 leading UTF-8 unit), at vm/JSAtom.cpp:1061 with WebAssembly

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20200305-2e1a978b09d7 (build with --enable-debug, run with --fuzzing-safe --no-threads):

load("wasm-module-builder.js");
var module = new WasmModuleBuilder();
module.addMemory();
module.addFunction([-5.2, -91.2], kSig_v_v).addBody([
  undefined | false,
  kExprI32StoreMem, 0, 0xFF, 0xFF, 0x7A
]).exportAs("main");
var instance = module.instantiate();
instance.exports.main();

Backtrace:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x000055dac662ced6 in JSAtom* AtomizeUTF8OrWTF8Chars<JS::UTF8Chars>(JSContext*, char const*, unsigned long) ()
#1  0x000055dac727120c in js::wasm::Instance::getFuncDisplayAtom(JSContext*, unsigned int) const ()
#2  0x000055dac7270f00 in js::wasm::WasmFrameIter::functionDisplayAtom() const ()
#3  0x000055dac6738f20 in js::SavedStacks::insertFrames(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#4  0x000055dac673877b in js::SavedStacks::saveCurrentStack(JSContext*, JS::MutableHandle<js::SavedFrame*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#5  0x000055dac6467621 in JS::CaptureCurrentStack(JSContext*, JS::MutableHandle<JSObject*>, mozilla::Variant<JS::AllFrames, JS::MaxFrames, JS::FirstSubsumedFrame>&&) ()
#6  0x000055dac646c0d5 in js::ErrorToException(JSContext*, JSErrorReport*, JSErrorFormatString const* (*)(void*, unsigned int), void*) ()
#7  0x000055dac66334d2 in js::ReportErrorNumberVA(JSContext*, unsigned int, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, js::ErrorArgumentsType, __va_list_tag*) ()
#8  0x000055dac64456cf in JS_ReportErrorNumberUTF8(JSContext*, JSErrorFormatString const* (*)(void*, unsigned int), void*, unsigned int, ...) ()
#9  0x000055dac71f7a2f in WasmHandleTrap() ()
#10 0x00001dfc51300208 in ?? ()
#11 0x0000000000000000 in ?? ()
rax	0x55dac78e64fd	94398139229437
rbx	0x7f0dd1727000	139697325240320
rcx	0x55dac892a850	94398156286032
rdx	0x0	0
rsi	0x7f0dd27e0770	139697342777200
rdi	0x7f0dd27df540	139697342772544
rbp	0x7fff2fb16fc0	140733993545664
rsp	0x7fff2fb16e40	140733993545280
r8	0x7f0dd27e0770	139697342777200
r9	0x7f0dd3883d00	139697360223488
r10	0x58	88
r11	0x7f0dd24877a0	139697339266976
r12	0x7f0dd1727000	139697325240320
r13	0x7f0dd173f0a0	139697325338784
r14	0x2	2
r15	0x7f0dd1727000	139697325240320
rip	0x55dac662ced6 <JSAtom* AtomizeUTF8OrWTF8Chars<JS::UTF8Chars>(JSContext*, char const*, unsigned long)+2294>
=> 0x55dac662ced6 <_Z22AtomizeUTF8OrWTF8CharsIN2JS9UTF8CharsEEP6JSAtomP9JSContextPKcm+2294>:	movl   $0x425,0x0
   0x55dac662cee1 <_Z22AtomizeUTF8OrWTF8CharsIN2JS9UTF8CharsEEP6JSAtomP9JSContextPKcm+2305>:	callq  0x55dac622505e <abort>

Marking s-s because it is not immediately clear to me what the consequences of violating that assert would be.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Christian Holler (:decoder)]

Since the testcase requires the wasm module builder, here is a full test.zip for automation.

Comment 3

[Christian Holler (:decoder)]

Attachment: Testcase for comment 2

Comment 4

[Lars T Hansen [:lth]]

Pretty clear-cut case of failing to sanitize input, judging from the code. The wasm code appears to assume either that the input is for sure valid, but it does not check that, or that the Atomize code will perform sanitazion. There might be a similar problem with the module name.

I need to figure out what the correct semantics are. It may be that we're not allowed to error out early if the name section is malformed.

Comment 5

[Lars T Hansen [:lth]]

I don't think this is exploitable. What happens is that we do a quick check for a fast case, and this is where the assertions are. If the quick check fails, we fall through into general code that has correct error checking.

This fastpath was added recently by waldo:

changeset:   489685:0a25146a7c1a
user:        Jeff Walden <[e-mail]>
date:        Sat Aug 24 03:05:54 2019 +0000
summary:     Bug 1575947 - AtomizeUTF8OrWTF8Chars should look up non-ASCII, Latin-1 single-code-point static strings itself.  (Static strings are not recorded as permanent atoms or in the normal atom table, so it's always necessary to specially look them up.)... r=jonco

At first blush I'd say that the fastpath needs to handle errors properly, probably by falling through to the common case that can signal the errors.

Comment 6

[Steven DeTar [:sdetar]]

Waldo, can you look at this based on Lar's comments?

Comment 7

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20200314214233-23c4c99d0c1f.
The bug appears to have been introduced in the following build range:
> Start: 9f96b6821f1dfe383b34a190df239dfe2463339e (20190823214900)
> End: ea3eebc73ccb07ce574044b7063b95a309fc0933 (20190824094946)
> Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9f96b6821f1dfe383b34a190df239dfe2463339e&tochange=ea3eebc73ccb07ce574044b7063b95a309fc0933

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Bug 1620290 - |AtomizeUTF8Chars| and |AtomizeWTF8Chars| should handle two-unit, single-code-point inputs better. r=arai!

Comment 9

[Jeff Walden [:Waldo]]

The consequences of this bug do not allow you to do anything you couldn't already do, validly -- namely, any invalid two-byte UTF-8 that could be misprocessed here, would just map those two bytes to a string in the range "\x00" to "\xFF", which you could just as well have encoded properly.

However, there is a very remote possible concern that if the server-side generator is depending upon an error being reported for invalid input, an unexpected string being the result could lead things awry. This isn't terribly different than, hypothetically, any site relying on buggy implementation behavior to control some sort of site functionality, resulting in XSS under the right conditions. So if it were just me, I wouldn't worry and would probably just open.

But, others want to be more paranoid, and I think we probably should indulge such feelings when it comes to security rather than downplay them, so better safe than sorry.

Comment 10

[Jeff Walden [:Waldo]]

That said -- given the low risks, even if I'm going to leave this bug hidden, I think the sec-approval process is not necessary here, and I'm going to land this now.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d14855885ec7
https://hg.mozilla.org/integration/autoland/rev/d14855885ec7b42aeab1cc743d057a4c608eb5d7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 9133422 [details]
Bug 1620290 - |AtomizeUTF8Chars| and |AtomizeWTF8Chars| should handle two-unit, single-code-point inputs better. r=arai!

Beta/Release Uplift Approval Request

• User impact if declined: A very few places that need to create atoms from un-validated UTF-8 data will return atoms for them rather than throw exceptions. The atoms that are returned are structurally valid, and they could just have been properly encoded, so no new (and undesirable) functionality is introduced by the error.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch includes an automated test that exhaustively tests atomization behavior for every two-byte sequence, verifying that each attempt throws or produces the appropriate atom. If I run that test against the code without the patch, I can #if 0 out each call I expected would fail without the patch til I'm left only with calls that pass. All those calls are the expected behavior. It doesn't get any better-tested than this, I'd say.
• String changes made/needed:

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 9133422 [details]
Bug 1620290 - |AtomizeUTF8Chars| and |AtomizeWTF8Chars| should handle two-unit, single-code-point inputs better. r=arai!

approved for 75.0b6

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/35469403809d

Comment 16

[Jason Kratzer [:jkratzer]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20200422214848-17aa41e3cb7c.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.