2021904 - Heap-Buffer-Overflow in RemoteLazyInputStream though [@ RemoteLazyInputStreamParent::RecvStreamNeeded]

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P2)

Description

[Christian Holler (:decoder)]

Attachment: test.html

Summary

Heap buffer overflow (WRITE) in the Firefox parent process via PRemoteLazyInputStream::StreamNeeded IPC. The root cause is a missing CheckedUint64 overflow check in /firefox/dom/file/ipc/RemoteLazyInputStreamStorage.cpp:166-168 which constructs a SlicedInputStream with mStart + mLength wrapping to zero, combined with unsafe arithmetic in /firefox/xpcom/io/SlicedInputStream.cpp:235-236 which underflows aCount to 0xFFFFFFFF. A compromised content process can overwrite ~2MB of parent process heap with fully attacker-controlled data by posting a Blob via BroadcastChannel and then requesting it with malicious slice parameters.

Affected Code

File 1: /firefox/dom/file/ipc/RemoteLazyInputStreamStorage.cpp, lines 166-169

// Now it's the right time to apply a slice if needed.
if (aStart > 0 || aLength < UINT64_MAX) {
  clonedStream =
      new SlicedInputStream(clonedStream.forget(), aStart, aLength);
}

aStart and aLength arrive directly from RemoteLazyInputStreamParent::RecvStreamNeeded(uint64_t aStart, uint64_t aLength) with zero validation. With aStart=1, aLength=UINT64_MAX, the condition aStart > 0 is satisfied, creating a SlicedInputStream where mStart + mLength = 1 + 0xFFFFFFFFFFFFFFFF wraps to 0. Notably, SlicedInputStream::Deserialize (line 494-497) defends against exactly this attack with CheckedUint64(params.start()) + params.length() — but GetStream bypasses the deserializer by calling the constructor directly.

File 2: /firefox/xpcom/io/SlicedInputStream.cpp, lines 235-244

// Let's reduce aCount in case it's too big.
if (mCurPos + aCount > mStart + mLength) {
  aCount = mStart + mLength - mCurPos;
}

// Nothing else to read.
if (!aCount) {
  return NS_OK;
}

nsresult rv = mInputStream->Read(aBuffer, aCount, aReadCount);

All variables are uint64_t. After discarding 1 byte to skip mStart (the inner pipe is not seekable), mCurPos=1. Then:

• Condition: (1 + 65536) > (1 + UINT64_MAX) → 65537 > 0 → TRUE
• aCount = (1 + UINT64_MAX) - 1 = 0 - 1 = 0xFFFFFFFFFFFFFFFF → truncated to uint32_t → 0xFFFFFFFF
• mInputStream->Read(aBuffer, 0xFFFFFFFF, ...) is called on a 64KB buffer

Correct Pattern

// In RemoteLazyInputStreamStorage::GetStream, before constructing SlicedInputStream:
auto end = CheckedUint64(aStart) + aLength;
if (!end.isValid()) {
  return;  // Reject overflowing slice parameters
}
if (aStart > 0 || aLength < UINT64_MAX) {
  clonedStream = new SlicedInputStream(clonedStream.forget(), aStart, aLength);
}

And defensively in SlicedInputStream::Read:

// Let's reduce aCount in case it's too big.
CheckedUint64 end = CheckedUint64(mStart) + mLength;
if (!end.isValid() || mCurPos >= end.value()) {
  return NS_OK;  // Nothing else to read.
}
uint64_t remaining = end.value() - mCurPos;
if (aCount > remaining) {
  aCount = static_cast<uint32_t>(remaining);
}

Exploit Chain

1. Content process creates two BroadcastChannel objects on the same channel name ("overflow-channel").
2. bc1.postMessage(blob) sends a 2MB Blob filled with attacker pattern 0x41 + (i % 26) → parent receives via PBroadcastChannel::PostMessage.
3. Parent's BroadcastChannelService::PostMessage iterates subscribers and calls SendNotify to bc2's actor. During re-serialization, IPCBlobUtils::Serialize → RemoteLazyInputStream::WrapStream stores a cloned nsPipeInputStream (holding the 2MB data) into RemoteLazyInputStreamStorage under a new nsID.
4. bc2.onmessage fires in content; e.data.arrayBuffer() triggers RemoteLazyInputStream::StreamNeeded().
5. Compromised content (simulated by patch) sends PRemoteLazyInputStream::StreamNeeded(aStart=1, aLength=UINT64_MAX) instead of legitimate (0, UINT64_MAX).
6. Parent's RecvStreamNeeded → storage->GetStream(mID, 1, UINT64_MAX, ...) → new SlicedInputStream(pipeClone, 1, UINT64_MAX). Member mStart + mLength wraps to 0.
7. SerializeIPCStream(..., /* aAllowLazy */ false) → inner pipe's SerializedComplexity reports *aPipes > 0, *aTransferables == 0 → SlicedInputStream::Serialize calls InputStreamHelper::SerializeInputStreamAsPipe(this, ...).
8. SerializeInputStreamAsPipe creates a 64KB DataPipe and launches NS_AsyncCopy(SlicedInputStream, DataPipeSender, STS, NS_ASYNCCOPY_VIA_WRITESEGMENTS, 65536).
9. On the STS thread: nsStreamCopierOB::DoCopy → DataPipeSender::WriteSegments(FillOutputBuffer, ..., 65536) → provides 64KB shared-memory segment → FillOutputBuffer calls SlicedInputStream::Read(64KB_buffer, 65536, ...).
10. SlicedInputStream::Read: discards 1 byte (mCurPos=1), computes aCount = 0 - 1 = 0xFFFFFFFF, calls nsPipeInputStream::Read(64KB_buffer, 0xFFFFFFFF, ...).
11. nsPipeInputStream::Read = ReadSegments(NS_CopySegmentToBuffer, 64KB_buffer, 0xFFFFFFFF, ...). The loop iterates through all 2MB of pipe segments, calling memcpy(&toBuf[aOffset], segment, segmentLen) with aOffset growing unboundedly. ASAN detects WRITE past buffer end.

IPC Path

• Setup protocol: PBroadcastChannel (/firefox/dom/broadcastchannel/PBroadcastChannel.ipdl) — PostMessage from child, Notify back to child. Essential because the broadcast re-serialization in the parent is what populates RemoteLazyInputStreamStorage with an nsPipeInputStream entry for the attacker to target.
• Attack protocol: PRemoteLazyInputStream (/firefox/dom/file/ipc/PRemoteLazyInputStream.ipdl), line 15: async StreamNeeded(uint64_t aStart, uint64_t aLength) returns (IPCStream? stream);
• Parent handler: RemoteLazyInputStreamParent::RecvStreamNeeded at /firefox/dom/file/ipc/RemoteLazyInputStreamParent.cpp:54-79 — passes aStart/aLength unvalidated to GetStream.
• Child sender: RemoteLazyInputStream::StreamNeeded at /firefox/dom/file/ipc/RemoteLazyInputStream.cpp:810+

Security Impact

• Severity: Critical — sandbox escape primitive
• Attacker capability: A compromised content process can write an arbitrary amount of fully attacker-controlled data (the blob payload) past the end of a 64KB heap allocation in the parent process. The overflow size is controlled by the blob size (2MB in this PoC, but can be larger). The write target is the DataPipe shared memory ring buffer, with overflow proceeding linearly into adjacent heap allocations. With heap grooming, this is a strong primitive for parent process code execution.
• Preconditions: Attacker must first compromise a content process (e.g., via a JS engine bug). No user interaction required beyond loading a page. Works on default Firefox configuration.
• Reliability: High. Unsigned integer wraparound is well-defined in C++ (not UB), so the arithmetic is deterministic. The stderr log shows the exploit hook fired 29 times — the path is stable and repeatable.

ASAN Report

==462029==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x727e04e86000 at pc 0x55d2f1cd172e bp 0x727e046428d0 sp 0x727e04642090
WRITE of size 65536 at 0x727e04e86000 thread T29
    #0 0x55d2f1cd172d in __asan_memcpy _asan_rtl_:3
    #1 0x727e3939167c in NS_CopySegmentToBuffer(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*) /firefox/xpcom/io/nsStreamUtils.cpp:781:3
    #2 0x727e3939108b in nsPipeInputStream::ReadSegments(nsresult (*)(nsIInputStream*, void*, char const*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) /firefox/xpcom/io/nsPipe3.cpp:1429:12
    #3 0x727e39341c6a in mozilla::SlicedInputStream::Read(char*, unsigned int, unsigned int*) /firefox/xpcom/io/SlicedInputStream.cpp:244:31
    #4 0x727e393b6575 in nsStreamCopierOB::FillOutputBuffer(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*) /firefox/xpcom/io/nsStreamUtils.cpp:558:35
    #5 0x727e3af0f324 in mozilla::FunctionRef<nsresult (mozilla::Span<char, 18446744073709551615ul>, unsigned int, unsigned int*)>::operator()(mozilla::Span<char, 18446744073709551615ul>, unsigned int, unsigned int*) const /firefox/obj-x86_64-pc-linux-gnu/dist/include/mozilla/FunctionRef.h:211:12
    #6 0x727e3af0f324 in mozilla::ipc::data_pipe_detail::DataPipeBase::ProcessSegmentsInternal(unsigned int, mozilla::FunctionRef<nsresult (mozilla::Span<char, 18446744073709551615ul>, unsigned int, unsigned int*)>, unsigned int*) /firefox/ipc/glue/DataPipe.cpp:358:23
    #7 0x727e3af119e0 in mozilla::ipc::DataPipeSender::WriteSegments(nsresult (*)(nsIOutputStream*, void*, char*, unsigned int, unsigned int, unsigned int*), void*, unsigned int, unsigned int*) /firefox/ipc/glue/DataPipe.cpp:568:10
    #8 0x727e393b5716 in nsStreamCopierOB::DoCopy(nsresult*, nsresult*) /firefox/xpcom/io/nsStreamUtils.cpp:576:16
    #9 0x727e393b69e6 in nsAStreamCopier::Process() /firefox/xpcom/io/nsStreamUtils.cpp:304:22
    #10 0x727e393b1569 in nsAStreamCopier::Run() /firefox/xpcom/io/nsStreamUtils.cpp:431:5
    #11 0x727e393b1569 in non-virtual thunk to nsAStreamCopier::Run() /firefox/xpcom/io/nsStreamUtils.cpp:0:0
    #12 0x727e39468517 in nsThreadPool::Run() /firefox/xpcom/threads/nsThreadPool.cpp:446:14
    #13 0x727e3945b590 in nsThread::ProcessNextEvent(bool, bool*) /firefox/xpcom/threads/nsThread.cpp:1175:16
    #14 0x727e394640e9 in NS_ProcessNextEvent(nsIThread*, bool) /firefox/xpcom/threads/nsThreadUtils.cpp:467:10
    #15 0x727e3af87881 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /firefox/ipc/glue/MessagePump.cpp:299:20
    #16 0x727e3adcb544 in MessageLoop::RunInternal() /firefox/ipc/chromium/src/base/message_loop.cc:373:10
    #17 0x727e3adcb544 in MessageLoop::RunHandler() /firefox/ipc/chromium/src/base/message_loop.cc:366:3
    #18 0x727e3adcb544 in MessageLoop::Run() /firefox/ipc/chromium/src/base/message_loop.cc:348:3
    #19 0x727e39453b50 in nsThread::ThreadFunc(void*) /firefox/xpcom/threads/nsThread.cpp:376:10
    #20 0x767e6525d88f in _pt_root /firefox/nsprpub/pr/src/pthreads/ptthread.c:191:3
    #21 0x55d2f1ccfb36 in asan_thread_start(void*) _asan_rtl_:28
    #22 0x767e6569caa3 in pthread_condattr_setpshared ??:?
    #23 0x767e65729c6b in __clone ??:?

0x727e04e86000 is located 6144 bytes before 131104-byte region [0x727e04e87800,0x727e04ea7820)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/firefox/obj-x86_64-pc-linux-gnu/dist/bin/firefox+0x17572d)
Shadow bytes around the buggy address:
  0x727e04e85f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x727e04e86000:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x727e04e86080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa

Suggested Fix

Validate in RemoteLazyInputStreamParent::RecvStreamNeeded (the IPC trust boundary):

// /firefox/dom/file/ipc/RemoteLazyInputStreamParent.cpp
mozilla::ipc::IPCResult RemoteLazyInputStreamParent::RecvStreamNeeded(
    uint64_t aStart, uint64_t aLength, StreamNeededResolver&& aResolver) {
  // Validate slice parameters from untrusted content process.
  CheckedUint64 end = CheckedUint64(aStart) + aLength;
  if (!end.isValid()) {
    return IPC_FAIL(this, "StreamNeeded: start + length overflows");
  }

  nsCOMPtr<nsIInputStream> stream;
  auto storage = RemoteLazyInputStreamStorage::Get().unwrapOr(nullptr);
  if (storage) {
    storage->GetStream(mID, aStart, aLength, getter_AddRefs(stream));
  }
  // ... rest unchanged
}

Additionally, add the same CheckedUint64 check to the SlicedInputStream constructor and fix the arithmetic in SlicedInputStream::Read for defense in depth — SlicedInputStream::Deserialize already validates, but the constructor does not, leaving a gap for any code path that constructs the object directly.

Comment 1

[Christian Holler (:decoder)]

Attachment: exploit.patch

Comment 2

[Christian Holler (:decoder)]

Attachment: crash_stack.txt

Comment 3

[Andrew McCreight [:mccr8]]

SlicedInputStream::AdjustRange also has the same mStart + mLength math. I'm not sure where exactly the trust boundary should be.

Comment 5

[Andrew McCreight [:mccr8]]

Anybody who fixes this should double check that all of the places mentioned in bug 2022490 are also covered.

Comment 6

[Andrew McCreight [:mccr8]]

I think every place that does mStart + mLength in this file should be audited, and ideally all places that do any address arithmetic.

Comment 7

[Christian Holler (:decoder)]

Attachment: Bug 2021904 - Reject overflowing slice ranges before they reach SlicedInputStream

[see below]

Comment 8

[Christian Holler (:decoder)]

Defense-in-depth fix covering both this bug and bug 2022490:

1. RecvStreamNeeded (IPC boundary, this bug): IPC_FAIL on aStart + aLength overflow.
2. IPCRead (IPC boundary, bug 2022490): FatalError on start + length overflow.
3. RemoteLazyInputStreamStorage::GetStream (common chokepoint): early return on overflow, mirroring the check SlicedInputStream::Deserialize already had.
4. SlicedInputStream constructor: MOZ_RELEASE_ASSERT so no future caller can reach Read() with a wrapping range. All existing callers audited — all pass start + length ≤ some_known_size, none trip the assert.
5. SlicedInputStream::Read: rewrote the clamp as mLength - (mCurPos - mStart) — no intermediate mStart + mLength sum, so aCount can never grow even if the invariant were somehow violated.
6. SlicedInputStream::AdjustRange (comment 3): uses CheckedUint64 for the end computation.

The remaining mStart + mLength uses in Seek() are protected by the constructor assert and are not independently memory-unsafe (they only affect seek range validation).

Verified: the PoC produces IPC_FAIL instead of heap-buffer-overflow; legitimate BroadcastChannel blob transfer and Blob.slice() work; all 21 TestSlicedInputStream gtests pass.

This is the analysis tool's suggested fix. Feel welcome to adopt it as a starting point and evolve it as needed to meet our coding standards.

Comment 10

[Kagami Rosylight 🏳️‍🌈🏳️‍⚧️ [:saschanaz] (they/them)]

Oh this needs an extra patch to exploit.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: (secure)

This is a clean-up of previous changes to sliced input stream
validation. This new approach should reduce the burdern on any external
callers while maintaining the documented expected behaviour.

It's OK for us to truncate these lengths, as start and length positions
past the end of the provided stream have always been documented to be
accepted, and this preserves the expected behaviour.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 9569337 [details]
(secure)

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It probably wouldn't be too hard to find the places which this security patch is fixing in various callers, but the patch avoids directly touching them.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: There's a very solid chance this just applies directly
• How likely is this patch to cause regressions; how much testing does it need?: This just adds some clamping on integer values to prevent overflows. I am not too worried about regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 9569337 [details]
(secure)

sec-approval+ to land and request uplifts

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: (secure)

This is a clean-up of previous changes to sliced input stream
validation. This new approach should reduce the burdern on any external
callers while maintaining the documented expected behaviour.

It's OK for us to truncate these lengths, as start and length positions
past the end of the provided stream have always been documented to be
accepted, and this preserves the expected behaviour.

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

Comment 15

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high bug
• 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: Should be a fairly straightforward and safe fix.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 16

[Nika Layzell [:nika] (ni? for response)]

Attachment: (secure)

This is a clean-up of previous changes to sliced input stream
validation. This new approach should reduce the burdern on any external
callers while maintaining the documented expected behaviour.

It's OK for us to truncate these lengths, as start and length positions
past the end of the provided stream have always been documented to be
accepted, and this preserves the expected behaviour.

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

Comment 17

[Phabricator Automation]

firefox-esr115 Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high bug
• 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: Should be a fairly straightforward and safe fix.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 18

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined/Reason for urgency: sec-high bug
• 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: Should be a fairly straightforward and safe fix.
• String changes made/needed?: N/A
• Is Android affected?: yes

Comment 19

[Nika Layzell [:nika] (ni? for response)]

Attachment: (secure)

This is a clean-up of previous changes to sliced input stream
validation. This new approach should reduce the burdern on any external
callers while maintaining the documented expected behaviour.

It's OK for us to truncate these lengths, as start and length positions
past the end of the provided stream have always been documented to be
accepted, and this preserves the expected behaviour.

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/9968df8fbe02
https://hg.mozilla.org/integration/autoland/rev/27d879391465
Clamp aStart and aLength when constructing a SlicedInputStream, r=asuth

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg-edge.mozilla.org/mozilla-central/rev/27d879391465

Comment 22

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:nika, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox150 to wontfix.

For more information, please visit BugBot documentation.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

There are already uplifts pending it appears

Comment 24

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/5958fa1fd6fb

Comment 25

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/93cde759a331

Comment 26

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/67bfad6b1242
https://hg.mozilla.org/releases/mozilla-release/rev/87b16eebb910

Comment 27

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/eed41f32ecc5

Comment 28

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/70263507d3d7