Closed Bug 1811627 (CVE-2023-25752) Opened 2 years ago Closed 1 year ago

Potential write beyond bounds caused by ThrottleInputStream::Read()/ReadSegments()

Categories

(Core :: Networking, defect, P1)

All
Unspecified
defect

Tracking

()

RESOLVED FIXED
112 Branch
Tracking Status
firefox-esr102 111+ fixed
firefox110 --- wontfix
firefox111 + fixed
firefox112 + fixed

People

(Reporter: mozillabugs, Assigned: valentin)

Details

(Keywords: csectype-bounds, reporter-external, sec-moderate, Whiteboard: [necko-triaged][necko-priority-queue][adv-main111+][adv-esr102.9+])

Attachments

(3 files)

Attached file ffbug_2080.htm

ThrottleInputStream::Read()/ReadSegments() (netwerk/base/ThrottleQueue.cpp ) ignore the caller's requested read size aCount, potentially causing lower layers to read data into memory beyond the end of a caller's buffer.

ThrottleInputStream is used only by ThrottleQueue. I have found one user (the throttling control on the Developer Tools panel) which can be used to observe usage of the incorrect read size, but which doesn't suffer a WBB (write beyond bounds) because the read size is not used to buffer data, only to send data out a socket. I am not sure whether there are other users, though nsHttpTransaction::Init() has the ability to use the class via its eventSink argument.

The bug is that ThrottleInputStream::Read()/ReadSegments() don't minimize the amount of data potentially available in the underlying stream (realCount) against the requested read size aCount. (While Available(), line 110, takes aCount, it doesn't use it):

102: NS_IMETHODIMP
103: ThrottleInputStream::ReadSegments(nsWriteSegmentFun aWriter, void* aClosure,
104:                                   uint32_t aCount, uint32_t* aResult) {
...
109:   uint32_t realCount;
110:   nsresult rv = mQueue->Available(aCount, &realCount);
111:   if (NS_FAILED(rv)) {
112:     return rv;
113:   }
114: 
115:   if (realCount == 0) {
116:     return NS_BASE_STREAM_WOULD_BLOCK;
117:   }
118: 
119:   rv = mStream->ReadSegments(aWriter, aClosure, realCount, aResult);
120:   if (NS_SUCCEEDED(rv) && *aResult > 0) {
121:     mQueue->RecordRead(*aResult);
122:   }
123:   return rv;
124: 
125: }

[This function doesn't use aRemaining:]

267: NS_IMETHODIMP
268: ThrottleQueue::Available(uint32_t aRemaining, uint32_t* aAvailable) {
269:   MOZ_ASSERT(OnSocketThread(), "not on socket thread");
270:   TimeStamp now = TimeStamp::Now();
271:   TimeStamp oneSecondAgo = now - TimeDuration::FromSeconds(1);
272:   size_t i;
273: 
274:   // Remove all stale events.
275:   for (i = 0; i < mReadEvents.Length(); ++i) {
276:     if (mReadEvents[i].mTime >= oneSecondAgo) {
277:       break;
278:     }
279:   }
280:   mReadEvents.RemoveElementsAt(0, i);
281: 
282:   uint32_t totalBytes = 0;
283:   for (i = 0; i < mReadEvents.Length(); ++i) {
284:     totalBytes += mReadEvents[i].mBytesRead;
285:   }
286: 
287:   uint32_t spread = mMaxBytesPerSecond - mMeanBytesPerSecond;
288:   double prob = static_cast<double>(rand()) / RAND_MAX;
289:   uint32_t thisSliceBytes =
290:       mMeanBytesPerSecond - spread + static_cast<uint32_t>(2 * spread * prob);
291: 
292:   if (totalBytes >= thisSliceBytes) {
293:     *aAvailable = 0;
294:   } else {
295:     *aAvailable = thisSliceBytes;
296:   }
297:   return NS_OK;
298: }

You can verify that ThrottleInputStream::ReadSegments() can call the lower layer with a byte count exceeding aCount by doing the following:

  1. Start FF and attach a debugger to it.
  2. Set a BP in ThrottleInputStream::ReadSegments() on the call to mStream->ReadSegments().
  3. Open FF's Web Developer Tools panel, then the network tab.
  4. Check "Disable Cache" and select "WiFi" from the throttling control.
  5. Put ffbug_2080.htm on a webserver. If the server is not running on 127.0.0.1, modify ffbug_2080.htm appropriately to do the GET request to the appropriate server.
  6. Load ffbug_2080.htm in FF.
  7. When the BP fires, examine realCount. Notice that it far exceeds aCount (in my test, realCount was 0x1e0000 and aCount was 0x8000.
  8. Step into mStream->ReadSegments(). Control should end up in nsStringInputStream::ReadSegments(). Proceed to the line beginning
   nsresult = aWriter(...);

and notice that source (as limited by this function's aCount and maxCount) contains the initial HTTP request to fetch ffbug_2080.htm. The request isn't very long, so it doesn't exceed the original aCount (0x8000).

  1. Proceed, take the BP again, step into mStream->ReadSegments() again, and proceed as in step 8. Notice that this function's aCount is now 0 because maxCount is 0.

  2. Proceed and take the BP again, and step into mStream->ReadSegments() again, and proceed as in step 8. Notice that source contains the long GET request that the script in ffbug_2080.htm generated, and that the function's aCount and maxCount are both 0xc877. This is much greater than the aCount that ThrottleInputStream::ReadSegments()'s caller requested (0x8000).

  3. You can trace all the way into nsHttpConnection::OnReadSegment() and find that 0xc877 bytes do get written to the socket.

Group: core-security → network-core-security

The fact that you can only hit this with Developer Tools throttling turned on makes this a bit less severe.
Thank you for the great bug report.

Severity: -- → S3
Keywords: csectype-bounds
Priority: -- → P1
Whiteboard: [necko-triaged][necko-priority-queue]

ThrottleQueue can also be used by creating an instance of @mozilla.org/network/throttlequeue;1 or ModuleID::Anonymous169, but it appears that only tests do this at present. Can old-style add-ons use these functions? Are there any old-style addons anymore?

No, XUL addons are long gone.

Flags: sec-bounty?
Assignee: nobody → valentin.gosu
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 112 Branch

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

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox111 to wontfix.

For more information, please visit auto_nag documentation.

Flags: needinfo?(valentin.gosu)

Comment on attachment 9318754 [details]
Bug 1811627 - ThrottleQueue::Available should return max aRemaining r=#necko

Beta/Release Uplift Approval Request

  • User impact if declined: Potential out of bounds read when devtools console is open and network throttling is enabled.
  • Is this code covered by automated tests?: Unknown
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is low risk as the change only introduces a bounds check when calling ThrottleQueue::Available.
    The risk of changing user behaviour is minimal - also this code is only called when the network throttling is activated from devtools.
  • String changes made/needed:
  • Is Android affected?: No

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: Potential out of bounds read when devtools console is open and network throttling is enabled.
  • Fix Landed on Version: 112
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is low risk as the change only introduces a bounds check when calling ThrottleQueue::Available.
    The risk of changing user behaviour is minimal - also this code is only called when the network throttling is activated from devtools.
Flags: needinfo?(valentin.gosu)
Attachment #9318754 - Flags: approval-mozilla-esr102?
Attachment #9318754 - Flags: approval-mozilla-beta?

Comment on attachment 9318754 [details]
Bug 1811627 - ThrottleQueue::Available should return max aRemaining r=#necko

Approved for 111.0b7

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

Comment on attachment 9318754 [details]
Bug 1811627 - ThrottleQueue::Available should return max aRemaining r=#necko

Approved for 102.9esr.

Attachment #9318754 - Flags: approval-mozilla-esr102? → approval-mozilla-esr102+
Flags: sec-bounty? → sec-bounty+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify-
Whiteboard: [necko-triaged][necko-priority-queue] → [necko-triaged][necko-priority-queue][adv-main111+]
Whiteboard: [necko-triaged][necko-priority-queue][adv-main111+] → [necko-triaged][necko-priority-queue][adv-main111+][adv-esr102.9+]
Alias: CVE-2023-25752
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: