892114 - nsIServerSocket should support Unix-domain sockets

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jim Blandy :jimb]

For security reasons, we don't want to have the debugging protocol server listening on a TCP port on the phone. Restricting the listener to accept connections only from the local host isn't good enough, as a compromised child process could then use the debugger server to get control of the main process.

Unix-domain sockets can be protected using filesystem permissions; child processes run as a different (less-privileged) user, and could thus be prevented from connecting to the debug server.

ADB can forward connections to unix-domain sockets, so we can forward debugging connections from the workstation that way.

Comment 1

[Jim Blandy :jimb]

Attachment: Make nsServerSocket::InitWithAddress provide more detailed error results.

Comment 2

[Jim Blandy :jimb]

Attachment: Add support for Unix-domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Comment 3

[Jim Blandy :jimb]

Those are work-in-progress patches. For a full implementation, there's more to be done: for example, it should be possible to retrieve the address to which an opened socket is bound, but the IDL hasn't been extended to do that now.

Comment 5

[Dave Camp (:dcamp)]

What's up with this bug?  Coming soon?  This is pretty important.

Comment 6

[Jim Blandy :jimb]

Attachment: Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

With the patches from bug 899757 applied, the xpcshell test script test_unix_domain.js has been observed to:
- create a server Unix-domain socket,
- create a client socket connecting to that server socket,
- exchange data between client and server, and
- check the client socket address.

So I guess this is "first light" for this patch.

Comment 7

[Jim Blandy :jimb]

Comment on attachment 789444 [details] [diff] [review]
Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Changing "review" request to "feedback"; I'd like to flesh out the tests a bit before opening this up for review; the patch itself has some "to-do" notes as well that I want to take care of.

Comment 8

[Jim Blandy :jimb]

Attachment: unix-domain.patch

Okay, this one is ready for review.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=04966b9dbf16
(I'm expecting some stray Windows build failures there, and perhaps some OSX issues, but hopefully Linux will be clean...)

Comment 9

[Jim Blandy :jimb]

Ah, note that WTC landed the NSPR error-reporting improvements in bug 907512, so some of the error handling tests need to be updated (for the better!).

Comment 10

[Jim Blandy :jimb]

Attachment: Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Updated for current sources; tests expanded.

Comment 11

[Jim Blandy :jimb]

Attachment: Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Here's a revision that ensures that nsSocketTransport and nsSocketTransportService properly recognize Unix domain sockets as local to the machine, so they shouldn't be affected by whether nsIIOService is offline or not.

Alex, give this one a try.

Comment 12

[Jim Blandy :jimb]

Attachment: incremental diff from second-to-last to last version of unix-domain.patch

Here's an interdiff, showing only what I changed last night.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 796416 [details] [diff] [review]
Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

At the first look, please remove the white-space only changes from the patch please.  It makes it hard to review and is no necessary.  If you want to remove them, then have a separate patch for it.

Thanks.

Comment 14

[Jim Blandy :jimb]

Attachment: Whitespace and spelling fixes encountered while working on Unix domain socket support.

Whitespace changes and spelling fixes.

Comment 15

[Jim Blandy :jimb]

Attachment: Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Unix-domain sockets, with whitespace changes removed.

Comment 16

[Honza Bambas (:mayhemer)]

Thanks Jim, just reviewing.  For info, please use context of 8 lines for the patches next time.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 797193 [details] [diff] [review]
Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

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

Thanks.  This looks really good.  But I'll rather check on this patch again, quickly this time.

I didn't try to run the test, I'll attempt to find time for it, but not block the review on it.

::: netwerk/base/public/nsIServerSocket.idl
@@ +145,5 @@
> +     * the socket itself is not granted since 2003 (Issue 6). NetBSD says
> +     * that the permissions on the containing directory (execute) have
> +     * always applied, so creating sockets in appropriately protected
> +     * directories should be secure on both old and new systems.
> +     */

Thanks for this comment!

::: netwerk/base/public/nsISocketTransportService.idl
@@ +81,5 @@
> +     * @param aPath
> +     *        The file name of the Unix domain socket to which we should
> +     *        connect.
> +     */
> +    nsISocketTransport createUnixDomainTransport(in nsIFile aPath);

Change IID of the nsISocketTransportService interface!

::: netwerk/base/src/nsServerSocket.cpp
@@ +285,5 @@
> +      return rv;
> +
> +    // Create a Unix domain PRNetAddr referring to the given path.
> +    PRNetAddr addr;
> +    if (path.Length() + 1 > sizeof(addr.local.path))

So on path.Length() == 0xffffffff we pass this check and copy all data somewhere in memory.  Please fix this.

*)

@@ +288,5 @@
> +    PRNetAddr addr;
> +    if (path.Length() + 1 > sizeof(addr.local.path))
> +        return NS_ERROR_FILE_NAME_TOO_LONG;
> +    addr.local.family = PR_AF_LOCAL;
> +    memcpy(addr.local.path, path.get(), path.Length());

I'd a little bit more see using BeginReading() to refer the buffer and EndReading() - BeginReading() as a length, but this should work as well.

::: netwerk/base/src/nsSocketTransport2.cpp
@@ +865,5 @@
>  
>  nsresult
> +nsSocketTransport::InitWithFilename(const char *filename)
> +{
> +#if defined(XP_UNIX) || defined(XP_OS2)

Does this work on android too? (I presume yes)

@@ +868,5 @@
> +{
> +#if defined(XP_UNIX) || defined(XP_OS2)
> +  size_t filenameLength = strlen(filename);
> +
> +  if (filenameLength + 1 > sizeof(mNetAddr.local.path))

*)

@@ +978,5 @@
>      if (!mProxyHost.IsEmpty()) {
>          if (!mProxyTransparent || mProxyTransparentResolvesHost) {
> +#if defined(XP_UNIX) || defined(XP_OS2)
> +        NS_ABORT_IF_FALSE(!mNetAddrIsSet || mNetAddr.raw.family != AF_LOCAL,
> +                          "Unix domain sockets can't be used with proxies");

Indent as it were in if { branch.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +591,5 @@
> +      return NS_ERROR_OUT_OF_MEMORY;
> +
> +  rv = trans->InitWithFilename(path.get());
> +  if (NS_FAILED(rv))
> +      return rv;

Correct indention as per file style (4 spaces)

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 797109 [details] [diff] [review]
Whitespace and spelling fixes encountered while working on Unix domain socket support.

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

r+ but I don't like whitespace only change patches.  We should do a global clean up rather as a separate project or just leave it...

Comment 19

[Jim Blandy :jimb]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=40069d5d894d

Comment 20

[Jim Blandy :jimb]

Attachment: Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Updated per comments.

I changed the UUIDs for both the interfaces to which I add methods.

>::: netwerk/base/src/nsServerSocket.cpp
>> +    if (path.Length() + 1 > sizeof(addr.local.path))
>
>So on path.Length() == 0xffffffff we pass this check and copy all data >somewhere in memory.  Please fix this.

True --- but path comes from an nsIFile, which can't be that large.

I did a subtraction on the sizeof side instead, in both places.

> Does this work on android too? (I presume yes)

It would seem to, from looking at configure.ac.

Sorry about the indentation mistakes. There's some inconsistency in that directory.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 798207 [details] [diff] [review]
Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

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

r=honzab

No time to run the test, I trust you to do it.

::: netwerk/base/src/nsServerSocket.cpp
@@ +288,5 @@
> +  PRNetAddr addr;
> +  if (path.Length() > sizeof(addr.local.path) - 1)
> +    return NS_ERROR_FILE_NAME_TOO_LONG;
> +  addr.local.family = PR_AF_LOCAL;
> +  memcpy(addr.local.path, path.BeginReading(), path.Length());

Here you should chose of one of the following approaches, but not mix them:

* .BeginReading() to access the buffer, .EndReading() - .BeginReading() to get actual buffer length

* .get() to access the buffer, .Length() to get the length

It's not for my self entirely clear what is the right way here, so it's up to you what you chose to access the buffer and get the length.

To make this 100% clear you may rather ask Boris Zbarsky.

::: netwerk/base/src/nsSocketTransportService2.cpp
@@ +589,5 @@
> +    nsRefPtr<nsSocketTransport> trans = new nsSocketTransport();
> +    if (!trans)
> +        return NS_ERROR_OUT_OF_MEMORY;
> +
> +    rv = trans->InitWithFilename(path.BeginReading());

Oh, I might not be enough specific.

BeginReading() and EndReading() should be used together.  When only the buffer is accessed, then .get() should be used.  So, here you were OK with .get().

Comment 22

[Jim Blandy :jimb]

Two hold-ups here:

1) My tests assumed that the notifications that occur when we close down one side of a connection would  happen in a particular order, which isn't the case. It's incidental to the test's purpose, so I simplified it, and it should be fine now.

2) We're not able to create the socket at all on Android:

TEST-INFO | test_unix_domain.js | "creating socket: /mnt/sdcard/tests/xpcshell/tmp/socket"
TEST-UNEXPECTED-FAIL | /mnt/sdcard/tests/xpcshell/head.js | NS_ERROR_CONNECTION_REFUSED: Component returned failure code: 0x804b000d (NS_ERROR_CONNECTION_REFUSED) [nsIServerSocket.initWithFilename] - See following stack:

"NS_ERROR_CONNECTION_REFUSED" is socket-speak for "permission denied" (EACCES). The test is calling do_get_tempdir() to get the name of the directory in which to create its sockets; I don't understand why we wouldn't be able to create a socket there.

Comment 23

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #22)
> 2) We're not able to create the socket at all on Android:

This may be a limitation of the filesystem holding the temp directory on Android; not all filesystems can hold Unix domain sockets. I've disabled the test on Android; if the harness provides a way to give us a suitable path, then we can re-enable it.

Comment 24

[Jim Blandy :jimb]

https://tbpl.mozilla.org/?tree=Try&rev=81000450993f

Comment 25

[Jim Blandy :jimb]

Try run looks good. Waiting on review of prerequisite patches in bug 899757.

Comment 26

[Jim Blandy :jimb]

Grr. I screwed up the xpcshell.ini syntax for disabling test_unix_domain.js on Android, and disabled the whole directory of tests. New try push:

https://tbpl.mozilla.org/?tree=Try&rev=52e0448a4bda

Comment 27

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #26)
> Grr. I screwed up the xpcshell.ini syntax for disabling test_unix_domain.js
> on Android, and disabled the whole directory of tests. New try push:

Okay. They all ran this time.

Comment 28

[Jim Blandy :jimb]

https://hg.mozilla.org/integration/mozilla-inbound/rev/028b937786cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0a79b1e60b1

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/028b937786cb
https://hg.mozilla.org/mozilla-central/rev/c0a79b1e60b1