1880692 - (CVE-2024-2608) Write beyond bounds caused by AppendEncodedAttributeValue() et al

Status: VERIFIED FIXED

(Core :: DOM: Serializers, defect)

Description

[mozillabugs]

Attachment: ffbug_2260.zip

AppendEncodedAttributeValue(), ExtraSpaceNeededForAttrEncoding() and AppendEncodedCharacters() (dom/base/nsContentUtils.cpp) can experience integer overflows, causing underallocation of an output buffer and subsequent writes beyond bounds when StringBuilder::ToString() is called to obtain the resulting string. The write occurs in a content process, and is of arbitrary attacker-provided data arbitrarily intermixed with the repeated string " or one of a few other escaped sequences. The length of the attacker-provided data and the length of the repeated strings must sum to >= 0x100000000.

Attached is a POC that demonstrates the bugs in the context of AppendEncodedAttributeValue().

The bugs are that the problem functions add uint32_t values -- in particular, lengths derived from strings that can be very long -- without checking for overflow. Lines 9360, 9370, and 9320, plus every instance of extraSpaceNeeded += <something> in ExtraSpaceNeededForAttrEncoding() and AppendEncodedCharacters() are vulnerable. (The following code is from FIREFOX_122_0_RELEASE):

    9326: static uint32_t ExtraSpaceNeededForAttrEncoding(const nsAString& aValue) {
    9327:   const char16_t* c = aValue.BeginReading();
    9328:   const char16_t* end = aValue.EndReading();
    9329: 
    9330:   uint32_t extraSpaceNeeded = 0;
    9331:   while (c < end) {
    9332:     switch (*c) {
    9333:       case '"':
    9334:         extraSpaceNeeded += ArrayLength("&quot;") - 2;
    9335:         break;
    ... (similar for other escaped chars)
    9344:     }
    9345:     ++c;
    9346:   }
    9347: 
    9348:   return extraSpaceNeeded;
    9349: }

    9351: static void AppendEncodedAttributeValue(const nsAttrValue& aValue,
    9352:                                         StringBuilder& aBuilder) {
    9353:   if (nsAtom* atom = aValue.GetStoredAtom()) {
    9354:     nsDependentAtomString atomStr(atom);
    9355:     uint32_t space = ExtraSpaceNeededForAttrEncoding(atomStr);
    9356:     if (!space) {
    ...
    9358:     } else {
    9359:       aBuilder.AppendWithAttrEncode(nsString(atomStr),
    9360:                                     atomStr.Length() + space);
    9361:     }
    9362:     return;
    9363:   }
    ...
    9366:   nsString str;
    9367:   aValue.ToString(str);
    9368:   uint32_t space = ExtraSpaceNeededForAttrEncoding(str);
    9369:   if (space) {
    9370:     aBuilder.AppendWithAttrEncode(std::move(str), str.Length() + space);
    9371:   } else {
    ...
    9373:   }
    9374: }

    9271: static void AppendEncodedCharacters(const nsTextFragment* aText,
    9272:                                     StringBuilder& aBuilder) {
    9273:   uint32_t extraSpaceNeeded = 0;
    9274:   uint32_t len = aText->GetLength();
    9275:   if (aText->Is2b()) {
    9276:     const char16_t* data = aText->Get2b();
    9277:     for (uint32_t i = 0; i < len; ++i) {
    9278:       const char16_t c = data[i];
    9279:       switch (c) {
    ...
    9289:         case 0x00A0:
    9290:           extraSpaceNeeded += ArrayLength("&nbsp;") - 2;
    9291:           break;
    9292:         default:
    9293:           break;
    9294:       }
    9295:     }
    9296:   } else {
    ... (similar for 1-byte characters)
    9317:   }
    9318: 
    9319:   if (extraSpaceNeeded) {
    9320:     aBuilder.AppendWithEncode(aText, len + extraSpaceNeeded);
    ...
    9323:   }
    9324: }

Use the attached POC this way:

1. Unzip ffbug_2260_11.zip, yielding ffbug_2260_11.htm and ffbug_2260_11_worker.js .
2. Put these files on a webserver (I used a local webserver at 127.0.0.1).
3. Start FF and attach a debugger to it.
4. Set a BP on AppendEncodedAttributeValue() line 9369.
5. Load ffbug_2260_11.htm in FF.
6. When the BP fires, examine space and notice that it's 0xd5552000.
7. Examine str.Length() and notice that it's 0x2aaae040, which, when added to space using 32-bit math, yields the overflowed quantity 0x40.
8. Step into StringBuilder::AppendWithAttrEncode() and notice that it adds a Unit having this length, also adding the same quantity to StringBuilder::mLength, which is the total length of the string that StringBuilder::ToString will attempt to allocate later.
9. Set a BP on StringBuilder::ToString lines 9125 (output buffer allocation) and 9145 (the Unit::Type::StringWithEncode case) and proceed.
10. When the buffer-allocation BP fires, notice that mLength is tiny (probably 0x50); this is because of the overflow that occurred in step 7. (The small additional string length of 0x10 is for the other elements that surround the gigantic string in the POC).
11. Proceed. When the StringWithEncode BP fires, step into EncodeAttrString() and watch it write the attacker-provided data (and the escaped strings) far beyond the end of the output buffer allocated in step 10 (in my test, the buffer appeared to have mCapacity == 0x7b).
12. Proceed. FF may crash copying the long string contents into the heap, or a different thread may malfunction or crash as it uses the data that EncodeAttrString() sprayed over the heap.

The POC uses a worker thread to illustrate what an attacker might do to stir the pot so that the heap-spraying is effective. In limited tests with an unoptimized x64 build without _DEBUG, only the ToString() thread crashes about 70% of the time, with both the ToString() thread and one or more other threads crashing most of the remainder. To see multiple crashes, proceed after the debugger pops up with the first thread's crash. Frequent crash locations are in arena_t::ArenaRunRegAlloc() on mask = aRun->mRegionsMask[i]; in WorkletThread::IsOnWorkletThread() on return ccjscx && ccjscx->GetAsWorkletJSContext();, and in GetCurrentThreadWorkerPrivate() on WorkerJSContext* workerjscx = ccjscx->GetAsWorkerJSContext();

Comment 1

[mozillabugs]

Attachment: ffbug_2260_11.zip

Comment 2

[Andrew McCreight [:mccr8]]

I guess I'll make this csectype-intoverflow, as that's the underlying problem here, though it is interesting that I think this won't show up on UBSan because the overflow is on an unsigned int, so it is defined behavior.

Comment 3

[Andrew McCreight [:mccr8]]

The overflow issue in the string builder being used here was fixed in bug 1512758.

Comment 4

[mozillabugs]

Not exactly. That overflow was due to StringBuilder::mLength overflowing, which was indeed fixed (by turning it into a CheckedInt). The problem here is that that various len + extraSpaceNeeded computations can overflow, as can the computations of extraSpaceNeeded itself. That's why this bug still exists in FF 122.0, as opposed to getting fixed in ~66.0.

Comment 5

[Andrew McCreight [:mccr8]]

Sorry, I didn't mean the issue you reported here was fixed. I meant that a similar integer overflow in StringBuilder itself was fixed by that other bug.

Comment 6

[Andrew McCreight [:mccr8]]

When I ran the test case locally on an unmodified debug build, I hit this assertion which looks like a failed bounds check:

Assertion failure: mPosition + len <= mHandle.Length(), at dom/base/nsContentUtils.cpp:9015
#01: (anonymous namespace)::StringBuilder::ToString(nsTSubstring<char16_t>&) (dom/base/nsContentUtils.cpp:9169)
#02: nsContentUtils::SerializeNodeToMarkup(nsINode*, bool, nsTSubstring<char16_t>&) (obj-dbg.noindex/dist/include/mozilla/dom/NodeInfo.h:0)

I considered whether we should make this a release assert, but it looks like the whole point of bug 1488697 which added this bulk writer mechanism is to eliminate redundant checks, so I'll leave it alone.

Hopefully doing CheckedInt everywhere won't slow things down too much. We could avoid the checks in the inner loops of AppendEncodedCharacters and ExtraSpaceNeededForAttrEncoding by using an unchecked version if the worst case scenario (eg 5 * len) won't overflow, but hopefully that's not necessary. I can try checking the microbenchmark linked to in bug 1488697, though it is so old I don't know how relevant it will be now.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1880692, part 2 - Use an estimate for encoded characters.

Comment 8

[mozillabugs]

(In reply to Andrew McCreight [:mccr8] from comment #6)

When I ran the test case locally on an unmodified debug build, I hit this assertion which looks like a failed bounds check:

Assertion failure: mPosition + len <= mHandle.Length(), at dom/base/nsContentUtils.cpp:9015
#01: (anonymous namespace)::StringBuilder::ToString(nsTSubstring<char16_t>&) (dom/base/nsContentUtils.cpp:9169)
#02: nsContentUtils::SerializeNodeToMarkup(nsINode*, bool, nsTSubstring<char16_t>&) (obj-dbg.noindex/dist/include/mozilla/dom/NodeInfo.h:0)

I considered whether we should make this a release assert, but it looks like the whole point of bug 1488697 which added this bulk writer mechanism is to eliminate redundant checks, so I'll leave it alone.

That would reduce this bug to a DoS bug, but those are undesireable too.

Comment 9

[Andrew McCreight [:mccr8]]

Eh, if we crashes in that situation it wouldn't be too terrible. Your test case doesn't feel like something people are going to hit normally.

My patch is a fairly severe performance regression. When running https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034 locally in a MacOS non-PGO opt build without the patch, I get around 58ms for body.innerHTML and body.outerHTML (about what I see on Nightly in my regular browsing session). With my patch, it is more like 76ms for both. I'll assume that's not okay and try to do something clever.

Comment 10

[Andrew McCreight [:mccr8]]

If I revert the CheckedInt changes for extraSpaceNeeded, that seems to fix the performance regression entirely, as you might expect. I'll see if I can come up with something tomorrow that bypasses the checking in the usual case.

Comment 11

[mozillabugs]

(In reply to Andrew McCreight [:mccr8] from comment #9)

My patch is a fairly severe performance regression. When running https://bug-81214-attachments.webkit.org/attachment.cgi?id=132034 locally in a MacOS non-PGO opt build without the patch, I get around 58ms for body.innerHTML and body.outerHTML (about what I see on Nightly in my regular browsing session). With my patch, it is more like 76ms for both. I'll assume that's not okay and try to do something clever.

That really surprises me. It maybe indicates that CheckedInt could use some performance work.

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1880692, part 1 - Make StringBuilder::Unit::mLength specific to literals.

That is the only case that uses it. This might save a bit of space
and avoids some useless stores. Idea by peterv.

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 9381124 [details]
Bug 1880692, part 2 - Use an estimate for encoded characters.

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It is probably easy to figure out how to get a buffer overflow if you look at the patch. (Technically this is a performance improvement so we could potentially land this in a cover bug with that excuse, but the addition of CheckedInt is probably a red flag if somebody was taking a close look at this patch.)
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: It looks like this code hasn't really changed since 114 (bug 1830986), so it shouldn't be an issue.
• How likely is this patch to cause regressions; how much testing does it need?: I think the main risk here is that this code is quite performance sensitive. The entire reason we have this odd code that led to a buffer overflow in the first place is due to performance tuning. My first patch slowed down a microbenchmark by a lot. The new patch speeds us up on the microbenchmark, but it does so by increasing memory usage a little bit. Hopefully that increase is not enough to matter.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 14

[Tom Ritter [:tjr]]

Comment on attachment 9381857 [details]
Bug 1880692, part 1 - Make StringBuilder::Unit::mLength specific to literals.

Approved to land and uplift

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/866922425c5e
part 1 - Make StringBuilder::Unit::mLength specific to literals. r=smaug
https://hg.mozilla.org/integration/autoland/rev/9669232a5702
part 2 - Use an estimate for encoded characters. r=smaug

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/866922425c5e
https://hg.mozilla.org/mozilla-central/rev/9669232a5702

Comment 17

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

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

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

For more information, please visit BugBot documentation.

Comment 18

[Andrew McCreight [:mccr8]]

I'll put in an uplift request on Monday. I think we're still early in the cycle and a few more days might let any performance issues shake out.

Comment 19

[Andrew McCreight [:mccr8]]

Here's a simplified version of the steps to reproduce from comment 0. Basically I removed all of the debugger steps, which are not necessary for verification in my experience. (I did not actually go through the debugger steps myself, though the description was useful for understanding the issue.

1. Download the attachment ffbug_2260_11.zip and unzip it in a new directory, yielding ffbug_2260_11.htm and ffbug_2260_11_worker.js. Note that ffbug_2260_11.htm is around 683MB.
2. Start a webserver hosting these files. On MacOS, I did this by running python3 -m http.server in the directory I put the files from the previous step.
3. Start Firefox.
4. Load ffbug_2260_11.htm in Firefox. With the way I set up the webserver, I was able to do this by visiting http://localhost:8000/ffbug_2260_11.htm.

Expected behavior: the page loads, but nothing happens, resulting in a blank tab.

Actual behavior (unpatched): the tab crashes. This shouldn't take more than a few seconds.

Comment 20

[Andrew McCreight [:mccr8]]

Comment on attachment 9381857 [details]
Bug 1880692, part 1 - Make StringBuilder::Unit::mLength specific to literals.

Beta/Release Uplift Approval Request

• User impact if declined: sec-high
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 19. It might be hard to verify on lower memory configurations, like 32-bit and Android, but the code is platform independent so I think it is okay to skip those if it is too difficult.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This makes us use slightly more memory in some situations, but I don't think it'll matter in any realistic scenario.
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
• User impact if declined: sec-high
• Fix Landed on Version: 125
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This makes us use slightly more memory in some situations, but I don't think it'll matter in any realistic scenario.

Comment 21

[Andrew McCreight [:mccr8]]

Comment on attachment 9381124 [details]
Bug 1880692, part 2 - Use an estimate for encoded characters.

Beta/Release Uplift Approval Request

• User impact if declined: (I got an error when trying to fill out the uplift request for beta before. See all of my previous answers.)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see above
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): see above
• String changes made/needed: none
• Is Android affected?: Yes

Comment 22

[Andrew McCreight [:mccr8]]

Would you mind putting together a version of your test case that dynamically constructs the attack string instead of including it inline? It would be nice to have a test case for this in-tree, but a 700MB test case is obviously a bit hard to deal with. Thanks.

Comment 23

[mozillabugs]

(In reply to Andrew McCreight [:mccr8] from comment #22)

Would you mind putting together a version of your test case that dynamically constructs the attack string instead of including it inline? It would be nice to have a test case for this in-tree, but a 700MB test case is obviously a bit hard to deal with. Thanks.

Try the new attachments ffbug_2060_20.htm and ffbug_2260_20_worker.js. This POC crashes other threads even more frequently than the previous version, probably because there's more JS running.

Comment 24

[mozillabugs]

Attachment: ffbug_2260_20.htm

Comment 25

[mozillabugs]

Attachment: ffbug_2260_20_worker.js

Comment 26

[Bogdan Maris, Desktop Test Engineering]

Using old Nightly from 2024-02-20 and the steps provided in comment 19 I was able to reproduce the crash (https://crash-stats.mozilla.org/report/index/4b6ebcd8-6dcf-436c-bd2d-774b50240228).
Using latest Nightly 125.0a1 from today I was not able to get the crash anymore and the expected result from comment 19 is met "Expected behavior: the page loads, but nothing happens, resulting in a blank tab.". Testing was done on macOS 13.6, Windows 10 64bit and Ubuntu 22.04.

Comment 27

[Dianna Smith [:diannaS]]

Comment on attachment 9381857 [details]
Bug 1880692, part 1 - Make StringBuilder::Unit::mLength specific to literals.

Approved for 124.0b6

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/releases/mozilla-beta/rev/516d301d7194
part 1 - Make StringBuilder::Unit::mLength specific to literals. r=smaug, a-dsmith
https://hg.mozilla.org/releases/mozilla-beta/rev/0d288e36a43a
part 2 - Use an estimate for encoded characters. r=smaug, a-dsmith

Comment 29

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris, Desktop QA from comment #26)

Using old Nightly from 2024-02-20 and the steps provided in comment 19 I was able to reproduce the crash (https://crash-stats.mozilla.org/report/index/4b6ebcd8-6dcf-436c-bd2d-774b50240228).
Using latest Nightly 125.0a1 from today I was not able to get the crash anymore and the expected result from comment 19 is met "Expected behavior: the page loads, but nothing happens, resulting in a blank tab.". Testing was done on macOS 13.6, Windows 10 64bit and Ubuntu 22.04.

Same thing using the latest Beta 124 build from treeherder on the same OSs. Not marking as verified until ESR gets the fix as well when the time comes.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9381124 [details]
Bug 1880692, part 2 - Use an estimate for encoded characters.

Approved for 115.9esr.

Comment 31

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/ab88c34b160c
https://hg.mozilla.org/releases/mozilla-esr115/rev/8986273499ef

Comment 32

[Bogdan Maris, Desktop Test Engineering]

Also verified using the latest Firefox 115 ESR build from treeherder across platforms (Windows 10 64bit, macOS 13.6 and Ubuntu 22.04) that this is fixed.

Comment 33

[Tyson Smith [:tsmith]]

Attachment: advisory.txt