1826666 - (CVE-2023-32213) Latent read and write beyond bounds in FileReader::DoReadData()

Status: RESOLVED FIXED

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

Description

[mozillabugs]

Attachment: ffbug_2118.htm

FileReader::DoReadData() fails to initialize the variable read that it passes to nsIAsyncInputStream::Read() . If nsIAsyncInputStream::Read() were to return NS_BASE_STREAM_CLOSED without initializing read, FileReader::DoReadData() would use the resulting uninitialized stack variable to determine how much data to read from a stack buffer into an output buffer (line 326, below) and to update, among other things, a pointer to an output buffer (line 328, below), potentially causing a read and/or write beyond bounds. [1]

The bug appears to be latent. Using FileReader::readAsBinaryString() on File and Blob objects causes FF to call this function, but the relevant stream is always buffered (and thus handled by lines 303-06), which prevents control from reaching the buggy code. [2] I do not know whether there is another way to reach the buggy code.

The bug is on line 312, which doesn't initialize read:

283: nsresult FileReader::DoReadData(uint64_t aCount) {
284:   MOZ_ASSERT(mAsyncStream);
285: 
286:   uint32_t bytesRead = 0;
287: 
288:   if (mDataFormat == FILE_AS_BINARY) {
289:     // Continuously update our binary string as data comes in
290:     CheckedInt<uint64_t> size{mResult.Length()};
291:     size += aCount;
292: 
293:     if (!size.isValid() || size.value() > UINT32_MAX || size.value() > mTotal) {
294:       return NS_ERROR_OUT_OF_MEMORY;
295:     }
296: 
297:     uint32_t lenBeforeRead = mResult.Length();
298:     MOZ_ASSERT(lenBeforeRead == mDataLen, "unexpected mResult length");
299: 
300:     mResult.SetLength(lenBeforeRead + aCount);
301:     char16_t* currentPos = mResult.BeginWriting() + lenBeforeRead;
302: 
303:     if (NS_InputStreamIsBuffered(mAsyncStream)) {
304:       nsresult rv = mAsyncStream->ReadSegments(ReadFuncBinaryString, currentPos,
305:                                                aCount, &bytesRead);
306:       NS_ENSURE_SUCCESS(rv, NS_OK);
307:     } else {
308:       while (aCount > 0) {
309:         char tmpBuffer[4096];
310:         uint32_t minCount =
311:             XPCOM_MIN(aCount, static_cast<uint64_t>(sizeof(tmpBuffer)));
312:         uint32_t read;
313: 
314:         nsresult rv = mAsyncStream->Read(tmpBuffer, minCount, &read);
315:         if (rv == NS_BASE_STREAM_CLOSED) {
316:           rv = NS_OK;
317:         }
318: 
319:         NS_ENSURE_SUCCESS(rv, NS_OK);
320: 
321:         if (read == 0) {
322:           // The stream finished too early.
323:           return NS_ERROR_OUT_OF_MEMORY;
324:         }
325: 
326:         PopulateBufferForBinaryString(currentPos, tmpBuffer, read);
327: 
328:         currentPos += read;
329:         aCount -= read;
330:         bytesRead += read;
331:       }
332:     }
...
360: }

[1] Some implementations of Read() can return NS_BASE_STREAM_CLOSED without initializing read (e.g., ThrottleInputStream::Read() (netwerk/base/ThrottleQueue.cpp) ) , despite the nsIInputStream contract prohibiting this return code (see nsIInputStream.idl)

[2] Attached is a page that causes FF to call FileReader::DoReadData() . Load it in FF and click "Browse" to select a text file, which causes FF to call it with a File object or click "click!" to cause FF to call it with a Blob object.

Comment 1

[Jens Stutte [:jstutte]]

FWIW, all the else block in question seems to not be executed in our automated test environment neither, as per searchfox' coverage instrumentation.

Comment 2

[Jens Stutte [:jstutte]]

The code in question was introduced by bug 1408397 and as a follow up the optimizing NS_InputStreamIsBuffered(mAsyncStream) case was added in bug 1409394.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Bug 1826666, initialize to 0, r=baku

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

dveditz, I'm not sure about sec-rating for this.
I couldn't immediately find code paths leading to the issue, but I could keep looking for it, if needed.

Comment 5

[Frederik Braun [:freddy]]

We usually rate these kinds of bugs as sec-moderate and fix them as a precaution, rather than a severe security issue.
For context, we've had other latent security issues in Bug 1818381 and bug 1820359.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9329472 [details]
Bug 1826666, initialize to 0, r=baku

Beta Uplift Approval Request

ESR Uplift Approval Request

Security Approval Request

• How easily could an exploit be constructed based on the patch?: There is no known exploit and atm it is unclear how to reach the problematic state, but the patch, which is created based on code inspection, is very trivial and the patch applies to mozilla-central, beta and esr.
• 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 older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: This should be very safe.
• Is Android affected?: Yes

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9329472 [details]
Bug 1826666, initialize to 0, r=baku

sec-moderates don't need sec-approval

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

yeah, I was just hoping to land this basically at the same time everywhere and I'm not sure how to do that.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/845a7a965f39

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9329472 [details]
Bug 1826666, initialize to 0, r=baku

Approved for 113.0b7.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/81a2a08e0546

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9329472 [details]
Bug 1826666, initialize to 0, r=baku

Approved for 102.11esr.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/82ed057450c6

Comment 14

[Tyson Smith [:tsmith]]

Attachment: advisory.txt