1870414 - (CVE-2024-1556) Potential access beyond bounds caused by InChunkPointer::ShouldPointAtValidBlock()

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[mozillabugs]

InChunkPointer::ShouldPointAtValidBlock() (mozglue/baseprofiler/public/ProfileChunkedBufferDetail.h) contains an invalid check, potentially allowing an access beyond bounds. The bug is at lines 389-90 (FIREFOX_120_0_1_RELEASE), which check IsNull() against *this, not against pointer.

If untrusted code can determine entrySize (line 381), then it might be able to cause an access beyond bounds in code that relies on ShouldPointAtValidBlock() to perform the correct check.

372:[[nodiscard]] bool ShouldPointAtValidBlock() const {
373:    if (IsNull()) {
374:      // Pointer is null, no blocks here.
375:      MOZ_ASSERT(false, "ShouldPointAtValidBlock - null pointer");
376:      return false;
377:    }
378:    // Use a copy, so we don't modify `*this`.
379:    InChunkPointer pointer = *this;
380:    // Try to read the entry size.
381:    Length entrySize = pointer.ReadEntrySize();
382:    if (entrySize == 0) {
383:      // Entry size of zero means we read 0 or a way-too-big value.
384:      MOZ_ASSERT(false, "ShouldPointAtValidBlock - invalid size");
385:      return false;
386:    }
387:    // See if the last byte of the entry is still inside the buffer.
388:    pointer += entrySize - 1;
389:    MOZ_ASSERT(!IsNull(), "ShouldPointAtValidBlock - past end of buffer");
390:    return !IsNull();
391: }

Comment 1

[Daniel Veditz [:dveditz]]

Nazım: can you take a look and see what the impact would be?

Comment 2

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

It looks like a valid bug. I think the impact of it is relatively low, because this code only runs when the profiler is active and running. So it requires users to do extra steps to enable it and run it. But this looks like something we should fix nevertheless.

I'll look into fixing it soon, but I believe most people who can review this are on PTO. I will try to find a person to review but it might wait until the new year until people are back.

Comment 3

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Attachment: Bug 1870414 - Check the correct buffer block r?#profiler

Comment 4

[Andrew McCreight [:mccr8]]

I'll mark this sec-moderate, though it might be a little high. I could imagine a web page being able to affect how much stuff the profiler is recording.

Comment 5

[Julien Wajsberg [:julienw]]

After discussion we thought that it's probably low impact. We think that there are other safeguards than just this code. Proof is that there are debug assertions, which, to me, points to the fact that the original author thought they wouldn't be hit thanks to some other logic. Of course in this case the condition is incorrect, but the intention shows that to me.

Comment 6

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Comment on attachment 9369840 [details]
Bug 1870414 - Check the correct buffer block r?#profiler

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it couldn't be easily constructed. The profiler needs to be active and running. And also we have some more safeguards to make sure that we point out to a valid buffer location. See Julien's message above.
• 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 older supported branches are affected by this flaw?:
• If not all supported branches, which bug introduced the flaw?: Bug 1753192
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I don't think it will be risky or hard considering that we didn't touch that code a lot.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely to cause. Did manual testing and it's all good.
• Is Android affected?: Yes

Comment 7

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Requesting a sec-approval just in case. Please let me know what you think.

Also, "Fixing Security Bugs" document mentions that I should remove the bug number from the commit message. Does it apply to sec-low as well?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #7)

Requesting a sec-approval just in case. Please let me know what you think.

You don't need sec-approval except for sec-high and sec-critical bugs that affect more than Nightly.

Also, "Fixing Security Bugs" document mentions that I should remove the bug number from the commit message. Does it apply to sec-low as well?

It actually says you should remove everything EXCEPT for the bug number. Please always include the bug number. I think it is better to also include a bit of a description, but nothing beyond what a reasonable person would figure out from looking at the patch.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 9369840 [details]
Bug 1870414 - Check the correct buffer block r?#profiler

sec-low and sec-moderate bugs don't need sec-approval.

Comment 10

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

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

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #7)

Requesting a sec-approval just in case. Please let me know what you think.

You don't need sec-approval except for sec-high and sec-critical bugs that affect more than Nightly.

Ah, thanks. I just wanted to be sure.

Also, "Fixing Security Bugs" document mentions that I should remove the bug number from the commit message. Does it apply to sec-low as well?

It actually says you should remove everything EXCEPT for the bug number. Please always include the bug number. I think it is better to also include a bit of a description, but nothing beyond what a reasonable person would figure out from looking at the patch.

Huh, I think got confused by this sentence: "Definitely don’t include the bug number in the commit message."
It's here: https://searchfox.org/mozilla-central/rev/8461ad1fc943ac560414322b66de5929ef10f706/docs/bug-mgmt/processes/fixing-security-bugs.rst#117-118
But I guess that's when you need to obfuscate your code.

Ok, then I will push it like this. Thank you for the answer!

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Nazım Can Altınova [:canova][:canaltinova on phabricator] from comment #10)

Huh, I think got confused by this sentence: "Definitely don’t include the bug number in the commit message."
It's here: https://searchfox.org/mozilla-central/rev/8461ad1fc943ac560414322b66de5929ef10f706/docs/bug-mgmt/processes/fixing-security-bugs.rst#117-118
But I guess that's when you need to obfuscate your code.

I think that's mostly about pushing to Try, but I guess the phrasing is a bit convoluted.

Comment 12

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

Oh sorry I missed that part. Thanks for clarifying.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14c89fd054c7
Check the correct buffer block r=profiler-reviewers,julienw

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/14c89fd054c7

Comment 15

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

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

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

For more information, please visit BugBot documentation.

Comment 16

[Nazım Can Altınova [:canova][:canaltinova on phabricator]]

I think it's safe enough that it's okay to ride the train.

Comment 17

[Daniel Veditz [:dveditz]]

We are awarding this as a "sec-low" bounty, but we would increase it back to sec-moderate if you can demonstrate a page that can trigger this condition when profiled.

Comment 18

[Tom Ritter [:tjr]]

Attachment: advisory.txt