Add SOCKS4 and SOCKS5 fuzzers.

Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525
Committed: https://crrev.com/99b5717592526361111ebf4558d1a22327c3ccde
Cr-Commit-Position: refs/heads/master@{#387413}

[mmenke, 2016-04-14 02:51:17]

Patchset #4 (id:60001) has been deleted

[mmenke, 2016-04-14 14:48:15]

Description was changed from

==========
Attempt to fuzz SOCKS4

BUG=none
==========

to

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=none
==========

[mmenke, 2016-04-14 14:48:52]

Description was changed from

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=none
==========

to

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525
==========

[mmenke, 2016-04-14 15:02:28]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2016-04-14 15:02:28]

Eric:  WDYT?  The HttpStreamParser can easily be rewritten to use FuzzedSocket
as well.

I considered writing unit tests for FuzzedSocket, given its complexity, but we'd
either have to have a hard dependency on how seeds are mapped to behaviors, or
override all the functions to get data from the seed.  In the latter case, we'd
probably want to change what each method returned before each read/write, and
once we're to that level of micromanagement, not sure how useful the tests would
be.  Happy to spend more time thinking about it, though - it is a fairly
complicated class, not to have tests for it.

Could add some reads/writes after the handshake - don't think we could be all
that thorough there, though, so not sure it's worth the effort.

I've tested this code, and it manages to generate successful SOCKS4 and SOCKS5
handshakes in 4 to 30 minutes.  SOCKS4 is always near the lower end, SOCKS5 is
often near the higher end, but seen it happen near the lower end, too.

[eroman, 2016-04-14 17:56:28]

Nice!

I agree this is a very useful primitive to have, and that separate unit-testing
isn't necessary. We will already exercise this though the consuming fuzz-tests
so if it has bugs we will either get crashes or crappy coverage.

My comments are mostly around naming and improvements, but overall looks good :)

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
File net/socket/fuzzed_socket.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:37: DCHECK(!read_pending_);
Do we run DCHECKs in fuzzers?

Seems like we definitely should. If not this shoudl be a CHECK() since it would
be interesting if it.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:69: result = OK;
This is implementing the socket interface.
So through the joys of our overloaded use of "int" and net errors, really this
should be a straight up 0 (EOF), since both Read() and Write() return (num_bytes
| error).

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:87: DCHECK(!write_pending_);
Same dcheck question.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:105: if (static_cast<size_t>(result) >
seed_.length())
duplicated line?

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:146: return OK;
Might consider fuzzing this too (use first byte to similarly decide async vs
sync vs error).

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:188: return total_bytes_written_ != 0 ||
total_bytes_read_ != 0;
technically in the case where we returned 0 from read/write (EOF) I believe this
should also return true.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:217: uint8_t FuzzedSocket::GetUint8FromSeed() {
optional: Instead of "Get*" I would suggest something more like "Consume" /
"Read" / "Next". If you decide to go that route use that language throughout.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
File net/socket/fuzzed_socket.h (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.h:36: // |data| must be of length |data_size| and is
used as a seed to determine
I was a bit confused when reading this on the meaning of "seed" used throughout.

Based on the variable names and earlier documentation I was expecting this to
feed into a PRNG to generate the actual output stream of byte (which I don't
think would be appropriate for the fuzzer).

I think the implementation is the right approach. But would suggest different
comments and perhaps choosing a name other than "seed" (which at least in my
mind implies a small number that would inflate the output to a larger range).
The seed in this case is really the full data to read.

Seed may still be a correct term here, just one that tripped me up.

Shrug.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
File net/socket/socks5_client_socket_fuzzer.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
net/socket/socks5_client_socket_fuzzer.cc:41: net::HostResolver::RequestInfo
request_info(net::HostPortPair("foo", 80));
Fuzzing different host portpairs would be a useful extension.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks_clien...
File net/socket/socks_client_socket_fuzzer.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks_clien...
net/socket/socks_client_socket_fuzzer.cc:43: net::MockHostResolver
mock_host_resolver;
It would be interesting to fuzz the host resolution as well. These states would
be the most interesting I think:

   * syn vs async completion
   * resolution failure
   * IPv4 vs IPv6 result (in theory the implementation requests IPv4 only so
IPv6 result would just exercise the failure case)

The actual hostname itself doesn't need to be an input since we don't use it
other than for resolving.

[mmenke, 2016-04-14 18:59:23]

Patchset #6 (id:120001) has been deleted

[mmenke, 2016-04-14 19:01:51]

I did ignore two of your comments:
  * The one on WasEverUsed - you're right, but I'm copying behavior of a real
class.
  * Fuzzing on host name / port for SOCKS5.

I'll plan to land as-is later today, but feel free to speak up if you think I
should address those before landing.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
File net/socket/fuzzed_socket.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:37: DCHECK(!read_pending_);
On 2016/04/14 17:56:27, eroman wrote:
> Do we run DCHECKs in fuzzers?
> 
> Seems like we definitely should. If not this shoudl be a CHECK() since it
would
> be interesting if it.

Good question...I *think* we do, but I've switched to CHECK, regardless - don't
get anything from making it a DCHECK, anyways.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:69: result = OK;
On 2016/04/14 17:56:27, eroman wrote:
> This is implementing the socket interface.
> So through the joys of our overloaded use of "int" and net errors, really this
> should be a straight up 0 (EOF), since both Read() and Write() return
(num_bytes
> | error).

Done.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:87: DCHECK(!write_pending_);
On 2016/04/14 17:56:27, eroman wrote:
> Same dcheck question.

Done.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:105: if (static_cast<size_t>(result) >
seed_.length())
On 2016/04/14 17:56:27, eroman wrote:
> duplicated line?

Done.  Actually, removed *both* copies, as they're completely unnecessary here.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:146: return OK;
On 2016/04/14 17:56:27, eroman wrote:
> Might consider fuzzing this too (use first byte to similarly decide async vs
> sync vs error).

It's not relevant for these fuzzers.  I'm thinking I'd add an option, disabled
by default, to fuzz that in a future CL.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:188: return total_bytes_written_ != 0 ||
total_bytes_read_ != 0;
On 2016/04/14 17:56:27, eroman wrote:
> technically in the case where we returned 0 from read/write (EOF) I believe
this
> should also return true.

This is actually duplicating tcp_client_socket's behavior, as I understand it: 
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/tcp_cli...

Yes, that behavior may not be correct, but seems like I should leave it as-is,
for now.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:217: uint8_t FuzzedSocket::GetUint8FromSeed() {
On 2016/04/14 17:56:27, eroman wrote:
> optional: Instead of "Get*" I would suggest something more like "Consume" /
> "Read" / "Next". If you decide to go that route use that language throughout.

Done.  Went with Consume*FromData.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
File net/socket/fuzzed_socket.h (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.h:36: // |data| must be of length |data_size| and is
used as a seed to determine
On 2016/04/14 17:56:27, eroman wrote:
> I was a bit confused when reading this on the meaning of "seed" used
throughout.
> 
> Based on the variable names and earlier documentation I was expecting this to
> feed into a PRNG to generate the actual output stream of byte (which I don't
> think would be appropriate for the fuzzer).
> 
> I think the implementation is the right approach. But would suggest different
> comments and perhaps choosing a name other than "seed" (which at least in my
> mind implies a small number that would inflate the output to a larger range).
> The seed in this case is really the full data to read.
> 
> Seed may still be a correct term here, just one that tripped me up.
> 
> Shrug.

Done.  I think of it as an input seed -> behavior, but can certainly understand
your view, and I'm not sure who's technically correct here.  I'd switched to
using "data" everywhere.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
File net/socket/socks5_client_socket_fuzzer.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
net/socket/socks5_client_socket_fuzzer.cc:41: net::HostResolver::RequestInfo
request_info(net::HostPortPair("foo", 80));
On 2016/04/14 17:56:28, eroman wrote:
> Fuzzing different host portpairs would be a useful extension.

Makes sense, but let's skip that, for now.  I'm not sure what sort of host names
we should allow - 0-length ones, characters that aren't normally allowed in host
names, etc.

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks_clien...
File net/socket/socks_client_socket_fuzzer.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks_clien...
net/socket/socks_client_socket_fuzzer.cc:43: net::MockHostResolver
mock_host_resolver;
On 2016/04/14 17:56:28, eroman wrote:
> It would be interesting to fuzz the host resolution as well. These states
would
> be the most interesting I think:
> 
>    * syn vs async completion
>    * resolution failure
>    * IPv4 vs IPv6 result (in theory the implementation requests IPv4 only so
> IPv6 result would just exercise the failure case)
> 
> The actual hostname itself doesn't need to be an input since we don't use it
> other than for resolving.

Done.  I'm returning a fixed address in each case - doesn't seem worth the
effort to do more.

[eroman, 2016-04-14 19:32:39]

lgtm

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
File net/socket/fuzzed_socket.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
net/socket/fuzzed_socket.cc:37: DCHECK(!read_pending_);
On 2016/04/14 19:01:51, mmenke wrote:
> On 2016/04/14 17:56:27, eroman wrote:
> > Do we run DCHECKs in fuzzers?
> > 
> > Seems like we definitely should. If not this shoudl be a CHECK() since it
> would
> > be interesting if it.
> 
> Good question...I *think* we do, but I've switched to CHECK, regardless -
don't
> get anything from making it a DCHECK, anyways.

(At least when I build fuzzers locally following the recommended settings, we
*do* enable DCHECKs. So I expect this is a moot point and DCHECK is always
enabled when running in clusterfuzz.)

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
File net/socket/socks5_client_socket_fuzzer.cc (right):

https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
net/socket/socks5_client_socket_fuzzer.cc:41: net::HostResolver::RequestInfo
request_info(net::HostPortPair("foo", 80));
On 2016/04/14 19:01:51, mmenke wrote:
> On 2016/04/14 17:56:28, eroman wrote:
> > Fuzzing different host portpairs would be a useful extension.
> 
> Makes sense, but let's skip that, for now.  I'm not sure what sort of host
names
> we should allow - 0-length ones, characters that aren't normally allowed in
host
> names, etc.

In theory the hostname comes from a normalized GURL so should be valid. moreover
shouldn't make a difference to the socks (client) code. The only interesting
data would be a hostname over 255 bytes long, since that will trigger a
different codepath.

Yeah fine to not worry about this.

[mmenke, 2016-04-14 19:37:09]

Thanks!

On 2016/04/14 19:32:39, eroman wrote:
> lgtm
> 
>
https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
> File net/socket/fuzzed_socket.cc (right):
> 
>
https://codereview.chromium.org/1854813004/diff/100001/net/socket/fuzzed_sock...
> net/socket/fuzzed_socket.cc:37: DCHECK(!read_pending_);
> On 2016/04/14 19:01:51, mmenke wrote:
> > On 2016/04/14 17:56:27, eroman wrote:
> > > Do we run DCHECKs in fuzzers?
> > > 
> > > Seems like we definitely should. If not this shoudl be a CHECK() since it
> > would
> > > be interesting if it.
> > 
> > Good question...I *think* we do, but I've switched to CHECK, regardless -
> don't
> > get anything from making it a DCHECK, anyways.
> 
> (At least when I build fuzzers locally following the recommended settings, we
> *do* enable DCHECKs. So I expect this is a moot point and DCHECK is always
> enabled when running in clusterfuzz.)
> 
>
https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
> File net/socket/socks5_client_socket_fuzzer.cc (right):
> 
>
https://codereview.chromium.org/1854813004/diff/100001/net/socket/socks5_clie...
> net/socket/socks5_client_socket_fuzzer.cc:41: net::HostResolver::RequestInfo
> request_info(net::HostPortPair("foo", 80));
> On 2016/04/14 19:01:51, mmenke wrote:
> > On 2016/04/14 17:56:28, eroman wrote:
> > > Fuzzing different host portpairs would be a useful extension.
> > 
> > Makes sense, but let's skip that, for now.  I'm not sure what sort of host
> names
> > we should allow - 0-length ones, characters that aren't normally allowed in
> host
> > names, etc.
> 
> In theory the hostname comes from a normalized GURL so should be valid.
moreover
> shouldn't make a difference to the socks (client) code. The only interesting
> data would be a hostname over 255 bytes long, since that will trigger a
> different codepath.
> 
> Yeah fine to not worry about this.

[mmenke, 2016-04-14 19:37:15]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-04-14 19:37:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1854813004/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1854813004/140001

[commit-bot: I haz the power, 2016-04-14 20:44:44]

Description was changed from

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525
==========

to

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525
==========

[commit-bot: I haz the power, 2016-04-14 20:44:45]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-04-14 20:46:13]

Description was changed from

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525
==========

to

==========
Add SOCKS4 and SOCKS5 fuzzers.

Introduces a FuzzedSocket class, which takes a seed and produces random
read/write results from it.

Also removes log spam from the SOCKS4 code.

BUG=599525

Committed: https://crrev.com/99b5717592526361111ebf4558d1a22327c3ccde
Cr-Commit-Position: refs/heads/master@{#387413}
==========

[commit-bot: I haz the power, 2016-04-14 20:46:14]

Patchset 6 (id:??) landed as
https://crrev.com/99b5717592526361111ebf4558d1a22327c3ccde
Cr-Commit-Position: refs/heads/master@{#387413}