1179345 - [e10s] Support listen() and accept() in TcpSocketIpc for ICE-TCP

Status: NEW

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

Description

[Randell Jesup [:jesup] (needinfo me)]

Follow-on from bug 950660 to add support for ::listen() and ::accept() in TCPSocketIpc for ICE-TCP use

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

May be higher than P2/25 since it blocks ICE-TCP

Comment 2

[Nils Ohlmeier [:drno]]

Reminder to self to remove the code from bug 1206981 together with landing this.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

jdm - per discussion with Nils, the issue here is that there's no equivalent to what we do in NrSocket::listen() and NrSocket::accept() under e10s (i.e. when we use NrTcpSocketIpc on top of TCPSocket).

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#904

and 

https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.cpp#1488

How can we move forward on this?  How problematic/how much effort is it to support listen() and accept() in TCPSocket?  Any security issues with access to listen/accept via TCPSocket?  Note that unlike DOM TCPSocket access, this is less exposed to content - but if the content process is compromised, any sort of request can come over the IPC channel.

Thanks!

Comment 4

[Josh Matthews [:jdm]]

As requested on IRC, I'd like more information about where the security features are currently implemented, if there was work done on the previous TCPSocket code that I could look at, etc. This feels like it should be possible to make use of TCPServerSocket, but I don't understand the use case at all.

Comment 5

[Nils Ohlmeier [:drno]]

The use case is to support ICE TCP under e10s. We have ICE TCP support for non-e10s already.

In case of ICE TCP we need to create a listening TCP socket and tell the other side of the WebRTC connection about the TCP port we are listening on. This allows the other side to open a TCP towards our Firefox instance. As we don't know the other sides address for sure (in case of NATs replacing private with public IP addresses) we need to accept incoming connections from anywhere.

Now in case of ICE TCP we are handing any incoming data to our ICE parser. And the other side needs to present the expected ICE username and password in the request/data it sends us. Otherwise the ICE stack will not accept the connection on the application layer and therefore not forward any of the data further up the stack. But to be clear at this point the TCP connection would be established already and the other side could send us data, which we would drop (as the ICE would fail to parse it).

Unfortunately I'm not aware of previous security feature work. I remember that we had a discussion about implementing a packet filter of some kind, but I can't find the ticket right now.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

A packet filter (which runs in the parent) exists for UDP currently.  For TCP, this would be the code that checks the ICE username/password.  

However: the UDP filter largely exists to stop you from starting a UDP "connection" to an arbitrary destination, and then smash it with traffic (i.e. DOS).  That filter verifies that the destination is running ICE (basically).  For this case, we're taking an incoming TCP connection, so it really can't be used for a DOS attack in the same way; you have to provoke the other system into opening a specific address/port to you before you can do anything.  The closest I can come to a DOS attack via this is something which provides a URL/etc to a site (image, etc) and then when they connect blasts them with data.  A simple check for ICE username/password on connect would block even that.  It's not clear this filter actually adds security here, but likely it's not hard.

Comment 7

[Nils Ohlmeier [:drno]]

I'm working on a packet filter like describe in comment #6 in bug 1244926 (which is not really needed for this, because we allow outgoing TCP connections under e10s already with our TURN TCP support in e10s).

It is going to ensure that Firefox sends initially a STUN request to the destination, and only after receiving a STUN reply with the same STUN request ID the connection will be white listed to transfer any packets. The idea here is to make the live harder for an attacker who has taken over the content process, but does not control the parent process (yet).
For this ticket the incoming traffic is probably more interesting. The filter will record STUN ID's from incoming packets and require the content process to respond to the STUN packets with the same STUN request ID, before with listing the connection and allow any content to get exchanged.

:jdm are the comments 5, 6 and this one here enough explanation for our use case, or do you have any other questions?

Comment 8

[Josh Matthews [:jdm]]

All of the actual details about the packet filter make my eyes glaze over. I'm most interested in the way that your filter interacts with the existing socket infrastructure and how we can tie that into TCPServerSocket's implementation.

Comment 9

[Josh Matthews [:jdm]]

For example, reading NrSocket::accept and NrSocket::listen, I don't see any kind of filtering happening.

Comment 10

[Nils Ohlmeier [:drno]]

I'm not sure if the whole filtering discussion is a little bit of a red herring.

Because the idea in case of these filters is "only" that if someone has gained control over the content process in the child, that the parent will make it harder to open UDP and TCP sockets (once bug 1244926 is landed) and connect them where ever he wants. So the filters enforce that once UDP or TCP socket gets opened by the content process it can only send STUN request or answers out initially, until the filter has verified that the other side understands STUN as well. So the content process can hopefully no longer connect to any listening service.

In case of non-e10s there is no such filtering, because if the content process is taken over it has gained full access to the networking code as well already.

And for non-e10s we already have the code landed, although still behind a pref, which accepts TCP connections on listening sockets.

Now for accept() and listen() support in e10s we can re-use the filters from bug 1244926, to ensure that an initial STUN request - response "handshake" has to happen on a new accepted TCP connection, before allowing other traffic on that connection.

A couple of notes (to make sure we are all on the same page here):
- to be able to identify the STUN request and response we hand the data to our STUN parser in the parent, so this does not add any defense against issues with our STUN parser getting abused once we start listening on a TCP socket.
- to avoid having to deal with buffering inside the filter itself the current implementation of the filter lets essentially all traffic from the far side pass to the child (e.g. in case we the STUN message in multiple IP packets) so it really only prevents sending non STUN message from within Firefox to the outside (although we could look into making this more strict)

Josh: is your thinking that you only want to allow accept() / listen() for e10s if the child specifies a filter?

Comment 11

[Josh Matthews [:jdm]]

No, that wasn't my thinking. I do appreciate the context provided in the previous comment. I'm still confused by the push to add more listening abilities to TCPSocket directly rather than use the existing (e10s-supporting) TCPServerSocket, but I feel like that's a conversation that's not going anywhere.

Comment 12

[Josh Matthews [:jdm]]

I mean, if we're going to add accept and listen to TCPSocket, then we might as well lock it down and only allow it with a filter I guess, since no other code is going to be using it.

Comment 13

[Nils Ohlmeier [:drno]]

(In reply to Josh Matthews [:jdm] from comment #11)
> I'm still confused by the push to add more listening
> abilities to TCPSocket directly rather than use the existing
> (e10s-supporting) TCPServerSocket, but I feel like that's a conversation
> that's not going anywhere.

I'll double check if we can use TCPServerSocket for our needs.

Comment 14

[Nils Ohlmeier [:drno]]

So the problem is the following:

Our base class looks like this: https://dxr.mozilla.org/mozilla-central/source/media/mtransport/nr_socket_prsock.h#152
which pretty much mimics a basic Unix network socket (except for the create() method) with connect() and listen() calls on it.

Now if I understand it correct in case of e10s this usual network socket logic got split into the two classes TCPSocket and TCPServerSocket. But who ever uses TCPServerSocket needs to implement TCPSocket as well, because when ever TCPServerSocket accepts a new incoming connection the accept() call returns a new TCPSocket. Is that an accurate description of how listening on TCP sockets is suppose to work under e10s, Josh?

If that is the case then we can probably make it working without additional listen() and accept() support for TCPSocket.
But the problem for us becomes then that in NrSocket we need to create and handle TCPSocket's and/or TCPServerSocket's depending on which functions get called on the NrSocket. So we need to abstract the two classes/types back into one, which can handle both use cases.
I guess the confusion came from our side not understanding why there are two partial implementations of a socket API. And I'm still confused what is the thinking behind the super limited functionality of the TCPServerSocket class.

Comment 15

[Josh Matthews [:jdm]]

Yes, TCPServerSocket and TCPSocket are always used together. I can't speak as to the motivation for splitting the functionality in this way; there's a web specification that includes the same design (https://www.w3.org/2012/sysapps/tcp-udp-sockets/), but I don't know which came first. Maybe it's more webby this way?

Comment 16

[Josh Matthews [:jdm]]

Oh, and this wasn't an e10s vs. non-e10s thing. This was a Web API thing, independent of what process the networking operations end up running in.

Comment 17

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 18

[Nils Ohlmeier [:drno]]

I don't have the capacity any more to work on this any time soon.