Closed Bug 1826666 (CVE-2023-32213) Opened 11 months ago Closed 10 months ago

Latent read and write beyond bounds in FileReader::DoReadData()

Categories

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

defect

Tracking

()

RESOLVED FIXED
114 Branch
Tracking Status
firefox-esr102 113+ fixed
firefox112 --- wontfix
firefox113 + fixed
firefox114 + fixed

People

(Reporter: mozillabugs, Assigned: smaug)

References

(Regression)

Details

(Keywords: regression, sec-moderate, Whiteboard: [adv-main113+][adv-ESR102.11+])

Attachments

(3 files)

Attached file 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.

Flags: sec-bounty?
Group: core-security → dom-core-security

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

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.

Assignee: nobody → smaug
Status: NEW → ASSIGNED

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.

Flags: needinfo?(dveditz)

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.

Flags: needinfo?(dveditz)
Keywords: sec-moderate

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
Attachment #9329472 - Flags: sec-approval?
Attachment #9329472 - Flags: approval-mozilla-esr102?
Attachment #9329472 - Flags: approval-mozilla-beta?

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

sec-moderates don't need sec-approval

Attachment #9329472 - Flags: sec-approval?

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

Severity: -- → S3
Priority: -- → P2
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch

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

Approved for 113.0b7.

Attachment #9329472 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Approved for 102.11esr.

Attachment #9329472 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Whiteboard: [adv-main113+]
Whiteboard: [adv-main113+] → [adv-main113+][adv-ESR102.11+]
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Flags: sec-bounty? → sec-bounty+
Keywords: regression
Regressed by: 1408397
Attached file advisory.txt
Alias: CVE-2023-32213
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.