781256 - Share FileDescriptors across processes in preparation for OS-level sandbox

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: 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 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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!

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This work lets us deny all fs access to content processes, which blocks other blockers.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Fixes for POSIX

Passes all the IPC tests.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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();

'= '

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1.1

Updated with the POSIX build fixes and khuey's comments addressed.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/418b5cbc7cd9

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/418b5cbc7cd9