1820359 - Logic error and overflow in nsSegmentedBuffer causes underallocation and write beyond bounds (latent)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[mozillabugs]

Attachment: Toolbar.js

nsSegmentedBuffer::AppendNewSegment() (xpcom/io/nsSegmentedBuffer.cpp) incorrectly checks its current size against its maximum allowed size, and nsSegmentedBuffer::GetSize() uses 32 bit math. The first bug allows an oversized allocation to occur, which then invokes the second bug, causing overflow in nsSegmentedBuffer::GetSize(). If other code were to trust nsSegmentedBuffer::GetSize() to return the correct size by using its return value to allocate memory, then were to copy out the object's contents by calling nsSegmentedBuffer::GetSegment() in a loop, it would write beyond bounds if the object contained > 0xffffffff bytes of data.

I believe that this bug currently cannot be reached because searchfox says that nsSegmentedBuffer::AppendNewSegment() is the only caller of nsSegmentedBuffer::GetSize().

The bugs are on line 24 of the .cpp file (which incorrectly tests against the object's current size instead of the requested new size) and on line 59 of the .h file (nsSegmentedBuffer::GetSize()), which incorrectly uses 32-bit math.

(.cpp)
------
23: char* nsSegmentedBuffer::AppendNewSegment() {
24:   if (GetSize() >= mMaxSize) {
25:     return nullptr;
26:   }
27: ...
54:   char* seg = (char*)malloc(mSegmentSize);
55:   if (!seg) {
56:     return nullptr;
57:   }
58:   mSegmentArray[mLastSegmentIndex] = seg;
59:   mLastSegmentIndex = ModSegArraySize(mLastSegmentIndex + 1);
60:   return seg;
61: }

(.h)
------
59: inline uint32_t GetSize() { return GetSegmentCount() * mSegmentSize; }

Attached is a POC that shows the oversized allocation and subsequent overflow. Use the POC thusly:

1. Start with a built version of FF, not an installed one, because you will need to replace some JS files to create the chrome necessary to invoke nsSegmentedBuffer::AppendNewSegment().

2. Copy toolbar.js into dist/bin/browser/chrome/devtools/modules/devtools/client/netmonitor/src/components/toolsbar.js.

3. Copy har-menu-utils.js into dist/bin/browser/chrome/devtools/modules/devtools/client/netmonitor/src/har/har-menu-utils.js.

4. Start FF and open "tools/browser tools/web developer tools".

5. Click the "network settings" star icon, then wait a minute or so. (This step helps avoid spurious breakpoints). Now attach a debugger to FF.

6. Set a BP at the beginning of nsSegmentednsSegmentedBuffer::AppendNewSegment() and another on nsPipe::nsPipe() on the MOZ_ALWAYS_SUCCEEDS statement.

7. Click the "network settings" star icon and select "netmonitor.context.exerciseabug".

8. When the BP in nsPipe::nsPipe() fires, examine arguments aSegmentSize and aSegmentCount. If they are not 0x80000000 and 3, respectively, proceed until you get those argument values.

9. Doctor mMaxAdvanceBufferSegmentCount from 1 to 3. This makes it possible to invoke the bug using this POC.

10. Record the value of &this->mBuffer for later use. Proceed.

11. When the BP on nsSegmentedBuffer::AppendNewSegment() fires, proceed until this matches the address you recorded in step 10. When that happens, step over the call to GetSize() and note what it returns. On the first BP, it should be 0. Note that this isn't really what line 24 should be checking for! It should instead be checking whether the current size, plus the size of the new segment, would exceed mMaxSize. Proceed.

12. You'll get another BP on nsSegmentedBuffer::AppendNewSegment() with a matching this pointer. Again step over GetSize() and notice that it returns 0x80000000. Notice also that mSegmentSize is 0x80000000. Proceed.

13. You'll get a third BP on nsSegmentedBuffer::AppendNewSegment() with a matching this pointer. Again step over GetSize() and notice that it returns 0 (!), and that the if statement does not cause line 25 to be executed. This is because GetSize() overflowed because it uses 32-bit math.

Comment 1

[mozillabugs]

Attachment: har-menu-utils.js

Comment 2

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

The severity field is not set for this bug.
:nika, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1820359 - Apply nsSegmentedBuffer limits at the segment level, r=#xpcom-reviewers

Previously this would be checked at the byte count level, which could
end up being less precise.

Comment 4

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

Landed: https://hg.mozilla.org/integration/autoland/rev/fd2a3531261bad2a4de38828e6499e7a8cd7081c
Backed out for causing remote failures in browser_navigationEvents.js:
https://hg.mozilla.org/integration/autoland/rev/9ce119b474d9e5b847417099211c20547d7ddd1c

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=MvZuyXG5SbugIsCaBAPgJQ.0&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Ctest-linux1804-64-qr%2Fdebug-mochitest-remote%2Cremote&revision=fd2a3531261bad2a4de38828e6499e7a8cd7081c
Failure log: https://treeherder.mozilla.org/logviewer?job_id=409660337&repo=autoland

TEST-UNEXPECTED-FAIL | remote/cdp/test/browser/network/browser_navigationEvents.js | Received expected number of events - Got 3, expected 10
Followed by more failure lines.

Comment 5

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

Also puppeteer failures stem from this change: https://treeherder.mozilla.org/logviewer?job_id=409662642&repo=autoland&lineNumber=1903

TEST-UNEXPECTED-FAIL | Page.click should click the button (click.spec.js) | expected PASS

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1820359 - Move precise max length limits from `nsSegmentedBuffer` to callers, r=#xpcom-reviewers

This means that the size limits are more accurately applied, as they can
operate on sub-segment precision.

Comment 7

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

Move precise max length limits from nsSegmentedBuffer to callers, r=xpcom-reviewers,mccr8
https://hg.mozilla.org/integration/autoland/rev/9891ea31c16bb68c22388842ac007a7a7707754b
https://hg.mozilla.org/mozilla-central/rev/9891ea31c16b

Comment 8

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

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

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

For more information, please visit auto_nag documentation.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

No, this bug doesn't need to be uplifted. This is unexploitable with the current implementation.