Prevent DOS attack on UDP echo servers by distinguishing between an echo request

Prevent DOS attack on UDP echo servers by distinguishing between an echo request
and the echo response.

Client sends <version><checksum><size><payload> data to TCP/UDP echo servers.
<checksum> is the checksum of the <payload>. For the first cut, we will sum up
the characters in <payload>.

If checksum of the <payload> is verified, echo servers encrypt the data
and send back the data as <version><checksum><size><key><encrypted_payload>. 
<key> is is used to decrypt the <encrypted_payload>. <encrypted_payload> is
the encrypted <payload>.

BUG=87297
R=jar
TEST=network_stats unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96890

[ramant (doing other things), 2011-06-25 02:32:13]

Hi Jim,
  Changed the network_stats code and testserver for TCP/UDP echo servers to
support "echo request" and "echo response" messaging.

  Would appreciate your comments.

thanks much,
raman

[ramant (doing other things), 2011-06-29 23:04:08]

Hi Jim,
  Synch'ed the code from server into echo_message.py (server CL id is 22093408).

thanks much,
raman

[ramant (doing other things), 2011-07-05 19:21:08]

Hi willchan,
  Would appreciate if you could look at these changes when you have sometime.

thanks very much,
raman

[ramant (doing other things), 2011-07-21 22:02:16]

Hi Jim and Willchan,
  I have integrated all the changes from python readability into echo_message.py
(this file is same as the one that is used in the servers and we use the same
file for testservers).

  Would appreciate if you could take a look at these changes.

thanks much,
raman

[jar (doing other things), 2011-08-05 19:37:31]

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:44: static const uint32 kVersionEnd = 2;  //
kVersionStart + kVersionLength.
nit: Suggest:

kVersionEnd = kVersionStart + kVersionLength;

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:49: static const uint32 kChecksumStart = 2; 
// kVersionEnd.
nit: assign value rather than comment..

Same below.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:272: bool NetworkStats::VerifyBytes(uint32
buffer_length) {
Suggest copying bytes into a local buffer, and then only verifying when you have
all the bytes.

Logic will be simpler, and it will handle the problems of not getting parts of
the header etc.  In the future, we could even mess with a segmented buffer that
needs to be re-ordered.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:287: key_ = key;
nit: avoid memcpy, and just use a special constructor that takes the number of
bytes to transfer.

Also, you should validate your data as being in the expected range for keys.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:308: int key_byte =
static_cast<int>(key_[kIdx]);
suggest using "char" as the types for lines 307 and 308, and then yo udon't need
to cast in 309 (and you might not need casts in 307 and/or 308).

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:147: // during the decrypting the message
from the server.
nit: Suggest:

key_index_ is in the range [0, key_.size()).  It identifies the next character
of key_ to use in decoding data (we cycle through the bytes of key_).

http://codereview.chromium.org/7246021/diff/29001/net/tools/testserver/echo_m...
File net/tools/testserver/echo_message.py (right):

http://codereview.chromium.org/7246021/diff/29001/net/tools/testserver/echo_m...
net/tools/testserver/echo_message.py:188: KEY_MIN_VALUE = 100000
Suggest key in range [0, 999999]

[ramant (doing other things), 2011-08-12 01:05:08]

Hi Jim,
  Made the changes you had suggested. Would appreciate if you could take a look
at these changes.

thanks much,
raman

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:44: static const uint32 kVersionEnd = 2;  //
kVersionStart + kVersionLength.
On 2011/08/05 19:37:31, jar wrote:
> nit: Suggest:
> 
> kVersionEnd = kVersionStart + kVersionLength;

Done.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:49: static const uint32 kChecksumStart = 2; 
// kVersionEnd.
On 2011/08/05 19:37:31, jar wrote:
> nit: assign value rather than comment..
> 
> Same below.

Done.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:272: bool NetworkStats::VerifyBytes(uint32
buffer_length) {
On 2011/08/05 19:37:31, jar wrote:
> Suggest copying bytes into a local buffer, and then only verifying when you
have
> all the bytes.
> 
> Logic will be simpler, and it will handle the problems of not getting parts of
> the header etc.  In the future, we could even mess with a segmented buffer
that
> needs to be re-ordered.

Done.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:287: key_ = key;
On 2011/08/05 19:37:31, jar wrote:
> nit: avoid memcpy, and just use a special constructor that takes the number of
> bytes to transfer.
> 
> Also, you should validate your data as being in the expected range for keys.

Done.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:308: int key_byte =
static_cast<int>(key_[kIdx]);
On 2011/08/05 19:37:31, jar wrote:
> suggest using "char" as the types for lines 307 and 308, and then yo udon't
need
> to cast in 309 (and you might not need casts in 307 and/or 308).

Done.

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.h (right):

http://codereview.chromium.org/7246021/diff/29001/chrome/browser/net/network_...
chrome/browser/net/network_stats.h:147: // during the decrypting the message
from the server.
On 2011/08/05 19:37:31, jar wrote:
> nit: Suggest:
> 
> key_index_ is in the range [0, key_.size()).  It identifies the next character
> of key_ to use in decoding data (we cycle through the bytes of key_).

Deleted this variable. Not needed because we are accumulating all the data.

Done.

http://codereview.chromium.org/7246021/diff/29001/net/tools/testserver/echo_m...
File net/tools/testserver/echo_message.py (right):

http://codereview.chromium.org/7246021/diff/29001/net/tools/testserver/echo_m...
net/tools/testserver/echo_message.py:188: KEY_MIN_VALUE = 100000
On 2011/08/05 19:37:31, jar wrote:
> Suggest key in range [0, 999999]

Done.

[ramant (doing other things), 2011-08-12 22:25:30]

Hi Jim,
  Added the timer tasks to timeout if we don't get a response from echo servers
in 60 secs. Tested the following conditions (as you had suggested).

1) echo server takes long time to respond (by adding a sleep for 10 secs in the
server).
2) Sending no data.
3) not sending all the data (send no key, no pay load and partial payload and
complete payload).

  Would appreciate if you could take another look at the changes.

thanks very much,
raman

[jar (doing other things), 2011-08-15 19:33:50]

LGTM

Comments below.

The code was nice... as I thought of changing it with a few other nits.... but
in each case, I came around to thinking your approach was better (several nice
DCHECKS).

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:102: kVersionLength + kChecksumLength +
kPayloadSizeLength + kKeyLength +
style nit: (personal?): suggest not wrapping until you hit the  "+" and were
about to go past 80.  It is just "ok" if you wrap at the "=", and don't create
extra lines.  Better to have fewer lines if possible.

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:176: MessageLoop::current()->PostTask(
This deserves some comment.  You mentioned that you wanted to avoid the chance
of an infinite loop, growing the stack with each recursion.  If there is a
chance at an infinite loop, perhaps we should also add a delay, like 1ms
(10ms?), so that the time-out will fire before we have time to really hog the
CPU too extensively (waiting for the time-out).

If we are timing this, perhaps we can't afford a 10ms delay... I'm not sure.

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:258: Finish(READ_FAILED,
net::ERR_INVALID_ARGUMENT);
Is there any better error code to supply here?

http://codereview.chromium.org/7246021/diff/47001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7246021/diff/47001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1511: """Handles the request from the client
and responds back with a response."""
nit: Avoid two forms of respond (responds + response) in one sentence.  Suggest:

Handles the request from the client and constructs a response.

Same in line 1534.

[ramant (doing other things), 2011-08-16 00:00:02]

Hi Jim,
  Made all the changes you had suggested.

thanks,
raman

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
File chrome/browser/net/network_stats.cc (right):

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:102: kVersionLength + kChecksumLength +
kPayloadSizeLength + kKeyLength +
On 2011/08/15 19:33:50, jar wrote:
> style nit: (personal?): suggest not wrapping until you hit the  "+" and were
> about to go past 80.  It is just "ok" if you wrap at the "=", and don't create
> extra lines.  Better to have fewer lines if possible.

Done.

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:176: MessageLoop::current()->PostTask(
On 2011/08/15 19:33:50, jar wrote:
> This deserves some comment.  You mentioned that you wanted to avoid the chance
> of an infinite loop, growing the stack with each recursion.  If there is a
> chance at an infinite loop, perhaps we should also add a delay, like 1ms
> (10ms?), so that the time-out will fire before we have time to really hog the
> CPU too extensively (waiting for the time-out).
> 
> If we are timing this, perhaps we can't afford a 10ms delay... I'm not sure.

Done.

http://codereview.chromium.org/7246021/diff/47001/chrome/browser/net/network_...
chrome/browser/net/network_stats.cc:258: Finish(READ_FAILED,
net::ERR_INVALID_ARGUMENT);
On 2011/08/15 19:33:50, jar wrote:
> Is there any better error code to supply here?

Done.

http://codereview.chromium.org/7246021/diff/47001/net/tools/testserver/testse...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/7246021/diff/47001/net/tools/testserver/testse...
net/tools/testserver/testserver.py:1511: """Handles the request from the client
and responds back with a response."""
On 2011/08/15 19:33:50, jar wrote:
> nit: Avoid two forms of respond (responds + response) in one sentence. 
Suggest:
> 
> Handles the request from the client and constructs a response.
> 
> Same in line 1534.

Done.