Don't allow negative date components in ParseCertificateDate(), or ones starting with a plus.

Don't allow negative date components in ParseCertificateDate(), or ones starting with a plus.

BUG=596523, 596544
Committed: https://crrev.com/1f90fcb579588d6491f04ffc586ba2bf5b2a1653
Cr-Commit-Position: refs/heads/master@{#382949}

[eroman, 2016-03-23 19:39:10]

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

[eroman, 2016-03-23 19:39:10]

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types.cc
File net/cert/x509_cert_types.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types.cc...
net/cert/x509_cert_types.cc:71: if (valid && year_length == 2)
Because of this line, negative years were not caught properly by
HasValidValues() below either! So really this is fixing 2 bugs.

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
File net/cert/x509_cert_types_unittest.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
net/cert/x509_cert_types_unittest.cc:155: {CERT_DATE_FORMAT_UTC_TIME,
"-90101000000Z", false, {0}},
Stupid reformatting! The only changes are addition of these two tests (-9 and
+9) as the year. Both were recognized, where -9 was interpreted as 2000-9  O_o

[eroman, 2016-03-23 19:50:50]

Description was changed from

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596544
==========

to

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596523, 596544
==========

[mmenke, 2016-03-23 19:58:36]

LGTM!

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
File net/cert/x509_cert_types_unittest.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
net/cert/x509_cert_types_unittest.cc:155: {CERT_DATE_FORMAT_UTC_TIME,
"-90101000000Z", false, {0}},
On 2016/03/23 19:39:10, eroman wrote:
> Stupid reformatting! The only changes are addition of these two tests (-9 and
> +9) as the year. Both were recognized, where -9 was interpreted as 2000-9  O_o

Should we have tests where some other field is weird?

"09+101000000Z", "0901-1000000Z", and/or "090101 00000Z"?  Last would have
failed before, too, but I'm paranoid.

[eroman, 2016-03-23 20:03:09]

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
File net/cert/x509_cert_types_unittest.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
net/cert/x509_cert_types_unittest.cc:155: {CERT_DATE_FORMAT_UTC_TIME,
"-90101000000Z", false, {0}},
On 2016/03/23 19:58:36, mmenke wrote:
> On 2016/03/23 19:39:10, eroman wrote:
> > Stupid reformatting! The only changes are addition of these two tests (-9
and
> > +9) as the year. Both were recognized, where -9 was interpreted as 2000-9 
O_o
> 
> Should we have tests where some other field is weird?
> 
   -90101000000Z
> "09+101000000Z", "0901-1000000Z", and/or "090101 00000Z"?  Last would have
> failed before, too, but I'm paranoid.

Yes, although because the parsing expects each component to be 2 bytes long, an
even stronger test is to change like "01" --> "+1" so it doesn't violate the
size constraint (otherwise would have failed in pervious version too).

Will add such tests shortly and ping back...

[mmenke, 2016-03-23 20:05:52]

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
File net/cert/x509_cert_types_unittest.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
net/cert/x509_cert_types_unittest.cc:155: {CERT_DATE_FORMAT_UTC_TIME,
"-90101000000Z", false, {0}},
On 2016/03/23 20:03:09, eroman wrote:
> On 2016/03/23 19:58:36, mmenke wrote:
> > On 2016/03/23 19:39:10, eroman wrote:
> > > Stupid reformatting! The only changes are addition of these two tests (-9
> and
> > > +9) as the year. Both were recognized, where -9 was interpreted as 2000-9 
> O_o
> > 
> > Should we have tests where some other field is weird?
> > 
>    -90101000000Z
> > "09+101000000Z", "0901-1000000Z", and/or "090101 00000Z"?  Last would have
> > failed before, too, but I'm paranoid.
> 
> Yes, although because the parsing expects each component to be 2 bytes long,
an
> even stronger test is to change like "01" --> "+1" so it doesn't violate the
> size constraint (otherwise would have failed in pervious version too).
> 
> Will add such tests shortly and ping back...

Isn't that what I did?  I changed each subsequent even position byte, without
modifying the size.

[eroman, 2016-03-23 20:33:43]

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
File net/cert/x509_cert_types_unittest.cc (right):

https://codereview.chromium.org/1832583002/diff/1/net/cert/x509_cert_types_un...
net/cert/x509_cert_types_unittest.cc:155: {CERT_DATE_FORMAT_UTC_TIME,
"-90101000000Z", false, {0}},
On 2016/03/23 20:05:52, mmenke wrote:
> On 2016/03/23 20:03:09, eroman wrote:
> > On 2016/03/23 19:58:36, mmenke wrote:
> > > On 2016/03/23 19:39:10, eroman wrote:
> > > > Stupid reformatting! The only changes are addition of these two tests
(-9
> > and
> > > > +9) as the year. Both were recognized, where -9 was interpreted as
2000-9 
> > O_o
> > > 
> > > Should we have tests where some other field is weird?
> > > 
> >    -90101000000Z
> > > "09+101000000Z", "0901-1000000Z", and/or "090101 00000Z"?  Last would have
> > > failed before, too, but I'm paranoid.
> > 
> > Yes, although because the parsing expects each component to be 2 bytes long,
> an
> > even stronger test is to change like "01" --> "+1" so it doesn't violate the
> > size constraint (otherwise would have failed in pervious version too).
> > 
> > Will add such tests shortly and ping back...
> 
> Isn't that what I did?  I changed each subsequent even position byte, without
> modifying the size.

Ah sg then!

Done.

[eroman, 2016-03-23 22:21:52]

The CQ bit was checked by eroman@chromium.org

[eroman, 2016-03-23 22:21:53]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1832583002/#ps40001
(title: "moar test")

[commit-bot: I haz the power, 2016-03-23 22:22:10]

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

[commit-bot: I haz the power, 2016-03-23 22:29:07]

Description was changed from

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596523, 596544
==========

to

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596523, 596544
==========

[commit-bot: I haz the power, 2016-03-23 22:29:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-23 22:31:28]

Description was changed from

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596523, 596544
==========

to

==========
Don't allow negative date components in ParseCertificateDate(), or ones starting
with a plus.

BUG=596523, 596544

Committed: https://crrev.com/1f90fcb579588d6491f04ffc586ba2bf5b2a1653
Cr-Commit-Position: refs/heads/master@{#382949}
==========

[commit-bot: I haz the power, 2016-03-23 22:31:29]

Patchset 3 (id:??) landed as
https://crrev.com/1f90fcb579588d6491f04ffc586ba2bf5b2a1653
Cr-Commit-Position: refs/heads/master@{#382949}