search

haskell-distributed / network-transport-tcp

TCP Realisation of Network.Transport
http://haskell-distributed.github.com
BSD 3-Clause "New" or "Revised" License
30 stars 27 forks source link

Check peer-reported host against socket host #54

Closed avieth closed 7 years ago

avieth commented 7 years ago

This is to prevent a peer from denying service to another peer by lying about their end point address. The attacker would give the address of the victim and then simply hold that heavyweight connection open, so that requests by the victim would be rejected as crossed connections.

avieth commented 7 years ago

Bump. I have a patch to implement #59 on top of this branch.

facundominguez commented 7 years ago

As it stands, it seems a bit useless to have the peer send the external address if the transport can find it out.

It is like the following dialog: -- Alexander, what is your name? -- My name is Duncan. -- You are lying to me, I know you are Alexander! I'll shut you down. -- Heck! Why did you ask to begin with?

avieth commented 7 years ago

As it stands, it seems a bit useless to have the peer send the external address if the transport can find it out.

Fair point, but the peer does need to send the port on which it listens, since it's not the same as the port of this socket. So we could have the peer deconstruct its own address and send only the port, but it was just easier to implement it as it is now: send the whole EndPointAddress.

avieth commented 7 years ago

Patch for testReconnect included in #60 https://github.com/haskell-distributed/network-transport-tcp/pull/60/commits/e0742c6d21dd193c1efde5904b836bb873860914

avieth commented 7 years ago

Rebased and pushed to fix conflicts. Why isn't CI running?

facundominguez commented 7 years ago

hm, I don't know why CI is not running.

So we could have the peer deconstruct its own address and send only the port, but it was just easier to implement it as it is now: send the whole EndPointAddress.

I think the most practical is to not send the host. Otherwise, the remote peer will have to guess what its external address is to contact a given node.

avieth commented 7 years ago

I think the most practical is to not send the host. Otherwise, the remote peer will have to guess what its external address is to contact a given node.

It won't have to guess, because it will reject when they don't match. Call it a sanity check: if the connecting EndPoint is misconfigured then all connections to honest peers will fail.

facundominguez commented 7 years ago

I think I failed to communicate the point: If node A wants to connect to B, node A will have to guess which is the external address that node B expects node A to use.

Asking A to know which external address B expects of it, looks like offering little benefit for the trouble it makes. But I might be misjudging here.

And assuming A knows which external addresses other nodes expect of it, is there any way for A to control which external address is sent to each node?

avieth commented 7 years ago

If node A wants to connect to B, node A will have to guess which is the external address that node B expects node A to use.

Asking A to know which external address B expects of it, looks like offering little benefit for the trouble it makes. But I might be misjudging here.

This isn't a problem for nodes which really are addressable on the network. In the data centre, for instance, it's feasible to configure each transport with the proper address given the machine on which it runs.

The benefit is mainly for security when using a network-transport-tcp on a network that you don't completely control.

And assuming A knows which external addresses other nodes expect of it, is there any way for A to control which external address is sent to each node?

A will always send the same address as determined by the address of its Transport and its EndPointId.

facundominguez commented 7 years ago

A will always send the same address as determined by the address of its Transport and its EndPointId.

Ok, so we wouldn't allow a transport to listen for connections on multiple interfaces (say by using "0.0.0.0").

avieth commented 7 years ago

Ok, so we wouldn't allow a transport to listen for connections on multiple interfaces (say by using "0.0.0.0").

This should be fine. The source IP is not checked against anything.

There would be problems if the system uses two or more interfaces for outgoing connections. It would have to send a different EndPointAddress depending upon which interface is used. But this is a problem even without this patch. It's assumed that there's just one network address for each transport, that all incoming connections come through one interface. Is there a good reason why outgoing connections would go through some other interface?

facundominguez commented 7 years ago

Is there a good reason why outgoing connections would go through some other interface?

I can imagine an hypothetical user comming across this, but we can defer it until then,.

If this is a sanity check, I think it is fair to ask the transport to answer back to the peer with the reason for closing the connection when the addresses don't match. Otherwise the failure cause is totally invisible to the application in both the local and the remote node.

avieth commented 7 years ago

If this is a sanity check, I think it is fair to ask the transport to answer back to the peer with the reason for closing the connection when the addresses don't match. Otherwise the failure cause is totally invisible to the application in both the local and the remote node.

I'm for this. I can also make this feature optional, with the default to not check the host.

facundominguez commented 7 years ago

Sounds good to me.

avieth commented 7 years ago

Parallel connecting tests intermittently failed but I believe this has fixed it https://github.com/haskell-distributed/network-transport-tcp/pull/54/commits/6757ce91eeb3fe67cde33f1f2178e090f8348345

The bug was introduced here https://github.com/haskell-distributed/network-transport-tcp/pull/56/commits/683d3bed497b967724cfb4206fd53faa78e5bf22 and manifested as a RELY violation in createConnectionTo; the remote was in a closed/closing state because there was a connection coming up which had not yet sent CreatedNewConnection. The echo server used in network-transport-tests mimics what the client does: connecting when it connects, closing when it closes. So it could try to CloseSocket while the client is doing a bunch of concurrent connections, depending on how things are scheduled (in the RTS and by the OS networking system).

facundominguez commented 7 years ago

Thanks @avieth!