2053853 - SMRegExpMacroAssembler::CheckPosition — 32-bit signed overflow bypasses lookbehind bounds check, OOB read in JIT-compiled regexp code

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

Summary

SMRegExpMacroAssembler::CheckPosition (js/src/irregexp/RegExpNativeMacroAssembler.cpp:638-658) computes the candidate position for a negative cp_offset (lookbehind) with a pointer-width computeEffectiveAddress and then does a signed pointer-width comparison against inputStart. On 32-bit targets the intermediate value current_position_ + cp_offset * char_size() can underflow int32_t when the subject is a near-maximum-length two-byte string and the match position is at (or very near) the start of the string. The wrapped result is a large positive value, so the signed inputStart > temp0_ test is false, the bounds check is bypassed, and LoadCurrentCharacterUnchecked reads at input_start + cp_offset * 2, i.e. up to ~65 KiB before the subject buffer.

This is an attacker-controlled out-of-bounds read reachable from web content via RegExp.prototype.exec on 32-bit builds (x86-32 / ARM32). The value read is compared against a regexp-controlled character/class, so the OOB memory can be probed bit-by-bit via match/no-match — an information-disclosure primitive.

The irregexp bytecode interpreter is unaffected (it uses plain int character indices, not byte offsets from input_end), and 64-bit native code is unaffected (the same LEA/compare uses 64-bit registers with ample headroom).

Root cause

js/src/irregexp/RegExpNativeMacroAssembler.cpp:649-656 (the cp_offset < 0 branch of CheckPosition):

  } else {
    // Compute offset position
    masm_.computeEffectiveAddress(
        Address(current_position_, cp_offset * char_size()), temp0_);

    // Compare to start of input.
    masm_.branchPtr(Assembler::GreaterThan, inputStart(), temp0_,
                    LabelOrBacktrack(on_outside_input));
  }

current_position_ is a negative byte offset from input end. For a subject string of length L two-byte characters matched at character index p:

current_position_ = -(2*L) + 2*p          // range: [-(2*L), 0]
inputStart        = -(2*L)

JS::MaxStringLength = (1<<30)-2, so for a max-length two-byte subject and p = 0, current_position_ = inputStart = -2147483644 — only 4 above INT32_MIN. Adding the negative displacement cp_offset * char_size() (up to -65534) in a 32-bit register wraps into the positive half of int32_t, and the signed branchPtr(GreaterThan, inputStart, temp0_) compares -2147483644 > +positive → false → the load proceeds as "in bounds".

A prerequisite is that the deepest cp_offset is bounds-checked first: for a read_backward TextNode, TextEmitPass runs the SIMPLE_CHARACTER_MATCH pass before CHARACTER_CLASS_MATCH and skips character-class elements, so a lookbehind of the form (?<=a[b][b]...[b]) emits LoadCurrentCharacter(cp_offset = -(N+1)) for the atom 'a' before any of the shallower class checks at -1..-N. The wraparound therefore happens before a shallower CheckPosition(-1) could catch the out-of-range condition (confirmed with --trace-regexp-assembler).

Reproduction

Testcase: poc.js (attached). Run with a 32-bit debug/ASan JS shell:

js --fuzzing-safe --no-threads poc.js

The testcase:

1. Allocates a flat two-byte string of length (1<<30)-2 (byte length 0x7FFFFFFC). This ~2 GiB allocation is address-space sensitive in a 32-bit process; the retry loop usually succeeds within a few attempts — if it prints could not allocate, rerun.
2. Compiles /^(?<=a[b]{5000})/ and warms it up on a small two-byte string so native UC16 regexp code is generated.
3. Executes on the huge string: ^ anchors at index 0 (current_position_ = -2147483644), the lookbehind's 'a' is bounds-checked at cp_offset = -5001 first, -2147483644 - 10002 wraps, the check passes, and the JIT reads from input_start - 10002.

The ensureLinearString() / gc() shell helpers are only used to make the 2 GiB flat allocation succeed reliably in the shell; the bug itself is in the native regexp macro-assembler, which runs identically in the browser (where RegExp.prototype.exec linearises the subject internally). No non-default prefs are required.

Differential confirmation: the same testcase with --no-native-regexp (bytecode interpreter) returns result: null — the interpreter's CheckPosition operates on int character indices (pos = current + cp_offset; if (pos < 0) fail;) and correctly rejects.

Tested build: SpiderMonkey 154.0a1 JS shell (mozilla-central, rev 0c4c351481d1, 32-bit x86 ASan debug build, objdir-js32-debug).

Crash signature

==2633972==ERROR: AddressSanitizer: SEGV on unknown address 0x67ffc0f6 (pc 0x4ef3c087 ...)
==2633972==The signal is caused by a READ memory access.
    #0 0x4ef3c087  ([anon:js-executable-memory]+0x87)
    #1 0x4ef2dbb7  ([anon:js-executable-memory]+0x1bb7)
    #2 0x4ef2f1d3  ([anon:js-executable-memory]+0x31d3)
    #3 0x4ef1052b  ([anon:js-executable-memory]+0x452b)
    #4 0x4eedc524  ([anon:js-executable-memory]+0x524)
    #5 0x5fe087a7 in EnterJit(...) js/src/jit/Jit.cpp:105:5

Register cross-check: edx = 0x80000004 (= current_position_ = inputStart = -2147483644), esi = 0x7fffd8f2 (the wrapped current_position_ + cp_offset*2), faulting address 0x67ffc0f6 = input_start (0x67ffe808) - 10002, exactly 2*(N+1) bytes before the buffer.

Suggested fix

In the cp_offset < 0 branch of CheckPosition, avoid computing current_position_ + cp_offset*char_size() directly. Instead compare current_position_ against inputStart - cp_offset*char_size() (load inputStart into a temp, subtract Imm32(cp_offset * char_size()), then branchPtr(GreaterThan, temp0_, current_position_, ...)). That intermediate cannot overflow since |inputStart| <= 0x7FFFFFFC and |cp_offset*char_size()| <= 65534. CheckAtStartImpl uses the same computeEffectiveAddress(Address(current_position_, cp_offset*char_size())) pattern and should be reviewed alongside.

Comment 1

[Christian Holler (:decoder)]

Attachment: poc.js — testcase (run with a 32-bit JS shell: js --fuzzing-safe --no-threads poc.js)

Comment 2

[Jan de Mooij [:jandem]]

Our implementation matches upstream V8 code for x86, but their maximum string length is smaller and I think that helps them here. I've reported the bug upstream just so they're aware of this: https://issues.chromium.org/issues/534027938

Comment 3

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 4

[Jan de Mooij [:jandem]]

Attachment: (secure)

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/41298d79dd9c
https://hg.mozilla.org/integration/autoland/rev/8d8c7c12af1e
Fix check in SMRegExpMacroAssembler::CheckPosition. r=dminor

Comment 6

[Donal Meehan [:dmeehan]]

https://hg-edge.mozilla.org/mozilla-central/rev/8d8c7c12af1e

Comment 7

[Donal Meehan [:dmeehan]]

Please add uplift requests for ESR115, ESR140, and ESR153

Comment 8

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: Security bug for 32-bit Firefox.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: It rewrites a check to avoid an integer overflow.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 9

[Jan de Mooij [:jandem]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D311806

Comment 10

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: Security bug for 32-bit Firefox.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: It rewrites a check to avoid an integer overflow.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 11

[Jan de Mooij [:jandem]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D311806

Comment 12

[Jan de Mooij [:jandem]]

Attachment: (secure)

Original Revision: https://phabricator.services.mozilla.com/D311806

Comment 13

[Phabricator Automation]

firefox-esr153 Uplift Approval Request

• User impact if declined/Reason for urgency: Security bug for 32-bit Firefox.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing:
• Risk associated with taking this patch: low
• Explanation of risk level: It rewrites a check to avoid an integer overflow.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/79b91e5818bc

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr153/rev/ab5e03201be4

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/2e80cea9b764