The default bug view has changed. See this FAQ.

Share FileDescriptors across processes in preparation for OS-level sandbox

RESOLVED FIXED in mozilla17

Status

()

Core
IPC
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 650201 [details] [diff] [review]
Patch, v1

We're about to start sandboxing child processes to prevent them from opening arbitrary files (bug 776840). Once that happens any file-backed input streams will no longer function correctly. Attached patch fixes this problem for Blobs since they're currently the only consumer of the InputStream serializer in the parent->child direction.

So there are several parts to this patch. I have them separated (sorta) in MQ but each patch changes a subset of directories so I think reviewing different parts of the final patch is probably easier.

The first part renames nsIIPCSerializable -> nsIIPCSerializableObsolete. The old nsIIPCSerializable is a bit fragile (manually matching WriteParam/ReadParam calls, no support for passing special IPDL types like Shmem). This actually touches many different places in the patch, but it's all rubber-stamp anyway.

The second part adds a FileDescriptor class that IPDL treats specially (similar to Shmem). It can be used as an argument in IPDL protocols. When sending one of these to another process IPDL will do system-level calls to duplicate the file descriptor in the target process and then dig it out on the receiving side.

The third part introduces the new nsIIPCSerializable system using IPDL unions. The code is a little verbose but safer and allows passing special IPDL types like Shmem and FileDescriptor.

The fourth part adds new nsIIPCSerializable support to several input stream types that are needed to make my tests pass. There are others, of course, and we'll need to fix those in a followup.

The final part updates the Blob implementation to use the new nsIIPCSerializable implementation.
Comment on attachment 650201 [details] [diff] [review]
Patch, v1

I think khuey can rubber-stamp the nsIIPCSerializable->nsIIPCSerializableObsolete changes, and then review the new nsIIPCSerializable implementations on the stream classes, and the Blob changes.

cjones can have all the FileDescriptor and IPDL stuff!
Attachment #650201 - Flags: review?(khuey)
Attachment #650201 - Flags: review?(jones.chris.g)
blocking-basecamp: --- → ?
This work lets us deny all fs access to content processes, which blocks other blockers.
blocking-basecamp: ? → +
Created attachment 650452 [details] [diff] [review]
Fixes for POSIX

Passes all the IPC tests.
Comment on attachment 650201 [details] [diff] [review]
Patch, v1

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

::: dom/ipc/Blob.cpp
@@ +894,5 @@
> +    return false;
> +  }
> +
> +  nsCOMPtr<nsIFileInputStream> fileInputStream = do_QueryInterface(stream);
> +  if (fileInputStream) {

We should probably always do this.  How are you going to peer into a multiplex stream or a DataOwnerAdapter?

@@ +1057,5 @@
> +      serializable->Serialize(params);
> +
> +      if (params.type() !=
> +          IPCSerializableParams::TInputStreamSerializableParams) {
> +        MOZ_NOT_REACHED("Serialize returned an unexpected type!");

This will crash us if the file doesn't exist or something.  We don't want that, I think.

How do we handle opening a deleted file with FileReader?

::: dom/ipc/Blob.h
@@ +61,5 @@
> +    virtual ~BaseType()
> +    { }
> +
> +    // Each instance of this class will be dispatched to the network socket
> +    // thread pool to run the first time where it will open the file input

*stream* thread pool.  The socket threadpool doesn't work when the browser is offline.

::: ipc/glue/InputStreamUtils.cpp
@@ +58,5 @@
> +      serializable = do_CreateInstance(kMultiplexInputStreamCID);
> +      break;
> +
> +    default:
> +      MOZ_NOT_REACHED("Unknown params!");

We consider should returning nullptr here so that a compromised child process does not cause the parent to abort.

::: ipc/glue/nsIIPCSerializable.h
@@ +9,5 @@
> +#include "mozilla/Attributes.h"
> +
> +#ifndef NS_NO_VTABLE
> +#define NS_NO_VTABLE
> +#endif

Just include nscore.h.

@@ +22,5 @@
> +  {0x1f56a3f8, 0xc413, 0x4274, {0x88, 0xe6, 0x68, 0x50, 0x9d, 0xf8, 0x85, 0x2d}}
> +
> +class NS_NO_VTABLE nsIIPCSerializable : public nsISupports
> +{
> +public: 

Trailing whitespace.

::: netwerk/base/public/nsIIPCSerializable.idl
@@ +16,5 @@
>  [ptr] native Message(IPC::Message);
>  [ptr] native Iterator(void*);
>  
>  [noscript, uuid(1f605ac7-666b-471f-9864-1a21a95f11c4)]
> +interface nsIIPCSerializableObsolete : nsISupports

You need to rename this to nsIIPCSerializableObsolete.idl, if splinter is telling me the truth that you didn't rename this.

::: xpcom/io/nsMultiplexInputStream.cpp
@@ +534,5 @@
> +    }
> +
> +    mCurrentStream = params.currentStream();
> +    mStatus = params.status();
> +    mStartedReadingCurrent =params.startedReadingCurrent();

'= '
Attachment #650201 - Flags: review?(khuey) → review+
Created attachment 651019 [details] [diff] [review]
Patch, v1.1

Updated with the POSIX build fixes and khuey's comments addressed.
Attachment #650201 - Attachment is obsolete: true
Attachment #650452 - Attachment is obsolete: true
Attachment #650201 - Flags: review?(jones.chris.g)
Attachment #651019 - Flags: review?(jones.chris.g)
Comment on attachment 651019 [details] [diff] [review]
Patch, v1.1


>diff --git a/ipc/glue/FileDescriptor.cpp b/ipc/glue/FileDescriptor.cpp

>+FileDescriptor::ShareTo(const FileDescriptor::IPDLPrivate&,
>+                        FileDescriptor::ProcessHandle aOtherProcess) const
>+{

>+  return base::FileDescriptor(newHandle, true);

Document what "true" means here.

>diff --git a/ipc/glue/FileDescriptor.h b/ipc/glue/FileDescriptor.h

>+class FileDescriptor

This needs a descriptive comment.  It's rather nontrivial.

>+#ifdef XP_WIN
>+  typedef HANDLE PlatformHandleType;
>+  typedef HANDLE PickleType;

Document the difference between these two animals.

>+  // This should only ever be created by IPDL.

Why?

>+  PickleType
>+  ShareTo(const IPDLPrivate&, ProcessHandle aOtherProcess) const;

Document this.

>+
>+  bool
>+  IsValid() const;

What's a valid FileDescriptor?

>diff --git a/ipc/glue/IPCSerializableParams.ipdlh b/ipc/glue/IPCSerializableParams.ipdlh

>+namespace mozilla {
>+namespace ipc {
>+

This wants a descriptive overview comment.

We need an ipc/ipdl/test/cxx test for FileDescriptor.

Didn't review the rest of this.

Looks good --- really good, actually --- r=me with comments added and
a test.
Attachment #651019 - Flags: review?(jones.chris.g) → review+
Blocks: 782649
I added a monster comment to the FileDescriptor class plus a few on the methods you mentioned.

As discussed on irc/email, I'll file a followup to add a cxx test for this since the existing mochitests hit this code.
https://hg.mozilla.org/integration/mozilla-inbound/rev/418b5cbc7cd9
Blocks: 783165
https://hg.mozilla.org/mozilla-central/rev/418b5cbc7cd9
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.