Closed Bug 1644839 Opened 4 years ago Closed 4 years ago

Assertion failure: target.isJumpTarget(), at vm/JSScript.cpp:4337 with parseModule and OOM

Categories

(Core :: JavaScript Engine, defect, P1)

x86_64
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 79+ fixed
firefox77 --- wontfix
firefox78 + wontfix
firefox79 + fixed
firefox80 + verified

People

(Reporter: decoder, Assigned: yulia)

References

(Regression)

Details

(4 keywords, Whiteboard: [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r][adv-ESR78.1+r])

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 20200610-590d76562067 (debug build, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off):

var code = `
  (\`\${key}: \${args[1]?.toString()}\`)
`;
oomTest(function() { return parseModule(code); });

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  0x0000555555c95ae5 in JSScript::assertValidJumpTargets() const ()
#1  0x0000555555c94b72 in JSScript::fullyInitFromStencil(JSContext*, js::frontend::CompilationInfo&, JS::Handle<JSScript*>, js::frontend::ScriptStencil&) ()
#2  0x0000555555c9602f in JSScript::fromStencil(JSContext*, js::frontend::CompilationInfo&, js::frontend::ScriptStencil&, js::SourceExtent) ()
#3  0x000055555618614f in js::frontend::CompilationInfo::instantiateStencils() ()
#4  0x00005555561c6589 in js::frontend::ModuleCompiler<char16_t>::compile(js::frontend::CompilationInfo&) ()
#5  0x000055555618f23e in js::frontend::ParseModule(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceText<char16_t>&, js::ScriptSourceObject**) ()
#6  0x000055555618fcd2 in js::frontend::CompileModule(JSContext*, JS::ReadOnlyCompileOptions const&, JS::SourceText<char16_t>&) ()
#7  0x00005555557da94d in ParseModule(JSContext*, unsigned int, JS::Value*) ()
#8  0x000038bef68cd32f in ?? ()
[...]
#12 0x0000000000000000 in ?? ()
rax	0x5555570111a2	93825020268962
rbx	0x394f549d1f0	3938305298928
rcx	0x55555836b980	93825040562560
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffff8fe0	140737488326624
rsp	0x7fffffff8f50	140737488326480
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f9bd40	140737353727296
r10	0x58	88
r11	0x7ffff6dac7a0	140737334921120
r12	0x394f547d100	3938305167616
r13	0x394f549d1f0	3938305298928
r14	0x394f549d1f0	3938305298928
r15	0x394f549d1f0	3938305298928
rip	0x555555c95ae5 <JSScript::assertValidJumpTargets() const+2117>
=> 0x555555c95ae5 <_ZNK8JSScript22assertValidJumpTargetsEv+2117>:	movl   $0x10f1,0x0
   0x555555c95af0 <_ZNK8JSScript22assertValidJumpTargetsEv+2128>:	callq  0x55555583f74e <abort>

Marking s-s because this looks like a scary assertion and OOM conditions are still considered exploitable.

Attached file Testcase (deleted) —

Is it possible to bisect this automatically? :)

Flags: needinfo?(choller)
Whiteboard: [bugmon:update,bisect] → [bugmon:update,bisected,confirmed]
Bugmon Analysis: Verified bug as reproducible on mozilla-central 20200611093454-10ad7868f3ca. The bug appears to have been introduced in the following build range: > Start: 93624f390bb7f536a0a07adcc977054faf42f094 (20200120163511) > End: 5d3392ccb733c35bc89b75949e58a2c79beac740 (20200120163646) > Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=93624f390bb7f536a0a07adcc977054faf42f094&tochange=5d3392ccb733c35bc89b75949e58a2c79beac740

Yulia, based on comment 3 it seems to be a bug introduced by one of your patches, can you investigate?

Severity: critical → S4
Flags: needinfo?(ystartsev)
Priority: -- → P1

Yep, on it.

Flags: needinfo?(ystartsev)
Assignee: nobody → ystartsev
Keywords: sec-high
Flags: needinfo?(choller)
Attachment #9156694 - Attachment description: Bug 1644839 - correct return value of emitOptionalChain on failure; r=jandem → Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem
Has Regression Range: --- → yes

BTW, S4 severity for a sec-high doesn't seem right to me.

Attachment #9156694 - Attachment description: Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem → Bug 1644839 - correct return value of emitOptionalChain on failure; r=jandem
Severity: S4 → S2
Attachment #9156694 - Attachment description: Bug 1644839 - correct return value of emitOptionalChain on failure; r=jandem → Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem
Attachment #9156694 - Attachment description: Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem → Bug 1644839 - correct return value of emitOptionalChain on failure; r=jandem
Attachment #9156694 - Attachment description: Bug 1644839 - correct return value of emitOptionalChain on failure; r=jandem → Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem
Flags: in-testsuite?

Comment on attachment 9156694 [details]
Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: They would need to trigger a specific OOM, I am not sure exactly how. It was caught by the oomTest.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
  • Which older supported branches are affected by this flaw?:
  • If not all supported branches, which bug introduced the flaw?: Bug 1566143
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: They would be easy to do.
  • How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause regressions.
Attachment #9156694 - Flags: sec-approval?

Given that we're already in RC week for 78 and this isn't a new issue, I think we should aim to ship this fix in 79/78.1esr.

Attachment #9156694 - Flags: sec-approval? → sec-approval+
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Status: RESOLVED → VERIFIED
Keywords: bugmon
Bugmon Analysis: Verified bug as fixed on rev mozilla-central 20200630020930-79d69f36a220. Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Flags: needinfo?(ystartsev)
Whiteboard: [bugmon:update,bisected,confirmed] → [bugmon:update,bisected,confirmed][sec-survey]

Please nominate this patch for Beta and ESR78 approval when you get a chance. It grafts cleanly to both as-landed.

Flags: needinfo?(ystartsev)

Comment on attachment 9156694 [details]
Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem

Beta/Release Uplift Approval Request

  • User impact if declined: Potential security issue that can be exposed via OOM
  • 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): This is a very minor change, and is only visible in an OOM case. There is a chance this is risky if it isn't uplifted.
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: OOM flaw
  • Fix Landed on Version: 79
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a very minor change, and is only visible in an OOM case. There is a chance this is risky if it isn't uplifted.
  • String or UUID changes made by this patch:
Attachment #9156694 - Flags: approval-mozilla-esr78?
Attachment #9156694 - Flags: approval-mozilla-beta?
Attachment #9156922 - Flags: approval-mozilla-beta?

Comment on attachment 9156922 [details]
Bug 1644839 - add oomTest for optionalChain bug; r=jandem

we're not landing the test just yet

Attachment #9156922 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ah, sorry I didn't see it set it for the test as well. I hadn't set it for that patch :/ thanks for catching it. I am not sure how it happened.

Comment on attachment 9156694 [details]
Bug 1644839 - fix return value of emitOptionalChain on failure; r=jandem

Approved for 79.0b5 and 78.1esr.

Attachment #9156694 - Flags: approval-mozilla-esr78?
Attachment #9156694 - Flags: approval-mozilla-esr78+
Attachment #9156694 - Flags: approval-mozilla-beta?
Attachment #9156694 - Flags: approval-mozilla-beta+
Whiteboard: [bugmon:update,bisected,confirmed][sec-survey] → [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r]
Whiteboard: [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r] → [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r][adv-ESR78.1+]
Whiteboard: [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r][adv-ESR78.1+] → [bugmon:update,bisected,confirmed][sec-survey][adv-main79+r][adv-ESR78.1+r]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: