1949987 - (CVE-2025-3032) Leak FDs of ForkServer process to content processes

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Thinker Li [:sinker]]

I run nightly build on my desktop. For some reason, the forkserver die without being noticed by the parent process (ForkServiceChild). It causes the parent wait for content process forever since the preallocated process never reports that it is ready. After a short investigation, I realize that the socket used to talk with the forkserver is still alive while the forkserver is already dead. Then, I realize that the other end of the sockets are not closed after forking content processes.

After launching firefox, the content of /proc/<forkserver-pid>/fd looks like.
$ ls -l /proc/<forkserver-pid>/fd
lrwx------ 1 thinker thinker 64 Feb 22 20:40 0 -> /dev/pts/2
lrwx------ 1 thinker thinker 64 Feb 22 20:40 1 -> /dev/pts/2
lrwx------ 1 thinker thinker 64 Feb 22 20:40 2 -> /dev/pts/2
lrwx------ 1 thinker thinker 64 Feb 22 20:40 3 -> 'socket:[643257]'
lrwx------ 1 thinker thinker 64 Feb 22 20:40 4 -> 'socket:[645653]'
lrwx------ 1 thinker thinker 64 Feb 22 20:40 5 -> 'socket:[645650]'
lr-x------ 1 thinker thinker 64 Feb 22 20:40 6 -> /dev/null
lr-x------ 1 thinker thinker 64 Feb 22 20:40 7 -> /dev/null
lr-x------ 1 thinker thinker 64 Feb 22 20:40 8 -> /dev/null

Check FDs of any content process, the information of these file descriptors are all the same. I mean FD 3 in every content process always points to socket:[643257]. FD 4, fd 5, ... etc all point to same objects. That means these file descriptors are leaked to content processes. We should set O_CLOEXEC on these file descriptors, especially sockets, or just close them after fork.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I'm going to assume it's been like this since bug 1440207 (part 3j), and I thought I checked for fd leaks but it looks as if I misundertood something important.

O_CLOEXEC won't help, because we're not execing. I think the fork server should use CloseSuperfluousFds to close everything except the fds that were given to GeckoArgs for the child process.

Also, this is security sensitive — a hostile child process could read the information intended for a different process launch, which I think will allow it to impersonate that process by stealing its initial channel ID.

Comment 2

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1440207

:nika, since you are the author of the regressor, bug 1440207, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

I've been able to build old versions and I can confirm that bug 1440207 is the regressor.

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Attachment: WIP: Bug 1949987.

Comment 5

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

Oh I thought I had handled that in bug 1440207, but it looks like I didn't. Oops. Looking at a command line for the fork server, we can see it looks roughly like this:

firefox -contentproc -ipcHandle 0 -sandboxReporter 1 -initialChannelId {ec4848ce-e997-43ca-9f63-3e1aaefb56f0} -parentPid 336992 -crashReporter 2 -appDir .... 2 forkserver

Which maps the FD arguments roughly as follows:

FD | CLI Argument
3  | -ipcHandle 0
4  | -sandboxReporter 1
5  | -crashReporter 2

We perhaps shouldn't have been passing the sandbox reporter and crash reporter arguments down into fork server process, as I doubt those arguments are consumed from the fork server command line (and thus closed).

I was surprised that the -ipcHandle 0 argument (fd 3) was shared between processes. In the fork server main this is taken from the command line here, and moved into the MiniTransceiver (https://searchfox.org/mozilla-central/rev/fa11d06fd84c487cb0a135c08af796daa6a56e44/ipc/glue/ForkServer.cpp#45-51). I think when writing the patch I expected MiniTransceiver to clean up the FD, but apparently it does not, leaking it into content processes. This is quite unfortunate and should definitely be fixed.

In terms of cleaning up unconsumed FD arguments which were sent down but never touched in the fork server, like the sandbox reporter and crash reporter FDs, in an ideal world we'd probably hold off on sending the arguments all-together, but perhaps we could also send down the # of FD arguments as an extra flag to the fork server, so that we can automatically clean up any unconsumed ones.

Comment 6

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

Attachment: Bug 1949987 - Don't pass unused FDs to the Fork Server, r=jld!

This removes 2 FD arguments which were passed to the fork server but not
used by the fork server. In addition, the lifecycle management for the
IPC FD used by the fork server was also cleaned up to make its lifespan
more explicit.

Comment 7

[Daniel Veditz [:dveditz]]

From a security POV how bad is it? A compromised child could impersonate another, breaking Fission? Could it impersonate one of the more privileged child processes? I think a lot of the privileged stuff is handled by whether specific JSACTORs are instantiated or not, so I think that would be mostly safe?

Comment 8

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

(In reply to Daniel Veditz [:dveditz] from comment #7)

From a security POV how bad is it? A compromised child could impersonate another, breaking Fission? Could it impersonate one of the more privileged child processes? I think a lot of the privileged stuff is handled by whether specific JSACTORs are instantiated or not, so I think that would be mostly safe?

Specifically a compromised child process could impersonate the fork server process. Theoretically with enough effort the compromised child could fully mimic the reply to a request to start up a new child process and then be able to impersonate that process (though it may be difficult, and would involve racing with the fork server).

Comment 9

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1440207

Comment 10

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

Comment on attachment 9468737 [details]
Bug 1949987 - Don't pass unused FDs to the Fork Server, r=jld!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would probably be relatively straightforward to identify the bug being fixed here relatively easily from the patch, however implementing a full exploit using this patch would likely be quite complex and fiddly to do.
• 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?: 133 (beta/release)
• If not all supported branches, which bug introduced the flaw?: Bug 1440207
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect this patch will apply cleanly to beta and release
• How likely is this patch to cause regressions; how much testing does it need?: The patch makes fairly straightforward changes to the fork server code which I do not expect will lead to any regressions.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: No

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/321a641155ac
Don't pass unused FDs to the Fork Server, r=jld

Comment 12

[Tom Ritter [:tjr]]

Comment on attachment 9468737 [details]
Bug 1949987 - Don't pass unused FDs to the Fork Server, r=jld!

Approved to land and request uplift

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/321a641155ac

Comment 14

[BugBot [:suhaib / :marco]]

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-firefox137 to wontfix.

For more information, please visit BugBot documentation.

Comment 15

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

Comment on attachment 9468737 [details]
Bug 1949987 - Don't pass unused FDs to the Fork Server, r=jld!

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Potential for sandbox escapes on Linux when fork server is in use.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• 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): Straightforward change which has been landed on nightly
• String changes made/needed: None
• Is Android affected?: No

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0c3fc282af4

Comment 17

[Tom Schuster [:tschuster]]

Attachment: advisory.txt