1757805 - Shmem stores length in shared memory region

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

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

The Shmem type stores its length within its own shared memory region, and only validates it the first time it is received over IPC (https://searchfox.org/mozilla-central/rev/48c71577cbb4ce5a218cf4385aff1ff244dc4432/ipc/glue/Shmem.cpp#404-407). In future times it is received over IPC, the length value is read out of the shared memory region without validation (https://searchfox.org/mozilla-central/rev/48c71577cbb4ce5a218cf4385aff1ff244dc4432/ipc/glue/Shmem.h#91, https://searchfox.org/mozilla-central/rev/48c71577cbb4ce5a218cf4385aff1ff244dc4432/ipc/glue/Shmem.cpp#456).

A malicious peer process could modify this length property within the shared memory region at runtime, as it is stored in mutable shared memory. As the length is not re-checked when the region is received for a second time, this could allow a compromised process to cause another process to buffer-overflow and read data it should not have access to.

This should be fixed by no longer storing the size field within the shared memory region proper and instead storing it directly in the table alongside the shared memory segment. Alternatively, or in addition, we could also fix the issue by no longer performing multiple sends which only use an ID to reference the same shared memory region as is proposed in bug 1757802.

Comment 1

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

One other option which may be easier to uplift and would require fewer changes would be to compare the mSize loaded from the shared memory region to the (known) true total size of the shared memory region, and double-check that the sizes line up.

Comment 2

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

Attachment: Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

Comment 3

[Frederik Braun [:freddy]]

This patch is simple and important enough to consider wide uplifts, they apply to all channels quite easily as you say, Nika.
(It's worth following-up with not storing the size in the shared memory segment (I suppose that's bug 1757802), but this one here should take priority)

Comment 4

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

(In reply to Frederik Braun [:freddy] from comment #3)

This patch is simple and important enough to consider wide uplifts, they apply to all channels quite easily as you say, Nika.
(It's worth following-up with not storing the size in the shared memory segment (I suppose that's bug 1757802), but this one here should take priority)

Yeah, that patch stops storing the length in the segment entirely, but I wanted a simple & easy to uplift patch.

Comment 5

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

Comment on attachment 9266135 [details]
Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

Security Approval Request

• How easily could an exploit be constructed based on the patch?: A dedicated attacker could probably figure out what is not being checked, and turn this into a sandbox escape from a non-sandboxed process, though it wouldn't be trivial to do.
  Given that the patch just adds an assertion, I would say that someone reading the patch can easily figure out what the problem is.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
• Which older supported branches are affected by this flaw?: all
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: I believe that this patch should apply cleanly to all branches
• How likely is this patch to cause regressions; how much testing does it need?: This patch just adds extra assertions for known invariants. It shouldn't cause any regressions.

Beta/Release Uplift Approval Request

• User impact if declined: Potential sandbox escape exploit
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch just adds extra assertions for known invariants. It shouldn't cause any regressions.
• String changes made/needed: none

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential sandbox escape exploit
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch just adds extra assertions for known invariants. It shouldn't cause any regressions.

Comment 6

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9266135 [details]
Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

Given the low risk and the high impact I would prefer to land this later in the cycle. Not much left, but last uplifts are the 24th; so if we land this Monday the 21st or Tuesday the 22nd we should have enough time to detect an unexpected blow-up and avoid uplifting if that is the case; either is fine.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9266135 [details]
Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

This isn't something we're going to take in a 98 dot release.

Comment 8

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

(In reply to Tom Ritter [:tjr] (ni? for response to CVE/sec-approval/advisories/etc) from comment #6)

Given the low risk and the high impact I would prefer to land this later in the cycle. Not much left, but last uplifts are the 24th; so if we land this Monday the 21st or Tuesday the 22nd we should have enough time to detect an unexpected blow-up and avoid uplifting if that is the case; either is fine.

Sounds good. I'll hold off landing it until early next week.

Comment 9

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

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

For more information, please visit auto_nag documentation.

Comment 10

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

Add additional assertions around shmem size, r=ipc-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/78279d1fb34e08495a734d4f17fb2576d4f8f542
https://hg.mozilla.org/mozilla-central/rev/78279d1fb34e

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9266135 [details]
Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

Approved for 99.0b7. Thanks.

Comment 12

[Donal Meehan [:dmeehan]]

Add additional assertions around shmem size, r=ipc-reviewers,handyman
https://hg.mozilla.org/releases/mozilla-beta/rev/ab1a8d7c93fc

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9266135 [details]
Bug 1757805 - Add additional assertions around shmem size, r=#ipc-reviewers

Approved for 91.8esr.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/372c9410ea69

Comment 15

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.