1811627 - (CVE-2023-25752) Potential write beyond bounds caused by ThrottleInputStream::Read()/ReadSegments()

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[mozillabugs]

Attachment: 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).

9. 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.

10. 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).

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

Comment 1

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 2

[mozillabugs]

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?

Comment 3

[Andrew McCreight [:mccr8]]

No, XUL addons are long gone.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1811627 - ThrottleQueue::Available should return max aRemaining r=#necko

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 8

[Donal Meehan [:dmeehan]]

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

Approved for 111.0b7

Comment 9

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/69f7fbc6a8a9

Comment 10

[Donal Meehan [:dmeehan]]

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

Approved for 102.9esr.

Comment 11

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/ae6329868fab

Comment 12

[Frederik Braun [:freddy]]

Attachment: advisory.txt