2025888 - nsMultiMixedConv AccumulateData stores raw pointer into reallocatable IncrementalTokenizer buffer

Status: RESOLVED FIXED

(Core :: Networking, defect, P2)

Description

[Joren Afman]

Attachment: poc-multipart-server.py

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:148.0) Gecko/20100101 Firefox/148.0

Steps to reproduce:

1. Set up a malicious HTTP server sending multipart/x-mixed-replace response
2. Craft the response so body data is followed by CRLF at buffer boundaries,
   causing the tokenizer to call NeedMoreInput()
3. The FeedInput while loop iterates, calling mInput.SetLength() which may
   reallocate the buffer
4. mRawData is now a dangling pointer
5. SendData() calls ss->ShareData(rawData, mRawDataLength) with the dangling pointer
6. Navigate to the malicious server's URL

Actual results:

mRawData (nsMultiMixedConv.cpp:917) points into the tokenizer's mInput nsCString.
If FeedInput's while loop (IncrementalTokenizer.cpp:45-86) reallocates mInput
between iterations (via mInput.SetLength at line 68), mRawData becomes dangling.

Note: Reliable exploitation requires specific conditions (partial reads from
nsIInputStream + buffer reallocation), making this difficult but not impossible
to trigger.

AFFECTED CODE:

• netwerk/streamconv/converters/nsMultiMixedConv.cpp:915-931 (AccumulateData)
• xpcom/ds/IncrementalTokenizer.cpp:45-86 (FeedInput while loop)

Expected results:

AccumulateData should copy accumulated data into a separate buffer rather than
storing a raw pointer into the tokenizer's internal buffer.

Comment 1

[Valentin Gosu [:valentin]]

In the normal HTTP path from a webpage, this is practically impossible to trigger. The chain is:

1. Pump calls Available() on nsBufferedInputStream → returns buffered + pipe.Available()
2. Pump calls OnDataAvailable with that count
3. FeedInput calls Read(buffer, count, &read) on the buffered stream
4. nsBufferedInputStream::ReadSegments loops: reads from its 4096-byte internal buffer, calls Fill() from the pipe when empty, repeats until all count bytes are delivered
5. Since Available() and Read() happen on the same thread with no yields in between, the pipe always has exactly what Available() reported

The FeedInput while loop would only iterate more than once if Read() returns fewer bytes than count. Given the above, Read() always returns exactly count bytes. The while loop runs exactly once. Process() is called once. No reallocation between AccumulateData and SendData.

A malicious webpage cannot trigger this through the normal HTTP stack. The partial-read condition that triggers the bug requires a nsIInputStream implementation that returns fewer bytes than available — which doesn't happen with the pipe + buffered stream combination used by the network layer.

Joren, I think this is still a valuable bug you identified, but the test case doesn't actually trigger a crash in ASAN.
I'll post a fix soon, but could you check if there's something I missed in regards to it being exploitable?

Comment 2

[Valentin Gosu [:valentin]]

Attachment: (secure)

Comment 3

[Valentin Gosu [:valentin]]

Attachment: (secure)

Comment 4

[Joren Afman]

Hi Valentin.

Thanks for the detailed analysis - I believe you're right on all counts.

I traced through the same path you described and confirmed: nsInputStreamPump calls Available() then passes that count to OnDataAvailable, and the pipe's ReadSegments (nsPipe3.cpp:1394) loops through all internal segments via NS_CopySegmentToBuffer until the full count is satisfied.
The FeedInput while loop therefore runs exactly once, and no reallocation occurs between AccumulateData and SendData.

I identified this bug through static code review - the pattern of storing a raw pointer into a growable buffer looked like a UAF setup, but I don't have a concrete path to trigger it through the current HTTP stack. Your analysis of why it doesn't trigger in practice is quite thorough and I can only agree with it.

Comment 5

[Valentin Gosu [:valentin]]

Thanks! We'll try to land the patch to make sure future code doesn't pose an issue.
Note for the future: running your test case under ASAN should confirm when you have an exploitable bug.
Cheers!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/6968cbae90fb
https://hg.mozilla.org/integration/autoland/rev/aa1d7d40a84e
Make IncrementalTokenizer read entire buffer r=necko-reviewers,jesup

Comment 7

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

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