Implementation of SecureP2PSocket

Implementation of SecureP2PSocket

Implement encryption on UDP for chromoting.

BUG=None
TEST=None

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

[Sergey Ulanov, 2011-06-22 23:51:35]

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:14: using net::IOBuffer;
not needed because you include io_buffer.h.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:28: const int kFrameTypeSize =
sizeof(kFrameType);
nit: put this next to kFrameType.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:39: inline void Set8(void* memory, size_t
offset, uint8 v) {
Do you really need Set8 and Get8? You just case |memory| to to uint8* in SetBE64
and GetBE64 and replace Set8 and Get8 with direct array access. It also might be
easier to optimize for compiler this way.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:60: | (static_cast<uint64>(Get8(memory,
1)) << 48)
nit: don't wrap operators, '|' should be at the end of the previous line.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:138: const int kEncryptedBufferSize =
kHeaderSize + buf_len;
This should not be a const.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:143: const int kRawMessageSize =
kSeqNumberSize + kFrameTypeSize + buf_len;
why is this a const? it depends on buf_len

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:153: // 7. Decrypt the message.
s/Decrypt/Encrypt/

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:202: return ReadInternal();
It looks wrong that ReadInternal() is recursive. Replace with a while loop?

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:249: return net::ERR_MSG_TOO_BIG;
UDP sockets just truncate packets in case read buffer is not big enough. I think
it's acceptable to do it here too. Usually Read() doesn't return
ERR_MSG_TOO_BIG.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:288: if (false && seq_number < read_seq_)
why "if(false)" ? Maybe just remove this code and add comments somewhere saying
that the behavior of this code doesn't completely confirm to the spec.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.h (right):

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.h:55: net::Socket* socket_;
SSL socket objects own underlying physical sockets, and I think we should follow
the same convention here, i.e. this should be scoped_ptr.

[Alpha Left Google, 2011-06-23 21:53:01]

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:14: using net::IOBuffer;
On 2011/06/22 23:51:35, sergeyu wrote:
> not needed because you include io_buffer.h.

This using is just so that I don't need net:: when I use IOBuffer.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:28: const int kFrameTypeSize =
sizeof(kFrameType);
On 2011/06/22 23:51:35, sergeyu wrote:
> nit: put this next to kFrameType.

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:39: inline void Set8(void* memory, size_t
offset, uint8 v) {
On 2011/06/22 23:51:35, sergeyu wrote:
> Do you really need Set8 and Get8? You just case |memory| to to uint8* in
SetBE64
> and GetBE64 and replace Set8 and Get8 with direct array access. It also might
be
> easier to optimize for compiler this way.

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:60: | (static_cast<uint64>(Get8(memory,
1)) << 48)
On 2011/06/22 23:51:35, sergeyu wrote:
> nit: don't wrap operators, '|' should be at the end of the previous line.

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:138: const int kEncryptedBufferSize =
kHeaderSize + buf_len;
On 2011/06/22 23:51:35, sergeyu wrote:
> This should not be a const.

This is just to make sure I don't modify it later.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:143: const int kRawMessageSize =
kSeqNumberSize + kFrameTypeSize + buf_len;
On 2011/06/22 23:51:35, sergeyu wrote:
> why is this a const? it depends on buf_len

Just so that I never modify it.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:153: // 7. Decrypt the message.
On 2011/06/22 23:51:35, sergeyu wrote:
> s/Decrypt/Encrypt/

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:202: return ReadInternal();
On 2011/06/22 23:51:35, sergeyu wrote:
> It looks wrong that ReadInternal() is recursive. Replace with a while loop?

I changed this code to simply return ret so that upper level should handle it.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:249: return net::ERR_MSG_TOO_BIG;
On 2011/06/22 23:51:35, sergeyu wrote:
> UDP sockets just truncate packets in case read buffer is not big enough. I
think
> it's acceptable to do it here too. Usually Read() doesn't return
> ERR_MSG_TOO_BIG.

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:288: if (false && seq_number < read_seq_)
On 2011/06/22 23:51:35, sergeyu wrote:
> why "if(false)" ? Maybe just remove this code and add comments somewhere
saying
> that the behavior of this code doesn't completely confirm to the spec.

Done.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.h (right):

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.h:55: net::Socket* socket_;
On 2011/06/22 23:51:35, sergeyu wrote:
> SSL socket objects own underlying physical sockets, and I think we should
follow
> the same convention here, i.e. this should be scoped_ptr.

Done.

[Sergey Ulanov, 2011-06-23 23:12:45]

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:14: using net::IOBuffer;
On 2011/06/23 21:53:01, Alpha wrote:
> On 2011/06/22 23:51:35, sergeyu wrote:
> > not needed because you include io_buffer.h.
> 
> This using is just so that I don't need net:: when I use IOBuffer.
Oh, yes, I misread it, sorry.

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:138: const int kEncryptedBufferSize =
kHeaderSize + buf_len;
On 2011/06/23 21:53:01, Alpha wrote:
> On 2011/06/22 23:51:35, sergeyu wrote:
> > This should not be a const.
> 
> This is just to make sure I don't modify it later.
call it encrypted_buffer_size?

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:143: const int kRawMessageSize =
kSeqNumberSize + kFrameTypeSize + buf_len;
On 2011/06/23 21:53:01, Alpha wrote:
> On 2011/06/22 23:51:35, sergeyu wrote:
> > why is this a const? it depends on buf_len
> 
> Just so that I never modify it.
raw_message_size?

http://codereview.chromium.org/7038053/diff/10001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:202: return ReadInternal();
On 2011/06/23 21:53:01, Alpha wrote:
> On 2011/06/22 23:51:35, sergeyu wrote:
> > It looks wrong that ReadInternal() is recursive. Replace with a while loop?
> 
> I changed this code to simply return ret so that upper level should handle it.

I think it's better to handle it here. In most cases (at least for remoting
code) reader stops reading after the first error, so an attacker can just send
invalid packet to terminate a session. Protection from invalid packets is
provided by the encryption layer, so it should handle it, not the upper layer.

[Alpha Left Google, 2011-06-24 00:08:02]

updated with test for the retry case.

[Alpha Left Google, 2011-06-24 18:52:35]

On 2011/06/24 00:08:02, Alpha wrote:
> updated with test for the retry case.

ping.

[Sergey Ulanov, 2011-06-24 19:15:26]

LGTM

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:198: if (ret ==
net::ERR_INVALID_RESPONSE)
nit: Add comment explaining why we retry here.

[Wez, 2011-06-24 23:23:51]

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:16: namespace remoting {
nit: Add a blank line after namespace foo {.

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:19: namespace {
nit: Blank line after namespace {.

[Sergey Ulanov, 2011-06-24 23:36:40]

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
File remoting/protocol/secure_p2p_socket.cc (right):

http://codereview.chromium.org/7038053/diff/16001/remoting/protocol/secure_p2...
remoting/protocol/secure_p2p_socket.cc:16: namespace remoting {
On 2011/06/24 23:23:51, Wez wrote:
> nit: Add a blank line after namespace foo {.
IMO it is not necessary in case of nested namespaces like remoting::protocol.