Properly handle STUN requests and responses from relay servers.

Properly handle STUN requests and responses from relay servers.

BUG=83242
TEST=Chromoting can connect using relay servers.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96072

[Wez, 2011-08-09 18:09:23]

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
How does receipt of a binding/allocate request/response imply authorization?

[Sergey Ulanov, 2011-08-09 18:15:40]

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
On 2011/08/09 18:09:23, Wez wrote:
> How does receipt of a binding/allocate request/response imply authorization?
By sending/receiving the stun request or response the remote host authorizes
connection with the web app.

[Wez, 2011-08-09 18:30:57]

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
On 2011/08/09 18:15:41, sergeyu wrote:
> On 2011/08/09 18:09:23, Wez wrote:
> > How does receipt of a binding/allocate request/response imply authorization?
> By sending/receiving the stun request or response the remote host authorizes
> connection with the web app.

The only effect that |authorized_| seems to have is to prevent out-going data
traffic; should it also block incoming data traffic if false?  I'm still not
clear on how this is authorization - it seems to reflect whether the channel is
bound, with no security guarantee regarding whether the peer is authorized?

[Sergey Ulanov, 2011-08-09 19:18:26]

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
On 2011/08/09 18:30:57, Wez wrote:
> On 2011/08/09 18:15:41, sergeyu wrote:
> > On 2011/08/09 18:09:23, Wez wrote:
> > > How does receipt of a binding/allocate request/response imply
authorization?
> > By sending/receiving the stun request or response the remote host authorizes
> > connection with the web app.
> 
> The only effect that |authorized_| seems to have is to prevent out-going data
> traffic; should it also block incoming data traffic if false?  I'm still not
> clear on how this is authorization - it seems to reflect whether the channel
is
> bound, with no security guarantee regarding whether the peer is authorized?

We do block incoming data packets before stun request or response is received -
see lines 143-149.

|authorized_| means that we've received STUN request or response from the remote
end, which means it is safe to relay data from/to web app. 'Authorization' may
not be the best term for this, your suggestions on what would be a better name
here are welcome.

Yes, authorized_ is usually set when the connection is bound, but we don't
inspect internals of STUN packets, e.g. don't verify that transaction id in
request and response match, so it will not be correct to call it |bound_|.

[Wez, 2011-08-09 19:54:50]

LGTM with a couple of remaining comments.

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
On 2011/08/09 19:18:26, sergeyu wrote:
> We do block incoming data packets before stun request or response is received
-
> see lines 143-149.

Sorry; forgot about the |!authorized_| surrounding this block.

> |authorized_| means that we've received STUN request or response from the
remote
> end, which means it is safe to relay data from/to web app. 'Authorization' may
> not be the best term for this, your suggestions on what would be a better name
> here are welcome.

The flag indicates whether the channel is bound to a peer, ready for data
transfer, so "ready", "bound" or "connected" would seem appropriate.

> Yes, authorized_ is usually set when the connection is bound, but we don't
> inspect internals of STUN packets, e.g. don't verify that transaction id in
> request and response match, so it will not be correct to call it |bound_|.

Should we be doing that verification here?

[Sergey Ulanov, 2011-08-09 21:06:56]

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
File content/browser/renderer_host/p2p/socket_host_tcp.cc (right):

http://codereview.chromium.org/7529039/diff/3001/content/browser/renderer_hos...
content/browser/renderer_host/p2p/socket_host_tcp.cc:141: if (stun &&
IsRequestOrResponse(type)) {
On 2011/08/09 19:54:50, Wez wrote:
> On 2011/08/09 19:18:26, sergeyu wrote:
> > We do block incoming data packets before stun request or response is
received
> -
> > see lines 143-149.
> 
> Sorry; forgot about the |!authorized_| surrounding this block.
> 
> > |authorized_| means that we've received STUN request or response from the
> remote
> > end, which means it is safe to relay data from/to web app. 'Authorization'
may
> > not be the best term for this, your suggestions on what would be a better
name
> > here are welcome.
> 
> The flag indicates whether the channel is bound to a peer, ready for data
> transfer, so "ready", "bound" or "connected" would seem appropriate.

Ok, connected sounds good. I will rename it in a separate CL.

> 
> > Yes, authorized_ is usually set when the connection is bound, but we don't
> > inspect internals of STUN packets, e.g. don't verify that transaction id in
> > request and response match, so it will not be correct to call it |bound_|.
> 
> Should we be doing that verification here?

I don't think so. It will make this code more complicated, increasing
probability of security vulnerabilities, but will not provide much additional
protection.