1546327 - Bytecode length can overflow UINT32_MAX

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jan de Mooij [:jandem]]

Vector<> happily supports sizes > UINT32_MAX, but parts of the emitter and SharedScriptData::codeLength_ assume the length <= UINT32_MAX. There also seem to be issues with jump offsets being signed so maybe bytecode length should be limited to INT32_MAX.

The testcase below crashes 64-bit opt builds and fails a JumpList assertion in debug builds. Test runs for 20-30 seconds in opt builds but I'm sure it could be "optimized" to crash faster.

One way to fix this without an extra check everywhere is with a custom AllocPolicy, like this: https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/js/src/jit/x86-shared/AssemblerBuffer-x86-shared.h#78

---

function f() {
    var prefix = "throw 1;";
    var suffix = "0";
    var s = "[...[]],".repeat(50000000);
    Function(prefix + s + suffix)();
}
f();

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)

There also seem to be issues with jump offsets being signed so maybe bytecode length should be limited to INT32_MAX.

Debug builds fail the delta < 0 assertion here:

https://searchfox.org/mozilla-central/rev/f46e2bf881d522a440b30cbf5cf8d76fc212eaf4/js/src/frontend/JumpList.cpp#25

Probably safest to limit the bytecode Vector length to INT32_MAX.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #0)

One way to fix this without an extra check everywhere is with a custom AllocPolicy, like this

A problem with this is that it uses allocation size, not length, so the max bytecode length would depend on Vector's over-allocation/doubling behavior and that doesn't seem ideal. For now it's probably safest to restrict bytecode and source note Vectors to INT32_MAX in emitCheck/AllocSrcNote.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1546327 - Clean up bytecode and source note allocation. r?arai!

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 9060059 [details]
Bug 1546327 - Clean up bytecode and source note allocation. r?arai!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not trivial but probably not too hard either.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: All
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply or be easy to backport.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. It adds some size limits that we shouldn't reach on real websites.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Just realized jandem already said all branches are affected.

Comment 6

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

Comment on attachment 9060059 [details]
Bug 1546327 - Clean up bytecode and source note allocation. r?arai!

Sec-approval+ for trunk after talking to Liz in release management. We'll want a Beta and ESR60 patch nominated as well.

Comment 7

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/c89cf3ac460f35aa4c0516410c25e88e0f6cc8e6

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/c89cf3ac460f

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 9060059 [details]
Bug 1546327 - Clean up bytecode and source note allocation. r?arai!

Beta/Release Uplift Approval Request

• User impact if declined: Security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• 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): It adds some limits that shouldn't affect real websites.
• String changes made/needed: None

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9060059 [details]
Bug 1546327 - Clean up bytecode and source note allocation. r?arai!

Fix for sec-high issue, let's take this for beta 16 and for esr 60.7.

Comment 11

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

Patch fails to apply, code has been altered by bug 1535994 and bug 1543211 before (checked on beta). Please provide patches for beta and esr.

Comment 12

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/586c8f0a3c2d5fda683fba7700c526735091d303

Comment 13

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/a096a8ee118db4fcd39b7dbf4e963c96a795bb4b

I left out the asserts in JSScript.cpp in the ESR60 patch: I added them originally to catch regressions in mozilla-central but the code on ESR60 is very different and I don't think we need those sanity checks there.