Properly handle STUN requests and responses from relay servers.

content/browser/renderer_host/p2p/socket_host_tcp.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/renderer_host/p2p/socket_host_tcp.h"
 
 #include "content/common/p2p_messages.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_util.h"
@@ skipping 120 matching lines @@
   DidCompleteRead(result);
   if (state_ == STATE_OPEN) {
     DoRead();
   }
 }
 
 void P2PSocketHostTcp::OnPacket(std::vector<char>& data) {
   if (!authorized_) {
     P2PSocketHost::StunMessageType type;
     bool stun = GetStunPacketType(&*data.begin(), data.size(), &type);
-    if (stun && (type == STUN_BINDING_REQUEST ||
-                 type == STUN_BINDING_RESPONSE)) {
+    if (stun && IsRequestOrResponse(type)) {

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

How does receipt of a binding/allocate request/response imply authorization?

[Sergey Ulanov, 2011/08/09 18:15:41]

By sending/receiving the stun request or response the remote host authorizes
connection with the web app.

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

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]

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]

Sorry; forgot about the |!authorized_| surrounding this block.
The flag indicates whether the channel is bound to a peer, ready for data
transfer, so "ready", "bound" or "connected" would seem appropriate.
Should we be doing that verification here?

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

Ok, connected sounds good. I will rename it in a separate CL.
I don't think so. It will make this code more complicated, increasing
probability of security vulnerabilities, but will not provide much additional
protection.

       authorized_ = true;
     } else if (!stun || type == STUN_DATA_INDICATION) {
       LOG(ERROR) << "Received unexpected data packet from "
                  << remote_address_.ToString()
                  << " before STUN binding is finished. "
                  << "Terminating connection.";
       OnError();
       return;
     }
   }
@@ skipping 112 matching lines @@
     DoWrite();
   }
 }
 
 P2PSocketHost* P2PSocketHostTcp::AcceptIncomingTcpConnection(
     const net::IPEndPoint& remote_address, int id) {
   NOTREACHED();
   OnError();
   return NULL;
 }