Closed Bug 1322506 Opened 3 years ago Closed 3 years ago

Move WebRTC-related network access (STUN/ICE/etc.) from content process to parent

Categories

(Core :: WebRTC: Networking, defect, P2)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1345511
Tracking Status
firefox53 --- affected
Blocking Flags:

People

(Reporter: jld, Unassigned)

References

Details

(Whiteboard: [sb+, webrtc])

I was investigating what we'd need to do to prevent content processes from directly accessing the system's network stack, by doing a Linux Try run where bind()/listen()/accept() are disallowed.  (connect() and socket() are also a problem, but we already know we can't make progress there until audio device access is remoted.)

This broke a number of WebRTC tests (and at least one for the Presentation API, it looks like), with the function nr_stun_find_local_addresses trying to call bind(), and — unlike what happened with bug 969715 — actually needing that bind() call to succeed for the tests to pass.  (It looks like the call chain from there goes -> nr_stun_get_addrs -> stun_getifaddrs -> getifaddrs in glibc -> bind()ing in the PF_NETLINK domain.)

So it will be necessary to e10s-enable this part of the WebRTC stack enough to move that code into the parent process and invoke it via IPC (and return only the information that's required to be exposed by these Web APIs).  I think we had a way to do this on B2G, because we could deny socket() without breaking anything (see, again, bug 969715).
The code you are referring to with nr_stun_get_addrs() is basically OS specific code which iterates over all network interfaces to collect all local IP addresses on the system. This information is needed to kick of the ICE connectivity checks for WebRTC.

While I think that for example the Windows version of this code does not use any sockets to gather this information it would make sense to try move the whole interface/IP address gathering over to the parent process.
The Android implementationS of getifaddrs (required because it is missing in bionic - and we have 2 separate ones in the tree) seems to get along fine without bind (still using socket(), of course). Which might explain why B2G worked.

Recent glibc does indeed bind the netlink socket, though.
Jed, how urgent is this from your point of view?

I'm asking because another alternative might be to move the whole ICE stack over into the parent process. Which would be more effort, but would also help solving some other issues.
backlog: --- → webrtc/webaudio+
Rank: 25
Priority: -- → P2
This isn't particularly high on our priority list, but it's noteworthy that this is the only thing holding us back from tightening the sandbox in a manner that would have neutered last week's 0-day against Firefox/Tor.
Whiteboard: sb? → sb+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #2)
> The Android implementationS of getifaddrs (required because it is missing in
> bionic - and we have 2 separate ones in the tree) seems to get along fine
> without bind (still using socket(), of course). Which might explain why B2G
> worked.

On B2G we made socket() always fail with EACCESS without breaking anything, and the getifaddrs()es I found were still creating and using a PF_NETLINK socket, which is why I think there was some kind of e10s going on on B2G.
Sorry for the lack of clarity in comment #0.  Stepping back a bit, the high-level goals here are:

* Prevent direct network access
* Restrict access to local services that use network-like IPC (how much of an issue this is varies by OS)
* Limit exposure of PII, such as MAC addresses (and, ideally, also network-layer addresses if the WebRTC features that expose them to content are preffed off) 
* General attack surface reduction (kernel vunerabilities, or just edge cases that allow things we thought we'd denied)

The tools we have here vary by OS and aren't always very granular.  On Mac OS X we can control networking separately from interface/address enumeration, but restricting by address type is probably not possible.  (For reference: OS X getifaddrs uses sysctl, which is why it works despite the sandbox policy not allowing anything to do with networking or sockets — it allows read access to the entire sysctl tree.  As does Chrome, incidentally.  So the question of leaking PII might be a lost cause here.)

On Linux, the right tool for the job is creating a new network namespace — basically a private instance of the network stack, with nothing but a private loopback interface — which will certainly break getifaddrs.  We don't have access to that on all systems, but we're also using seccomp-bpf to filter system calls (as well as for attack surface reduction); however, that's also not very granular in practice.  Which means we can't really get where we're going without breaking this part of WebRTC.  (The other blocker we know about here is audio, thanks to PulseAudio, but there's work in progress on remoting that.)

Windows networking… is a topic I know very little about.  But, longer-term, we'd like to restrict it as much as possible for the high-level reasons above.


That said, as comment #4 points out, this isn't an immediate priority, but I wanted to try to get an idea of where the blockers would be and get the information into bugs.
One option we're considering would be to move the mtransport network code out of Content, but into a separate sandbox (as an alternative to either moving it to the Master process or to adding IPC interfaces for the getifaddrs calls).  

One question is can we make a sandbox specifically for network or network + media capture + audio output, and does that gain us security versus having them in Master, or having them in Content.  The answer to that may vary by OS of course.  Right now video input is in Master, audio in/out (cubeb) is in Content but will be moved to Master (current plan).
Flags: needinfo?(jld)
Flags: needinfo?(gpascutto)
Whiteboard: sb+ → [sb+, webrtc]
(In reply to Randell Jesup [:jesup] from comment #7)
> One option we're considering would be to move the mtransport network code
> out of Content, but into a separate sandbox (as an alternative to either
> moving it to the Master process or to adding IPC interfaces for the
> getifaddrs calls).  
> 
> One question is can we make a sandbox specifically for network or network +
> media capture + audio output, and does that gain us security versus having
> them in Master, or having them in Content.  The answer to that may vary by
> OS of course.  Right now video input is in Master, audio in/out (cubeb) is
> in Content but will be moved to Master (current plan).

Hey jesup, we'd like to schedule a meeting with you to chat about this. I'll reach out on irc to set something up in  the new year.
Flags: needinfo?(jld)
Flags: needinfo?(gpascutto)
From the pure networking side I currently see three options on the table:

- add an IPC interface to Necko in the parent process which delivers all the network interfaces and all local IP addresses, but leave everything else as is
- move (almost) everything from mtransport into the parent process
- create a new networking process for Necko and mtransport
  - that networking process could also do media handling, or media could move even into yet another process

@jimm would appreciate if you would include me in that meeting.
I think this bug got resolved by landing bug 1345511. Feel free to re-open if that is not the case, or if there is still something left to be done which was not resolved as part of bug 1345511.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1345511
You need to log in before you can comment on or make changes to this bug.