Fix number parsing for max-age for HSTS/HPKP.

Fix number parsing for max-age for HSTS/HPKP.

 * Don't allow leading '+'
 * Dont' allow trailing garbage (for overflowed numbers)

BUG=596523, 596561
Committed: https://crrev.com/e8a43d8aa92e2bdb795fbccd67a3e8add8d3a9e8
Cr-Commit-Position: refs/heads/master@{#386596}

[eroman, 2016-04-11 18:25:25]

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

[eroman, 2016-04-11 18:25:25]

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:259:
EXPECT_FALSE(ParseAsHPKPHeader("max-aged=3488923; " + good_pin, chain_hashes,
These look like max-age parsing tests with bad inputs, however I couldn't get
them to pass by correcting the max-age format. So they must be failing for a
different reason.

Do you know how these currently work?

[estark, 2016-04-11 18:29:53]

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:259:
EXPECT_FALSE(ParseAsHPKPHeader("max-aged=3488923; " + good_pin, chain_hashes,
On 2016/04/11 18:25:25, eroman wrote:
> These look like max-age parsing tests with bad inputs, however I couldn't get
> them to pass by correcting the max-age format. So they must be failing for a
> different reason.
> 
> Do you know how these currently work?

Can you clarify which you mean by "These"? Do you mean all the tests in this
function? This one in particular (line 259) looks like it has a typo in max-age
(max-aged) and it's missing the required backup pin.

It would probably be good if each of them had comments. :(

[estark, 2016-04-11 18:36:50]

lgtm

[eroman, 2016-04-11 18:46:36]

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1831963002/diff/60001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:259:
EXPECT_FALSE(ParseAsHPKPHeader("max-aged=3488923; " + good_pin, chain_hashes,
On 2016/04/11 18:29:52, estark wrote:
> On 2016/04/11 18:25:25, eroman wrote:
> > These look like max-age parsing tests with bad inputs, however I couldn't
get
> > them to pass by correcting the max-age format. So they must be failing for a
> > different reason.
> > 
> > Do you know how these currently work?
> 
> Can you clarify which you mean by "These"? Do you mean all the tests in this
> function? This one in particular (line 259) looks like it has a typo in
max-age
> (max-aged) and it's missing the required backup pin.
> 
> It would probably be good if each of them had comments. :(

Agreed that comments would be helpful.

The 12 or so tests surrounding this line all seem to be playing with invalid
stuff on max-age:
 * different mis-spellings -- max-aged, amax-age
 * negative numbers
 * fractional numbers
 * incorrect separators (double ==, missing semicolon, etc)

However that isn't the only thing wrong with these tests, since correcting the
max-age portion doesn't cause them to parse. So it is unclear exactly what these
are trying to test, and if they really are max-age tests for HPKP or not.

I didn't have time to dig deeper into it.

[eroman, 2016-04-11 18:52:27]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-04-11 18:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org
Link to the patchset: https://codereview.chromium.org/1831963002/#ps80001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-11 18:53:05]

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

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

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

[commit-bot: I haz the power, 2016-04-11 22:46:21]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[eroman, 2016-04-11 22:56:16]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-04-11 22:56:59]

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

[commit-bot: I haz the power, 2016-04-12 01:58:02]

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

[commit-bot: I haz the power, 2016-04-12 01:58:03]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[eroman, 2016-04-12 05:31:23]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-04-12 05:31:44]

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

[commit-bot: I haz the power, 2016-04-12 06:02:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-12 06:03:37]

Description was changed from

==========
Fix number parsing for max-age for HSTS/HPKP.

 * Don't allow leading '+'
 * Dont' allow trailing garbage (for overflowed numbers)

BUG=596523, 596561
==========

to

==========
Fix number parsing for max-age for HSTS/HPKP.

 * Don't allow leading '+'
 * Dont' allow trailing garbage (for overflowed numbers)

BUG=596523, 596561

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

[commit-bot: I haz the power, 2016-04-12 06:03:38]

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