Closed Bug 2077386 Opened 1 month ago Closed 1 month ago

Ion throws for a WebAssembly i64 call in a zero-iteration loop

Categories

(Core :: JavaScript Engine: JIT, defect, P2)

defect

Tracking

()

RESOLVED FIXED
159 Branch
Tracking Status
firefox-esr115 --- wontfix
firefox-esr153 --- wontfix
firefox157 --- wontfix
firefox158 --- wontfix
firefox159 + fixed

People

(Reporter: bugmon, Assigned: iain)

References

(Blocks 1 open bug)

Details

(Keywords: testcase)

Attachments

(4 files, 1 external file)

MToInt64 marks potentially throwing string-to-i64 conversions movable and reports AliasSet::None() in js/src/jit/MIR.h:3845. LICM consequently permits moving the conversion into a loop preheader. The generated code calls DoStringToInt64, which reports invalid BigInt syntax rather than bailing out, allowing an exception to occur even when the loop body never executes.

A web-content author can interrupt JavaScript execution by supplying an invalid BigInt string to an Ion-optimized function whose WebAssembly i64 call should be skipped. On the tested 64-bit configuration, the function throws SyntaxError instead of returning 0n. No memory corruption or privilege escalation was demonstrated.

This finding was discovered during variant analysis seeded by bug 2074632.

Build Info

Affected Code

File: js/src/jit/CacheIR.cpp, line 13143-13147

case wasm::ValType::I64:
  if (!argVal.isBigInt() && !argVal.isBoolean() && !argVal.isString()) {
    return AttachDecision::NoAction;
  }
  break;

File: js/src/jit/WarpCacheIRTranspiler.cpp, line 6993-6995

case wasm::ValType::I64:
  conversion = MToInt64::New(alloc(), arg);
  break;

File: js/src/jit/MIR.h, line 3844-3864

class MToInt64 : public MUnaryInstruction, public ToInt64Policy::Data {
  explicit MToInt64(MDefinition* def) : MUnaryInstruction(classOpcode, def) {
    setResultType(MIRType::Int64);
    setMovable();

    // Guard unless the conversion is known to be non-effectful & non-throwing.
    if (!def->typeIsOneOf(
            {MIRType::Boolean, MIRType::BigInt, MIRType::Int64})) {
      setGuard();
    }
  }

 public:
  INSTRUCTION_HEADER(ToInt64)
  TRIVIAL_NEW_WRAPPERS

  bool congruentTo(const MDefinition* ins) const override {
    return congruentIfOperandsEqual(ins);
  }

  AliasSet getAliasSet() const override { return AliasSet::None(); }

File: js/src/jit/LICM.cpp, line 154-157

static bool IsHoistableIgnoringDependency(MInstruction* ins, bool hasCalls) {
  return ins->isMovable() && !ins->isEffectful() &&
         !HasOperandInLoop(ins, hasCalls);
}

File: js/src/jit/CodeGenerator.cpp, line 5551-5552

using Fn = bool (*)(JSContext*, HandleString, uint64_t*);
callVM<Fn, DoStringToInt64>(lir);

File: js/src/jit/VMFunctions.cpp, line 2735-2746

bool DoStringToInt64(JSContext* cx, HandleString str, uint64_t* res) {
  BigInt* bi;
  JS_TRY_VAR_OR_RETURN_FALSE(cx, bi, js::StringToBigInt(cx, str));

  if (!bi) {
    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                              JSMSG_BIGINT_INVALID_SYNTAX);
    return false;
  }

  *res = js::BigInt::toUint64(bi);
  return true;
}

Verified at revision f0ce2dc6c1dbf8259f09627d8ea4a308e07a461c. The final call has zero loop iterations, so the WebAssembly call must not execute and the correct result is 0n. The no-Ion configuration returns that result. The independent LICM-off comparison isolates loop-invariant code motion as the responsible optimization.

Exploit Chain

  1. Instantiate a standard WebAssembly module exporting an i64 identity function.
  2. Repeatedly call it from a JavaScript loop with the valid string "17", allowing Ion to compile the caller.
  3. Invoke the optimized caller with "not a bigint" and an iteration count of zero.
  4. The hoisted conversion executes despite the skipped loop and raises SyntaxError, interrupting JavaScript execution.

Steps to Reproduce

  1. Use the unmodified Firefox source revision f0ce2dc6c1dbf8259f09627d8ea4a308e07a461c. The submitted testcase is /tmp/tmp8gyc2gpp/testcase.js; its SHA-256 is aa8f99705c0022b96f6afb253e8efbb91b96929a79d69a28e6d799984d767217.
  2. Build the debug shell using build_firefox with firefox_dir=/firefox and mozconfig_path=/app/mozconfigs/mozconfig.js.linux.debug.
  3. Pass the testcase content to js_shell_differential_evaluator, using /firefox/obj-js-debug/dist/bin/js for both shells. Set flags_a to ["--no-threads", "--no-ion"] and flags_b to ["--no-threads", "--ion-warmup-threshold=100", "--baseline-warmup-threshold=10"]. The evaluator adds --fuzzing-safe. Observe differs=true, with exit codes 0 and 3.
  4. Repeat with --differential-testing added to both flag sets. Observe the same divergence and invalid BigInt syntax error.
  5. Compare the gated Ion configuration against the same configuration with --ion-licm=off. The LICM-disabled side exits 0; the ordinary Ion side exits 3.
  6. No source patch, browser preference, or external service is needed. Do not apply the retired slot-audit diff. An independent ASAN build also reproduced the ungated divergence; its optimized shell ignored --differential-testing, so the debug-shell results establish the required gate.

Security Impact

  • Severity: Low
  • Attacker capability: Trigger an unexpected JavaScript exception and interrupt execution of affected optimized code. Memory corruption, sensitive-data disclosure, native-code execution, and privilege escalation were not demonstrated.
  • Preconditions: A 64-bit build using Ion's WebAssembly fast-call path, sufficient execution to optimize the caller, and an invalid string argument supplied when control flow skips the i64 call.

ASAN Report

No sanitizer crash was observed. This finding is established by the required differential evidence.

Ungated evaluator result:
{"differs":true,"shell_a":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--no-ion","/tmp/fx_audit_jsdiff_yd5hp47g/testcase.js"],"exit_code":0,"timed_out":false},"shell_b":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--ion-warmup-threshold=100","--baseline-warmup-threshold=10","/tmp/fx_audit_jsdiff_yd5hp47g/testcase.js"],"exit_code":3,"timed_out":false}}
Ion stderr:
/tmp/fx_audit_jsdiff_yd5hp47g/testcase.js:3:32 SyntaxError: invalid BigInt syntax
Stack:
  convert@/tmp/fx_audit_jsdiff_yd5hp47g/testcase.js:3:32
  @/tmp/fx_audit_jsdiff_yd5hp47g/testcase.js:11:12

Differential-testing evaluator result:
{"differs":true,"shell_a":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--no-ion","--differential-testing","/tmp/fx_audit_jsdiff_86j2vpx5/testcase.js"],"exit_code":0,"timed_out":false},"shell_b":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--ion-warmup-threshold=100","--baseline-warmup-threshold=10","--differential-testing","/tmp/fx_audit_jsdiff_86j2vpx5/testcase.js"],"exit_code":3,"timed_out":false}}
Ion stderr:
/tmp/fx_audit_jsdiff_86j2vpx5/testcase.js:3:32 SyntaxError: invalid BigInt syntax
Stack:
  convert@/tmp/fx_audit_jsdiff_86j2vpx5/testcase.js:3:1
  @/tmp/fx_audit_jsdiff_86j2vpx5/testcase.js:11:1

LICM control evaluator result:
{"differs":true,"shell_a":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--ion-warmup-threshold=100","--baseline-warmup-threshold=10","--ion-licm=off","--differential-testing","/tmp/fx_audit_jsdiff_09_0ysi9/testcase.js"],"exit_code":0,"timed_out":false},"shell_b":{"argv":["/firefox/obj-js-debug/dist/bin/js","--fuzzing-safe","--no-threads","--ion-warmup-threshold=100","--baseline-warmup-threshold=10","--differential-testing","/tmp/fx_audit_jsdiff_09_0ysi9/testcase.js"],"exit_code":3,"timed_out":false}}
Ion stderr:
/tmp/fx_audit_jsdiff_09_0ysi9/testcase.js:3:32 SyntaxError: invalid BigInt syntax
Stack:
  convert@/tmp/fx_audit_jsdiff_09_0ysi9/testcase.js:3:1
  @/tmp/fx_audit_jsdiff_09_0ysi9/testcase.js:11:1
Attached patch fix.patch — — Splinter Review
Attached file crash_stack.txt —
Attached file testcase.js —

This bug was filed automatically by an AI-powered security testing tool. The analysis, testcase, and any suggested patches were generated by an AI model and have NOT been verified by a human.

Please review all details carefully before triaging. The issue was confirmed by an independent verifier, but for non-ASAN issues, the verifier has been made less strict. Additionally, the root cause analysis and suggested fix may contain errors.

Group: core-security → javascript-core-security

I don't think this is security sensitive, but it is a bug.

This code is the only caller of DoStringToInt64. The best fix is probably to rename that function to DoStringToInt64Pure, then bail out if we would throw. If we hoisted this via LICM and don't hit the fallback, the bailout will disable LICM, preventing bailout loops.

Digging a little deeper, I don't think the bailout works: we're inserting the conversion automatically based on the wasm signature, not based on CacheIR. it would fix the LICM problem, but in the simpler case where we always pass a non-BigInt string, it would cause bailout loops.

The proposed fix is fine.

Attached file (secure) —

Conversions to BigInt that can throw (in this case String -> BigInt) can't be hoisted by LICM, because hoisting a conversion out of a zero-iteration loop could cause us to throw when we shouldn't.

Assignee: nobody → iireland
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

:iain, please review the release status flags for beta, release, and ESR and update each one as appropriate.

For each version, indicate whether it is affected, unaffected, disabled, or wontfix if it is affected but does not need to be fixed on that branch.

Flags: needinfo?(iireland)

This is just a correctness issue in a corner case. It can ride the trains.

Flags: needinfo?(iireland)
Duplicate of this bug: 2077078
Blocks: sm-security
Severity: -- → S3
Priority: -- → P2
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 159 Branch
Group: core-security-release
Keywords: sec-other
QA Whiteboard: [qa-triage-done-c160/b159]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: