1431077 - Controllable large allocation in BinTokenReaderTester

Status: RESOLVED DUPLICATE of bug 1434573

(Core :: JavaScript Engine, defect, P3)

Description

[Christian Holler (:decoder)]

The attached testcase causes a large (controllable) allocation on mozilla-central revision 22130a72837b when run through the BinJS parse API.


Allocation Backtrace:

    #0 0x67a6e2 in void mozilla::detail::VectorImpl<js::frontend::BinField, 8ul, js::TempAllocPolicy, false>::new_<js::frontend::BinField>(js::frontend::BinField*, js::frontend::BinField&&) js/src/opt64lf/dist/include/mozilla/Vector.h:66:5
    #1 0x67a6e2 in void mozilla::detail::VectorImpl<js::frontend::BinField, 8ul, js::TempAllocPolicy, false>::moveConstruct<js::frontend::BinField>(js::frontend::BinField*, js::frontend::BinField*, js::frontend::BinField*) js/src/opt64lf/dist/include/mozilla/Vector.h:110
    #2 0x67a6e2 in mozilla::Vector<js::frontend::BinField, 8ul, js::TempAllocPolicy>::convertToHeapStorage(unsigned long) js/src/opt64lf/dist/include/mozilla/Vector.h:965
    #3 0x6662d7 in mozilla::Vector<js::frontend::BinField, 8ul, js::TempAllocPolicy>::reserve(unsigned long) js/src/opt64lf/dist/include/mozilla/Vector.h:1112:9
    #4 0x6662d7 in js::frontend::BinTokenReaderTester::enterTaggedTuple(js::frontend::BinKind&, mozilla::Vector<js::frontend::BinField, 8ul, js::TempAllocPolicy>&, js::frontend::BinTokenReaderTester::AutoTaggedTuple&) js/src/frontend/BinTokenReaderTester.cpp:324
    #5 0x62f496 in js::frontend::BinASTParser::parseProgram() js/src/frontend/BinSource.cpp:196:5
    #6 0x62e006 in js::frontend::BinASTParser::parseAux(unsigned char const*, unsigned long) js/src/frontend/BinSource.cpp:179:5
    #7 0x62d87a in js::frontend::BinASTParser::parse(unsigned char const*, unsigned long) js/src/frontend/BinSource.cpp:156:19
    #8 0x62d87a in js::frontend::BinASTParser::parse(mozilla::Vector<unsigned char, 0ul, js::TempAllocPolicy> const&) js/src/frontend/BinSource.cpp:150
[...]

The problem is here:

https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/js/src/frontend/BinTokenReaderTester.cpp#324

In line 320, we read an uint32_t and in line 324, we directly use it to allocate memory.

To allow better automated testing and protect against easy denial-of-service attacks, we should have safety limits on fields like this. This should probably also be reflected in the specification somehow.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

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

Ah, good catch.
What's the policy on large allocations? Should I simply bound it?

While this specific tokenizer will not be part of the spec, as it's designed for debugging, the optimized one will also have a number of user-controllable allocations, including string size and several header-defined tables, so the problem will need to be solved before any release.

Comment 3

[Jason Orendorff [:jorendorff]]

I too would love to know what the policy on large allocations is. Maybe Christian can point us at something or someone.

Comment 4

[Jason Orendorff [:jorendorff]]

Also: David, would you mind translating "will need to be solved before" (comment 2) into some kind of Bugzilla metadata to help make sure this isn't forgotten?

Comment 5

[Christian Holler (:decoder)]

I don't know of any written policy, I can just tell you that

* We try to avoid easy/trivial forms of DoS in the browser in general, e.g. in image decoding, video stuff, etc.
* Allowing huge allocations that are completely unchecked make automated testing significantly harder (hence this bug)
* If this specification was ever to be adapted in another environment, outside the browser (e.g. in a server-side environment), the lack of such checks could be seen as a severe limitation.


My suggestion would be to apply sane limits to these values. Maybe :jandem can also say something about if there is any other such case in the JS engine.

Comment 6

[Jan de Mooij [:jandem]]

Controllable large allocations are scary also because one integer overflow and we have a sec-crit. So yes, if we can easily avoid this, let's do it.

Ideally we would have a policy where we restrict all allocations to max X MB and if we want to allow larger allocations somewhere (say ArrayBuffers) we could annotate that allocation site with some kind of RAII class.

Comment 7

[Jon Coppeard (:jonco)]

I don't have anything to add to this beyond what Jan said above.

Comment 8

[Tooru Fujisawa [:arai]]

this specific case is fixed by bug 1434573.
are we going to address more underlying issue here?
if not, I'd suggest closing it and filing dedicated bug for restricting the size.

Comment 9

[Tooru Fujisawa [:arai]]

closing, given comment #8.
also BinTokenReaderTester will be removed in bug 1515224