nsIServerSocket should support Unix-domain sockets

RESOLVED FIXED in mozilla26

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: jimb, Assigned: jimb)

Tracking

Trunk
mozilla26
All
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

12.81 KB, patch
Details | Diff | Splinter Review
4.22 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
39.37 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 773570 [details] [diff] [review]
Make nsServerSocket::InitWithAddress provide more detailed error results.
(Assignee)

Comment 2

5 years ago
Created attachment 773571 [details] [diff] [review]
Add support for Unix-domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.
(Assignee)

Comment 3

5 years ago
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.
Duplicate of this bug: 834002
(Assignee)

Updated

5 years ago
Depends on: 899757
(Assignee)

Updated

5 years ago
Attachment #773570 - Attachment is obsolete: true

Comment 5

5 years ago
What's up with this bug?  Coming soon?  This is pretty important.
(Assignee)

Comment 6

5 years ago
Created attachment 789444 [details] [diff] [review]
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.
Assignee: nobody → jimb
Attachment #773571 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Attachment #789444 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 7

5 years ago
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.
Attachment #789444 - Flags: review?(jduell.mcbugs) → feedback?(jduell.mcbugs)
(Assignee)

Comment 8

5 years ago
Created attachment 791174 [details] [diff] [review]
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...)
Attachment #789444 - Attachment is obsolete: true
Attachment #789444 - Flags: feedback?(jduell.mcbugs)
Attachment #791174 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 9

5 years ago
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!).
(Assignee)

Comment 10

5 years ago
Created attachment 795477 [details] [diff] [review]
Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.

Updated for current sources; tests expanded.
Attachment #791174 - Attachment is obsolete: true
Attachment #791174 - Flags: review?(jduell.mcbugs)
Attachment #795477 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 11

5 years ago
Created attachment 796416 [details] [diff] [review]
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.
Attachment #795477 - Attachment is obsolete: true
Attachment #795477 - Flags: review?(jduell.mcbugs)
Attachment #796416 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #796416 - Attachment description: unix-domain.patch → Add support for Unix domain sockets to nsIServerSocket.idl and @mozilla.org/network/server-socket;1.
(Assignee)

Comment 12

5 years ago
Created attachment 796525 [details] [diff] [review]
incremental diff from second-to-last to last version of unix-domain.patch

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

Updated

5 years ago
Attachment #796416 - Flags: review?(jduell.mcbugs) → review?(honzab.moz)
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.
(Assignee)

Comment 14

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

Whitespace changes and spelling fixes.
Attachment #797109 - Flags: review?(honzab.moz)
(Assignee)

Comment 15

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

Unix-domain sockets, with whitespace changes removed.
Attachment #796416 - Attachment is obsolete: true
Attachment #796416 - Flags: review?(honzab.moz)
Attachment #797193 - Flags: review?(honzab.moz)
Thanks Jim, just reviewing.  For info, please use context of 8 lines for the patches next time.
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)
Attachment #797193 - Flags: review?(honzab.moz) → review-
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...
Attachment #797109 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 20

5 years ago
Created attachment 798207 [details] [diff] [review]
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.
Attachment #797193 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #798207 - Flags: review?(honzab.moz)
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().
Attachment #798207 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Comment 23

5 years ago
(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.
(Assignee)

Comment 25

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

Updated

5 years ago
Blocks: 912913
(Assignee)

Comment 26

5 years ago
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
(Assignee)

Comment 27

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/028b937786cb
https://hg.mozilla.org/mozilla-central/rev/c0a79b1e60b1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

3 years ago
Blocks: 1211567
You need to log in before you can comment on or make changes to this bug.