1466877 - Web Replay: Support sandboxing

Status: RESOLVED FIXED

(Core Graveyard :: Web Replay, enhancement)

Description

[Brian Hackett [Laid off!]]

Currently, the process sandbox is turned off in recording, replaying, and middleman processes.  The attached patches, and changes in other bugs as noted, fix this so that the sandbox is enabled in all three types of processes.

The behaviors this changes are as follows:

- Replaying processes no longer create temporary files for storing snapshot data.  The patches in bug 1464903 have been updated to reflect this.

- Some new redirections were needed to record/replay the sandbox initialization code.  The APIs which actually initialize the sandbox itself are redirected, and ensure that the sandbox initialization is performed on the process when both recording and replaying.

- Middleman processes do not directly spawn replaying children or open socket file descriptors to communicate with those replaying children, but send IPDL messages to their ContentParent to do so on their behalf.  The middleman still does these tasks for its recording child (if any) itself, since this needs to be done at startup before the PContent protocol has been initialized.

- The UI process keeps track of the recording file and picks a temporary recording file if one is desired.  This and other UI process changes are in bug 1465287 part 8.

- The UI process sends a file descriptor to the middleman process for making a copy of the recording, rather than letting it open the file descriptor itself.

- The mach messages sent back and forth between the middleman and child processes to share handles to the graphics shmem have been reworked to be sandbox friendly.

Recording/replaying/middleman processes still open handles to the recording file directly, but do this before the sandbox is initialized (and before any untrusted data is accessed).

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Sandbox redirections.

New redirections required for sandboxing, plus a little cleanup.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Child side changes for sandboxing.

Recording/replaying processes no longer need to delete snapshot files on exit, but need a new command line argument for passing in the middleman pid, since their parent could be either the middleman or the UI process.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Middleman changes for sandboxing.

Changes to the middleman to ask the UI process to spawn replaying processes and open IPC channels on its behalf, to provide an API that the UI process can call to do these tasks, and to avoid opening recording file descriptors after the sandbox has been initialized.

Comment 4

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Graphics shmem changes for sandboxing.

Changes to mach messages sent while sharing the graphics shmem.  The middleman needs to open a receive port before the sandbox has been initialized, then wait for each child process to send it a port which it can use to send that child another port which will give the child access to the shmem.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8983458 [details] [diff] [review]
Part 3 - Middleman changes for sandboxing.

Review of attachment 8983458 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/recordreplay/ipc/ChildProcess.cpp
@@ +460,5 @@
> +    if (!gRecordingProcess->LaunchAndWaitForProcessHandle(extraArgs)) {
> +      MOZ_CRASH("ChildProcess::LaunchSubprocess");
> +    }
> +  } else {
> +    dom::ContentChild::GetSingleton()->SendCreateReplayingProcess(channelId);

Where is CreateReplayingProcess added to PContent.ipdl? I don't see it anywhere.

@@ +513,5 @@
>    }
>  
>    mNumRestarts++;
>  
> +  dom::ContentChild::GetSingleton()->SendTerminateReplayingProcess(mChannel->GetId());

Likewise.

::: toolkit/recordreplay/ipc/ParentIPC.cpp
@@ -693,2 @@
>  
> -  PrintSpew("Copied Recording %s\n", aFilename);

Maybe you still want the spew to indicate that this process has finished the copying, even if you don't know the file name?

@@ +1182,5 @@
>    }
> +
> +  // Open a file handle to the recording file we can use for saving recordings
> +  // later on.
> +  gRecordingFd = DirectOpenFile(gRecordingFilename, false);

This is the middleman process, right? How can it open a file in the presence of sandboxing? Is this early enough that the sandbox isn't active or something?

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> Comment on attachment 8983458 [details] [diff] [review]
> Part 3 - Middleman changes for sandboxing.
> 
> Review of attachment 8983458 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/recordreplay/ipc/ChildProcess.cpp
> @@ +460,5 @@
> > +    if (!gRecordingProcess->LaunchAndWaitForProcessHandle(extraArgs)) {
> > +      MOZ_CRASH("ChildProcess::LaunchSubprocess");
> > +    }
> > +  } else {
> > +    dom::ContentChild::GetSingleton()->SendCreateReplayingProcess(channelId);
> 
> Where is CreateReplayingProcess added to PContent.ipdl? I don't see it
> anywhere.

The PContent.ipdl changes and other UI process changes are in part 8 of bug 1465287, which is a little more intertwined with the middleman stuff now that the latter has to ask the UI process to take care of such tasks.

> @@ +1182,5 @@
> >    }
> > +
> > +  // Open a file handle to the recording file we can use for saving recordings
> > +  // later on.
> > +  gRecordingFd = DirectOpenFile(gRecordingFilename, false);
> 
> This is the middleman process, right? How can it open a file in the presence
> of sandboxing? Is this early enough that the sandbox isn't active or
> something?

Yes, the sandbox is not active until after a PContent::SetProcessSandbox message is sent by the UI process.  The middleman does some tasks beforehand that it cannot do afterwards, including opening this fd, opening a mach port it can use to share memory with child processes, and (if necessary) spawning a recording process and opening a channel with it.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/601b20ba619b
Part 1 - Sandbox redirections, r=froydnj.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8e773fd295e
Part 2 - Child side changes for sandboxing, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e331895810fb
Part 3 - Middleman changes for sandboxing, r=mccr8.
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc52c82903fc
Part 4 - Graphics shmem changes for sandboxing, r=jld.

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/601b20ba619b
https://hg.mozilla.org/mozilla-central/rev/f8e773fd295e
https://hg.mozilla.org/mozilla-central/rev/e331895810fb
https://hg.mozilla.org/mozilla-central/rev/dc52c82903fc