Add simple HPKP and HSTS header parser fuzzers.

Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
Committed: https://crrev.com/db9ad4ff5423c8c8633cc71c1e6647af7bcf08c7
Cr-Commit-Position: refs/heads/master@{#427754}

[martijnc, 2016-10-24 20:36:30]

Patchset #1 (id:1) has been deleted

[martijnc, 2016-10-24 20:36:59]

martijn@martijnc.be changed reviewers:
+ mmenke@chromium.org

[martijnc, 2016-10-25 17:08:07]

Patchset #1 (id:20001) has been deleted

[martijnc, 2016-10-25 17:09:46]

Hi, can you review this patch?

Thanks!

[mmenke, 2016-10-25 17:14:14]

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

[mmenke, 2016-10-25 17:14:15]

[+davidben]:  Mind reviewing this, or would sleevi or estark be better?  Not
exactly a huge CL, but I'm not familiar with this code, and not sure if it's
worth fuzzing more parameters.

[aizatsky, 2016-10-25 19:11:20]

aizatsky@chromium.org changed reviewers:
+ aizatsky@chromium.org

[aizatsky, 2016-10-25 19:11:20]

https://codereview.chromium.org/2448603002/diff/40001/net/http/http_security_...
File net/http/http_security_headers_hpkp_fuzzer.cc (right):

https://codereview.chromium.org/2448603002/diff/40001/net/http/http_security_...
net/http/http_security_headers_hpkp_fuzzer.cc:23:
hash.FromString("sha256/1111111111111111111111111111111111111111111=");
does this need to be a valid hash of anything? Will the code below try to verify
it?

[martijnc, 2016-10-25 19:40:52]

https://codereview.chromium.org/2448603002/diff/40001/net/http/http_security_...
File net/http/http_security_headers_hpkp_fuzzer.cc (right):

https://codereview.chromium.org/2448603002/diff/40001/net/http/http_security_...
net/http/http_security_headers_hpkp_fuzzer.cc:23:
hash.FromString("sha256/1111111111111111111111111111111111111111111=");
On 2016/10/25 at 19:11:20, aizatsky wrote:
> does this need to be a valid hash of anything? Will the code below try to
verify it?

The hash is only compared to other hashes. Passing in an invalid hash would also
work I think. We could pass part of |data| to hash::FromString and also fuzz
that and ignore the return value. Not sure how useful or representative that
would be for HPKP.

net::ParseHPKPHeader checks that at least one of the pins matches the ones in
SSLInfo::public_key_hashes (which represents the hashes of the certificates in
the certificate chain currently in use) and that at least one of the pins does
not match. This ensures you don't lock out the user by pinning keys that you
aren't currently using and that you pin at least one backup (a key that isn't in
use).

My thinking was that passing in at least on hash (could) give the fuzzer better
coverage because IsPinListValid() returns early otherwise [1]. 

[1]
https://cs.chromium.org/chromium/src/net/http/http_security_headers.cc?sq=pac...

[aizatsky, 2016-10-25 20:50:17]

We'll have to see how well it penetrates the target in reality. E.g. in libpng
we had to disable CRC checks - it is hard to get past them.

[aizatsky, 2016-10-25 20:50:19]

lgtm

[davidben, 2016-10-25 21:32:04]

lgtm

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn#newcode2065
net/BUILD.gn:2065: "//net",
Nit: for the sake of completeness, since you include url/gurl.h in this file,
probably adding //url is prudent. (Although //net has it in public_deps anyway.)

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn#newcode2076
net/BUILD.gn:2076: "//net",
Ditto.

[mmenke, 2016-10-26 16:40:24]

Description was changed from

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
==========

to

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
==========

[mmenke, 2016-10-26 16:40:25]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[martijnc, 2016-10-26 17:10:27]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2016-10-26 17:10:28]

Thank you!

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn#newcode2065
net/BUILD.gn:2065: "//net",
On 2016/10/25 at 21:32:04, davidben wrote:
> Nit: for the sake of completeness, since you include url/gurl.h in this file,
probably adding //url is prudent. (Although //net has it in public_deps anyway.)

Done.

https://codereview.chromium.org/2448603002/diff/40001/net/BUILD.gn#newcode2076
net/BUILD.gn:2076: "//net",
On 2016/10/25 at 21:32:04, davidben wrote:
> Ditto.

Done.

[martijnc, 2016-10-26 17:10:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, aizatsky@chromium.org
Link to the patchset: https://codereview.chromium.org/2448603002/#ps60001
(title: "deps")

[commit-bot: I haz the power, 2016-10-26 17:11:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 17:20:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 17:20:48]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[martijnc, 2016-10-26 17:51:09]

The CQ bit was checked by martijn@martijnc.be

[commit-bot: I haz the power, 2016-10-26 17:51:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 18:34:35]

Description was changed from

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
==========

to

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
==========

[commit-bot: I haz the power, 2016-10-26 18:34:36]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-10-26 18:53:22]

Description was changed from

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523
==========

to

==========
Add simple HPKP and HSTS header parser fuzzers.

BUG=599523

Committed: https://crrev.com/db9ad4ff5423c8c8633cc71c1e6647af7bcf08c7
Cr-Commit-Position: refs/heads/master@{#427754}
==========

[commit-bot: I haz the power, 2016-10-26 18:53:24]

Patchset 2 (id:??) landed as
https://crrev.com/db9ad4ff5423c8c8633cc71c1e6647af7bcf08c7
Cr-Commit-Position: refs/heads/master@{#427754}

[Avi (use Gerrit), 2016-10-27 19:32:50]

On 2016/10/26 18:53:24, commit-bot: I haz the power wrote:
> Patchset 2 (id:??) landed as
> https://crrev.com/db9ad4ff5423c8c8633cc71c1e6647af7bcf08c7
> Cr-Commit-Position: refs/heads/master@{#427754}

FYI this fuzzer doesn't work correctly. See the report at
http://crbug.com/659878 .