Closed Bug 1823172 (CVE-2023-5173) Opened 11 months ago Closed 7 months ago

Write beyond bounds in TransactionObserver::OnDataAvailable()

Categories

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

defect

Tracking

()

RESOLVED FIXED
118 Branch
Tracking Status
firefox-esr102 --- wontfix
firefox-esr115 - wontfix
firefox113 --- wontfix
firefox116 --- wontfix
firefox117 --- wontfix
firefox118 + fixed

People

(Reporter: mozillabugs, Assigned: acreskey)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [necko-triaged][necko-priority-queue] [adv-main118+])

Attachments

(4 files)

Attached file 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"
  1. 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
  1. Attach a debugger to FF and set a BP on line 824, above.
  2. Load ffbug_2106.htm in FF.
  3. 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.
  4. FF will accumulate > 4GB of memory usage. When it stops adding memory, use the debugger to break into execution.
  5. Thaw the thread that you froze in step 7. Proceed.
  6. When you get a second BP, aCount probably will be >= 0x10000, which would make the remainder of the function a no-op. Just proceed.
  7. 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.
  8. 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.
Flags: sec-bounty?
Attached file gen-attack.cpp
Group: core-security → network-core-security
Whiteboard: [necko-triaged][necko-priority-review]
Severity: -- → S2
Priority: -- → P2

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

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.

Whiteboard: [necko-triaged][necko-priority-review] → [necko-triaged][necko-priority-next]
Whiteboard: [necko-triaged][necko-priority-next] → [necko-triaged][necko-priority-queue]

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.

Assignee: nobody → acreskey
Pushed by acreskey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3987cdfccbc
Refactor TransactionObserver::OnDataAvailable() r=necko-reviewers,jesup
Group: network-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → 118 Branch

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.

Flags: needinfo?(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.

Flags: needinfo?(acreskey)
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify+
QA Whiteboard: [post-critsmash-triage]
Whiteboard: [necko-triaged][necko-priority-queue] → [necko-triaged][necko-priority-queue] [adv-main118+]
Attached file advisory.txt
Group: core-security-release
Alias: CVE-2023-5173
You need to log in before you can comment on or make changes to this bug.