1460833 - [BinAST] AddressSanitizer: stack-buffer-underflow [@ js::frontend::FunctionBox::function] with READ of size 8

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

The attached testcase crashes on mozilla-central revision ba62c1380491+ (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --enable-tests --enable-fuzzing --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2, run using FUZZER=BinAST ./fuzz-tests test.bin).

Backtrace:

==3152==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fff76389ba8 at pc 0x0000006bf10e bp 0x7fff76389030 sp 0x7fff76389028
READ of size 8 at 0x7fff76389ba8 thread T0
    #0 0x6bf10d in js::frontend::FunctionBox::function() const js/src/frontend/SharedContext.h:425:44
    #1 0x6bf10d in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseAndUpdateCapturedNames(js::frontend::BinKind) js/src/frontend/BinSource.cpp:238
    #2 0x651f1e in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseInterfaceAssertedBlockScope(unsigned long, js::frontend::BinKind, js::frontend::BinTokenReaderMultipart::BinFields const&) js/src/frontend/BinSource-auto.cpp:3046:5
    #3 0x6639aa in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseOptionalAssertedBlockScope() js/src/frontend/BinSource-auto.cpp:8233:9
    #4 0x64381d in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseInterfaceBlock(unsigned long, js::frontend::BinKind, js::frontend::BinTokenReaderMultipart::BinFields const&) js/src/frontend/BinSource-auto.cpp:3720:5
    #5 0x635b55 in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseStatement() js/src/frontend/BinSource-auto.cpp:2579:5
    #6 0x663d84 in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseListOfStatement() js/src/frontend/BinSource-auto.cpp:8171:9
    #7 0x650784 in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseInterfaceScript(unsigned long, js::frontend::BinKind, js::frontend::BinTokenReaderMultipart::BinFields const&) js/src/frontend/BinSource-auto.cpp:6460:5
    #8 0x6345b5 in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseSumProgram(unsigned long, js::frontend::BinKind, js::frontend::BinTokenReaderMultipart::BinFields const&) js/src/frontend/BinSource-auto.cpp:2228:9
    #9 0x6345b5 in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseProgram() js/src/frontend/BinSource-auto.cpp:2212
    #10 0x6b9c7f in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseAux(unsigned char const*, unsigned long) js/src/frontend/BinSource.cpp:115:5
    #11 0x6ba1df in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parse(unsigned char const*, unsigned long) js/src/frontend/BinSource.cpp:90:19
    #12 0x6ba1df in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parse(mozilla::Vector<unsigned char, 0ul, js::TempAllocPolicy> const&) js/src/frontend/BinSource.cpp:84
    #13 0x561118 in testBinASTReaderFuzz(unsigned char const*, unsigned long) js/src/fuzz-tests/testBinASTReader.cpp:62:29
    #14 0x613dbb in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) tools/fuzzing/libfuzzer/FuzzerLoop.cpp:517:13
[...]

Address 0x7fff76389ba8 is located in stack of thread T0 at offset 8 in frame
    #0 0x6b92ef in js::frontend::BinASTParser<js::frontend::BinTokenReaderMultipart>::parseAux(unsigned char const*, unsigned long) js/src/frontend/BinSource.cpp:98

  This frame has 4 object(s):
    [32, 88) 'globalsc'
    [128, 496) 'globalpc'
    [560, 616) 'varScope'
    [656, 672) 'bindings'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
SUMMARY: AddressSanitizer: stack-buffer-underflow js/src/frontend/SharedContext.h:425:44 in js::frontend::FunctionBox::function() const
Shadow bytes around the buggy address:
  0x10006ec69360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10006ec69370: 00 00 00 00 f1[f1]f1 f1 00 00 00 00 00 00 00 f2
  0x10006ec69380: f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00 00 00
  Addressable:           00
  Stack left redzone:      f1
  Stack mid redzone:       f2
==3152==ABORTING


Note that reproducing with the attached test requires not only an --enable-fuzzing shell build but also the patch from bug 1455593. Marking s-s because I don't know if there are any plans to enable BinJS in Nightly by default in the near future.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Mmmh... Given the stack, I suspect that we're not in a function at all and that we don't have a

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Given the stack I suspect that we're not in a function at all, so the `parseContext_->function()` returns something incorrect. In a DEBUG build, I imagine that this would have been caught by the MOZ_ASSERT above, which would have been better, but still indicates a way to remotely crash Firefox.

I believe that we should tweak parseAndUpdateCapturedNames to raise an error if we're called from the wrong context, instead of crashing.

Comment 4

[Christian Holler (:decoder)]

:arai, would you have time to look into this? Thanks!

Comment 5

[Tooru Fujisawa [:arai]]

out of curiosity, which format is this testcase using?  (this cannot be parsed by binjs-ref, and I even cannot locate BINJS header)
is this test about feeding corrupted file to reader?

Comment 6

[Tooru Fujisawa [:arai]]

oh... we're not testing the kind when parsing optional :P
https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/js/src/frontend/BinSource-auto.cpp#8230-8235

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Check interface when parsing field with single interface.

When parsing a field with multiple interfaces, we test the kind in parseSumInerface* methods.
When parsing a field with single interface, we didn't.
both for optional and non-optional cases.

Added type tests there and raise error if it's not expected interface.

then, how do we put this kind of testcase in-tree?

Comment 8

[Tooru Fujisawa [:arai]]

Comment on attachment 8988644 [details] [diff] [review]
Check interface when parsing field with single interface.

Clearing review flag for now.

this patch itself should be fine, but I guess the actual issue here is something different,
which still can be caused by tweaking the file in order to pass the check added by the patch,
given that this patch doesn't change the successful-case's path.
So this bug needs yet another fix.

I'll look into it.

Comment 9

[Tooru Fujisawa [:arai]]

for future reference, in order to effectively fuzz the fixed code, the check added by the above patch should be deactivated.
(it just checks the specific byte in the file has the expected value)

Comment 10

[Tooru Fujisawa [:arai]]

Comment on attachment 8988644 [details] [diff] [review]
Check interface when parsing field with single interface.

Figured out the issue, and it turns out that this patch properly fixed it.

the issue was the following:
  1. we didn't check the tuple tag (kind) for optional AssertedBlockScope,
     where the actual kind in the file is AssertedParameterScope
  2. we pass that kind (=BinKind::AssertedParameterScope) to parseAndUpdateCapturedNames
  3. parseAndUpdateCapturedNames assumes it's inside function

so, it's caused by the unchecked value flows into the branch's condition,
and checking the tuple tag fixes it.

Comment 11

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #9)
> for future reference, in order to effectively fuzz the fixed code, the check
> added by the above patch should be deactivated.
> (it just checks the specific byte in the file has the expected value)

so, this was wrong. and what speedups the fuzzing is, overwriting the kind variable with expected value, for single interface,
and randomly choose between the expected interface and _Null for optional field.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8988644 [details] [diff] [review]
Check interface when parsing field with single interface.

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

Good catch, thanks!

We don't have negative tests yet for BinAST, I'm afraid :/ But if you wish to add some, that would be quite nice.

Comment 13

[Tooru Fujisawa [:arai]]

Comment on attachment 8988644 [details] [diff] [review]
Check interface when parsing field with single interface.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This feature is disabled by default (dom.script_loader.binast_encoding.enabled),
and only a few people are supposed to enable this.
(people who's developing BinAST spec/impl)

If it's enabled, this can easily be exploited.

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

Yes, but not specifically what happens.

> Which older supported branches are affected by this flaw?

the feature was exposed to web in 62 (bug 1444956), but disabled by default.

> If not all supported branches, which bug introduced the flaw?

added by bug 1377007.
exposed by bug 1444956.

> Do you have backports for the affected branches? If not, how different,
> hard to create, and risky will they be?

Not yet, but easily.

> How likely is this patch to cause regressions; how much testing does it need?

Not likely.
Existing automated tests should be fine.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.

Comment 15

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13ec4ae4de5dcc95ee1fd0f5c016493c2243f7d2
Bug 1460833 - Check interface when parsing field with single interface. r=Yoric

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/13ec4ae4de5d