1823172 - (CVE-2023-5173) Write beyond bounds in TransactionObserver::OnDataAvailable()

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[mozillabugs]

Attachment: ffbug_2106.htm

TransactionObserver::OnDataAvailable() (netwerk/protocol/http/AlternateServices.cpp) can experience an integer overflow and subsequent underallocation and write beyond bounds (WBB) under specific conditions. The data written beyond bounds is unmodified attacker-provided data, and it is written into the heap of the chrome process.

The bug is in line 824, which adds two uint32_ts without checking for overflow:

   818: NS_IMETHODIMP
   819: TransactionObserver::OnDataAvailable(nsIRequest* aRequest,
   820:                                      nsIInputStream* aStream, uint64_t aOffset,
   821:                                      uint32_t aCount) {
   822:   MOZ_ASSERT(NS_IsMainThread());
   823:   uint32_t oldLen = mWKResponse.Length();
   824:   uint64_t newLen = aCount + oldLen;
   825:   if (newLen < MAX_WK) {
   826:     auto handleOrErr = mWKResponse.BulkWrite(newLen, oldLen, false);
   827:     if (handleOrErr.isErr()) {
   828:       return handleOrErr.unwrapErr();
   829:     }
   830:     auto handle = handleOrErr.unwrap();
   831:     uint32_t amtRead;
   832:     if (NS_SUCCEEDED(
   833:             aStream->Read(handle.Elements() + oldLen, aCount, &amtRead))) {
   834:       MOZ_ASSERT(oldLen + amtRead <= newLen);
   835:       handle.Finish(oldLen + amtRead, false);
   836:       LOG(("TransactionObserver onDataAvailable %p read %d of .wk [%zd]\n",
   837:            this, amtRead, mWKResponse.Length()));
   838:     } else {
   839:       LOG(("TransactionObserver onDataAvailable %p read error\n", this));
   840:     }
   841:   }
   842:   return NS_OK;
   843: }

The conditions required are (1) some nonstandard prefs; and (2) a main thread that stalls at the right time and long enough for the socket thread to accumulate enough data to trigger the overflow. These conditions are unlikely to occur in the wild, though probably there are other combinations of prefs that would elicit the same behavior, and there probably still are ways to monopolize the chrome process's main thread for long periods of time.

Attached is a POC that shows the overflow and WBB. Use the POC thusly:

1. Copy ffbug_2106.htm to a webserver.
2. Compile and run gen-attack.cpp and copy the resulting file to /.well_known/http-opportunistic on the same webserver as in step 1. This file can contain any attacker-provided data, but must be large enough to cause the overflow.
3. Configure the webserver to serve HTTP content on port 444 (as well as the default port 80) and to send the header

   Alt-Svc: h2=":444"

4. Start FF and set prefs as follows:

   network.http.altsvc.oe     true
   network.buffer.cache.size  510       // yes, really 510 and not 512
   network.buffer.cache.count 8421510   // not critical, just has to be large enough

5. Attach a debugger to FF and set a BP on line 824, above.
6. Load ffbug_2106.htm in FF.
7. When the BP fires, freeze the thread (should be main thread in the chrome process) to simulate a lengthy operation on the main thread. Continue execution.
8. FF will accumulate > 4GB of memory usage. When it stops adding memory, use the debugger to break into execution.
9. Thaw the thread that you froze in step 7. Proceed.
10. When you get a second BP, aCount probably will be >= 0x10000, which would make the remainder of the function a no-op. Just proceed.
11. After a few seconds you'll get a third BP, this time with aCount == 0xfffffefe. Now step lines 823-24 and notice that oldLen probably == 0x200, and that aCount + oldLen on line 824 overflowed, causing newLen to have the value 0xfe.
12. Step through lines 822-23 and watch them write beyond bounds. Proceed and the program probably eventually will hit a no-access page and crash.

Comment 1

[mozillabugs]

Attachment: gen-attack.cpp

Comment 2

[mozillabugs]

BTW I tested the POC on FF 110.0, but the offending code is unchanged in trunk.

Comment 3

[Andrew McCreight [:mccr8]]

Lots of weird pref values, including the .oe thing that got disabled in 2021 for some other security issue, but I guess I'll mark this sec-moderate. That might be a bit high though.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Yeah, non-standard prefs seriously reduces the impact here. It's good to understand and fix areas where this could happen, so future changes don't hit the same problem, or if we were ever to want to use those prefs. If the flaw is in code directly affected by one of the prefs, we may want to simply remove that code wholesale unless we're hoping to eventually use that pref.

Many prefs are effectively "dead code" we don't ever plan on using again.

Comment 5

[Andrew Creskey [:acreskey]]

Attachment: Bug 1823172 - Refactor TransactionObserver::OnDataAvailable() r=#necko-reviewers

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3987cdfccbc
Refactor TransactionObserver::OnDataAvailable() 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/b3987cdfccbc

Comment 8

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

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

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

For more information, please visit BugBot documentation.

Comment 9

[Andrew Creskey [:acreskey]]

I set the status of firefox117 to wontfix to because I don't believe this is worthy of an uplift.
The user would need to set very off-the-beaten path preferences and then delay a thread to take advantage of this.

Comment 10

[Tom Ritter [:tjr]]

Attachment: advisory.txt