Opened 17 months ago

Closed 9 months ago

Last modified 9 months ago

#22794 closed defect (fixed)

Don't open AF_INET/AF_INET6 sockets when AF_LOCAL is configured.

Reported by: yawning Owned by: pospeselr
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-security, tbb-sandboxing, TorBrowserTeam201802R
Cc: intrigeri, arthuredelstein, mcs Actual Points:
Parent ID: #20775 Points:
Reviewer: Sponsor: Sponsor4

Description

Discovered when trying to resolve #20775.

Unsandboxed Tor Browser 7.0.1:

socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 67
fcntl(67, F_GETFL)                      = 0x2 (flags O_RDWR)
fcntl(67, F_SETFL, O_RDWR|O_NONBLOCK)   = 0
socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 68
close(68)                               = 0
socket(AF_INET6, SOCK_STREAM, IPPROTO_IP) = 68
fcntl(68, F_GETFL)                      = 0x2 (flags O_RDWR)
fcntl(68, F_SETFL, O_RDWR|O_NONBLOCK)   = 0
close(68)                               = 0
setsockopt(67, SOL_TCP, TCP_NODELAY, [1], 4) = 0

socket(AF_UNIX, SOCK_STREAM, 0)         = 68
fcntl(68, F_GETFL)                      = 0x2 (flags O_RDWR)
fcntl(68, F_SETFL, O_RDWR|O_NONBLOCK)   = 0
close(67)                               = 0
connect(68, {sa_family=AF_UNIX, sun_path="/var/run/tor/socks"}, 106) = 0

If the first socket (AF_INET) call fails (as it will due to seccomp-bpf) the AF_LOCAL socket never gets created, and pages don't load. The failure mode doesn't appear to depend on errno (at least, it didn't make a difference if it was ENOSYS or EAFNOSUPPORT).

Using IPC should mean, "Tor Browser uses IPC, and only IPC", and not "Tor Browser refuses to work if non-IPC socket creation fails", because the whole point of using IPC in the first place is so that Tor Browser can be ran in a way that disallows non-IPC connections.

Child Tickets

Attachments (2)

seccomp-browser.c (2.7 KB) - added by yawning 10 months ago.
seccomp based test case.
0001-Bug-22794-Don-t-open-AF_INET-AF_INET6-sockets-when-A.patch (2.8 KB) - added by pospeselr 10 months ago.
updated patch to also be enabled on macOS rather than just Linux

Download all attachments as: .zip

Change History (30)

comment:1 Changed 17 months ago by yawning

A minimal/self contained LD_PRELOAD that can reproduce the behavior:

#include <sys/types.h>
#include <sys/socket.h>
#include <errno.h>
#include <stdio.h>

#define _GNU_SOURCE
#include <unistd.h>
#include <syscall.h>

int socket(int domain, int type, int protocol) {
  fprintf(stderr, "stub: socket(%d, 0x%08x, %d)\n", domain, type, protocol);
  if (domain != AF_LOCAL) {
    fprintf(stderr, "stub: domain is not AF_LOCAL, rejecting\n");
    errno = EAFNOSUPPORT;
    return -1;
  }
  return syscall(SYS_socket, domain, type, protocol);
}

And commenting out the rejection (as in always calling syscall(), regardless of the domain), magically makes things start to work.

comment:2 Changed 17 months ago by yawning

Parent ID: #20775

Setting a parent so it's obvious why the sandbox doesn't use Tor Browser's IPC support.

comment:3 Changed 17 months ago by yawning

Component: Applications/Tor Browser SandboxApplications/Tor Browser
Owner: changed from yawning to tbb-team

It also helps if I file this against the correct component.

comment:4 Changed 17 months ago by intrigeri

Cc: intrigeri added

comment:5 Changed 16 months ago by gk

Keywords: TorBrowserTeam201707 added

Hm, I wonder what is going on here. Putting it on our radar.

comment:6 Changed 16 months ago by arthuredelstein

Cc: arthuredelstein added

comment:7 Changed 16 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:8 Changed 15 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:9 Changed 14 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:10 Changed 12 months ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:11 Changed 12 months ago by gk

Moving tickets to December 2017

comment:12 Changed 12 months ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

comment:13 Changed 10 months ago by gk

Keywords: TorBrowserTeam201801 added; TorBrowserTeam201712 removed

Moving tickets to 2018.

comment:14 Changed 10 months ago by pospeselr

Owner: changed from tbb-team to pospeselr
Status: newassigned

comment:15 Changed 10 months ago by mcs

Cc: mcs added

comment:16 Changed 10 months ago by yawning

I looked at this a bit more writing an e-mail explaining the issue to pospeselr, and I strongly suspect that at least some of the behavior (the strace output) can be explained by this:

https://github.com/mozilla/gecko-dev/blob/1d9f00cb8d2088fccb7a611a34be655b9fa9fec1/netwerk/socket/nsSOCKSIOLayer.cpp#L601

Changed 10 months ago by yawning

Attachment: seccomp-browser.c added

seccomp based test case.

comment:17 Changed 10 months ago by yawning

Apparently someone really wanted to be able to see this with seccomp, so I attached a trivial program that uses libseccomp to initialize a filter that will reject socket(AF_INET[6], ..., ...) by SIGSYSing the thread.

Usage:

  1. Compile it.
  2. Set the required env vars to get Tor Browser to talk to a external tor process over AF_UNIX sockets.
  3. Run it from the root of a tor browser installation (It calls system("Browser/start-tor-browser"); because it's a trivial test case and I'm lazy).
  4. Watch in horror as nothing works.
  5. Run it with strace to see the socket thread traces ending abruptly with +++ killed by SIGSYS +++.

The control port appears to work, but any attempt to use the socks port get stomped on, so I'm basically convinced that the root cause is the non-AF_INET socket support in nsSOCKSIOLayer being a steaming pile of shit.

comment:18 Changed 10 months ago by yawning

Really though, just look at nsSOCKSIOLayer.

  1. All outgoing SOCKS connections made by firefox start out their lives as AF_INET sockets. (Assumption, firefox code gives me a headache, but it matches the trace output).
  2. When firefox actually goes to connect to the proxy (ConnectToProxy), the FixupAddressFamily routine is called.
  3. FixupAddressFamily checks to see if the proxy actually is reachable via an AF_INET socket, and if not, opens a new file descriptor with the correct domain.

What appears to have happened judging for a cursory inspection of the file history was:

  1. Back in the day, this was only expected to handle AF_INET, because "this IPng thing will never happen".
  2. When AF_INET6 support was required, it was kludged on this way.
  3. When AF_UNIX (and Windows pipes or whatever that's also in the code) support was required, the kludge was enhanced.

Which is great if the only reason you want something like AF_UNIX is to use AF_UNIX socket for the hell of it, and not so great if you want use something like seccomp to prohibit AF_INET.

comment:19 Changed 10 months ago by pospeselr

Keywords: TorBrowserTeam201802R added; TorBrowserTeam201801 removed
Status: assignedneeds_review

comment:20 Changed 10 months ago by yawning

Oh hey, I was right.

Regarding the patch:

  • I'm not sure if checking that the URI scheme is file is the correct long term fix (eg: #20337), but that bridge can be burnt when someone gets there.
  • Should this apply to OSX as well? I do not know how OSX's process sandboxing stuff works, or what options it has for limiting what a process can do.

comment:21 Changed 10 months ago by teor

macOS legacy sandbox support has:

     #include <sandbox.h>
...
     kSBXProfileNoInternet              TCP/IP networking is prohibited.
                                        DEPRECATED.

     kSBXProfileNoNetwork               All sockets-based networking is pro-
                                        hibited.  DEPRECATED.

sandbox-exec is also legacy, but it works from the command line:
https://paolozaino.wordpress.com/2015/10/20/maximum-security-and-privacy-using-mac-os-sandbox-and-tor-browser-bundle/

Unfortunately, the replacement API doesn't seem to distinguish between TCP/IP and unix sockets. We'd need to do some testing.

com.apple.security.network.client

Network socket for connecting to other machines

com.apple.security.network.server

Network socket for listening for incoming connections initiated by other machines

https://developer.apple.com/library/content/documentation/Miscellaneous/Reference/EntitlementKeyReference/Chapters/EnablingAppSandbox.html#//apple_ref/doc/uid/TP40011195-CH4-SW9

comment:22 Changed 10 months ago by yawning

For what it's worth, I'm of the opinion that the AF_INET6 socket being created is probably fine, as long as the browser isn't rendered non-functional if AF_INET6 isn't supported.

This can be revisited if/when people want to write seccomp rules that will kill (rather than gently reject) proscribed calls, but I don't think that's realistic at the moment.

comment:23 in reply to:  20 Changed 10 months ago by mcs

Replying to yawning:

Oh hey, I was right.

Regarding the patch:

  • I'm not sure if checking that the URI scheme is file is the correct long term fix (eg: #20337), but that bridge can be burnt when someone gets there.

Agreed. Richard, if we keep the "file://" prefix test, please add a comment to #20337 so we don't forget about this loose end.

  • Should this apply to OSX as well? I do not know how OSX's process sandboxing stuff works, or what options it has for limiting what a process can do.

I think we might as well apply the patch to OSX as well since it may be useful there. The not-really-awesome OSX sandboxing prototype that Kathy and I created a while ago does use the older mechanism of the ones that teor discussed (sandbox-exec).

Changed 10 months ago by pospeselr

updated patch to also be enabled on macOS rather than just Linux

comment:24 Changed 10 months ago by pospeselr

Yeah so long as the seccomp policies for AF_INET6 socket creation doesn't kill the calling thread, the existing Firefox code gracefully handles socket creation failure.

In the event that we want to enable such a strict policy for the sandbox the relevant code in nsSOCKSIOLayer.cpp's nsSOCKSIOLayerAddToSocket() can probably be safely refactored to ignore the IPv6 detection if the desired socket family is AF_LOCAL.

comment:25 Changed 9 months ago by mcs

r=mcs
The latest patch looks good to me, and it compiles and seems to work on macOS.

comment:26 Changed 9 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good to me and applied to tor-browser-52.6.0esr-8.0-2 (commit eafaa94a613a326bd13234540fe88b86451ee3e9).

Leaving the remaining things to Richard:

1) Comment on #20337
2) Open a bug in Mozilla's bug tracker and trying to get this patch upstreamed for ESR60.

comment:27 Changed 9 months ago by gk

Sponsor: Sponsor4

comment:28 Changed 9 months ago by pospeselr

Bug for uplift has been created here: https://bugzilla.mozilla.org/show_bug.cgi?id=1441327

Note: See TracTickets for help on using tickets.