Web Replay: Support sandboxing

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Trunk
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(4 attachments)

Assignee

Description

a year ago
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).
Assignee

Comment 1

a year ago
New redirections required for sandboxing, plus a little cleanup.
Assignee: nobody → bhackett1024
Attachment #8983453 - Flags: review?(nfroyd)
Assignee

Comment 2

a year ago
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.
Attachment #8983455 - Flags: review?(continuation)
Assignee

Comment 3

a year ago
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.
Attachment #8983458 - Flags: review?(continuation)
Assignee

Comment 4

a year ago
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.
Attachment #8983460 - Flags: review?(jld)
Attachment #8983455 - Flags: review?(continuation) → review+
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?
Attachment #8983458 - Flags: review?(continuation)
Assignee

Comment 6

a year ago
(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.
Attachment #8983453 - Flags: review?(nfroyd) → review+
Component: General → Web Replay
Attachment #8983460 - Flags: review?(jld) → review+

Comment 7

10 months ago
Pushed by bhackett@mozilla.com:
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.
You need to log in before you can comment on or make changes to this bug.