New issue
Advanced search Search tips

Issue 831864 link

Starred by 1 user

Issue metadata

Status: Verified
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android
Pri: 1
Type: Bug



Sign in to add a comment

CHECK failure: ms <= kMaximumECMADateInMs in date_math.cc

Project Member Reported by ClusterFuzz, Apr 11 2018

Issue description

Detailed report: https://clusterfuzz.com/testcase?key=5910493500538880

Fuzzer: libFuzzer_mhtml_parser_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  ms <= kMaximumECMADateInMs in date_math.cc
  blink::ParseDate
  blink::MIMEHeader::ParseHeader
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=547821:547831

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5910493500538880

Issue filed automatically.

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.
 
Project Member

Comment 1 by ClusterFuzz, Apr 11 2018

Components: Platform
Labels: Test-Predator-Auto-Components
Automatically applying components based on crash stacktrace and information from OWNERS files.

If this is incorrect, please apply the Test-Predator-Wrong-Components label.
Project Member

Comment 2 by ClusterFuzz, Apr 11 2018

Labels: Test-Predator-Auto-Owner
Owner: dewittj@chromium.org
Status: Assigned (was: Untriaged)
Automatically assigning owner based on suspected regression changelist https://chromium.googlesource.com/chromium/src/+/cbd393505608eae84929c78ea1249ee2cf8e9121 ([Offline Pages] Extract main-frame URL from mhtml when rendered.).

If this is incorrect, please let us know why and apply the Test-Predator-Wrong-CLs label. If you aren't the correct owner for this issue, please unassign yourself as soon as possible so it can be re-triaged.
The minimized test case is:

Date:May1 922372

The apparent stack trace is:
 src/third_party/blink/renderer/platform/wtf/date_math.cc:300 MinimumYearForDST()
 src/third_party/blink/renderer/platform/wtf/date_math.cc:315 EquivalentYearForDST(year)
 src/third_party/blink/renderer/platform/wtf/date_math.cc:383 CalculateDSTOffset(ms, utc_offset)
 src/third_party/blink/renderer/platform/wtf/date_math.cc:800 ParseDateFromNullTerminatedCharacters(date_string)
 src/third_party/blink/renderer/platform/mhtml/mhtml_parser.cc:180 ParseHeader(buffer)

My CL added a new use of ParseDate, but I don't think that my code should be responsible for bad input to ParseDate. I'd expect bad input to return NaN.

Cc: tkent@chromium.org dcheng@chromium.org tzik@chromium.org
Components: -Platform UI>Browser>Offline Blink>Internals>WTF
Labels: OS-Android
Adding a few folks that could help me decide what tod o here.

ParseDateFromNullTerminatedCharacters(date_string, have_tz, offset); seems to parse this header as "May 1, the year 922372", which is converted to ms since the epoch.

According to JS Date.parse("May1 922372") this is 29045092172400000 ms since the epoch.

This is ultimately passed into MsToYear, which compares it to 8640000000000000.0 in the DCHECK.

The max js year is apparently Sep 12 275760.  Why is this a DCHECK if it's called in a parser?

The dcheck in question seems to have been added in 2014 so maybe I'm holding it wrong.

Comment 5 by tkent@chromium.org, Apr 13 2018

>  I'd expect bad input to return NaN.

We should fix ParseDateFromNullTerminatedCharacters() so that it returns NaN for dates later than 275760-09-1300:00Z.  However a fix would be tricky because ms argument of CalculateDSTOffset() should accept values larger than 8,640,000,000,000,000 which can be less than 8,640,000,000,000,000 after adjusting time zone offset.

We might want to replace ParseDateFromNullTerminatedCharacters() with base::Time::FromUTCString().





I think that base::Time::FromString (ultimately uses NSPR [1]) is more permissive than the Date header's spec [2].  Is that a problem here?

Perhaps this function could be even more restrictive and reject any date whose year is > 9999 - since the spec states that years are 4 digits long.  This would not be subject to timezone rules.

[1] https://cs.chromium.org/chromium/src/base/third_party/nspr/prtime.h?sq=package:chromium
[2] https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.18
and http://www.ietf.org/rfc/rfc1123.txt
and https://www.ietf.org/rfc/rfc822.txt



Comment 7 by tkent@chromium.org, Apr 17 2018

> I think that base::Time::FromString (ultimately uses NSPR [1]) is more permissive than the Date header's spec [2].

What we should know is whether base::Time::FromString is more permissive than WTF::ParseDateFromNullTerminatedCharacters. If it's true, we can replace WTF::ParseDateFromNullTerminatedCharacters with base::Time::FromString.

> Perhaps this function could be even more restrictive and reject any date whose year is > 9999

I agree with it. We don't need to handle future years in WTF::ParseDateFromNullTerminatedCharacters, which is used only for HTTP Date field or MHTML Date field.


I think base::Time::FromString is less permissive in some ways than WTF::ParseDateFrmNullTerminatedCharacters.  In particular, if the date is greater than 32767 it will reject the date.  There must be other differences.

My current idea is to just change the MHTMLParser to use WTF::Time::FromString - and not touch ParseDate.  This will appease the fuzzer.  I'm surprised this issue doesn't appear elsewhere.
https://cs.chromium.org/chromium/src/net/http/http_response_headers.cc?rcl=867f0910e006ebca81db347849eb47159eec9902&l=1142

In net they use base::Time::FromUTCString(...) to parse timevalued headers.  This implies that it's probably safe to use here and even to replace the other instances of ParseDate.
Project Member

Comment 10 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee1a7ef4d3573b44b751f51b122d2fa1eccc3caf

commit ee1a7ef4d3573b44b751f51b122d2fa1eccc3caf
Author: Justin DeWitt <dewittj@chromium.org>
Date: Fri Apr 20 17:18:30 2018

[MHTML] Fix crashes in date header parsing.

Previously the MHTML parser would use ParseDate which has bugs for several
values of the date string.  This issue is fixed by using
WTF::Time::FromString instead.  Fuzzer group link:
https://clusterfuzz.com/v2/testcases?q=group%3A6154265509494784

Note that this does not fix the underlying issue in ParseDate.

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Bug:  831864 , 834629 
Change-Id: Ib0f8210e42667bb031dd13b0c6c86204bab08822
Reviewed-on: https://chromium-review.googlesource.com/1020362
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552365}
[modify] https://crrev.com/ee1a7ef4d3573b44b751f51b122d2fa1eccc3caf/third_party/blink/renderer/platform/mhtml/mhtml_parser.cc
[modify] https://crrev.com/ee1a7ef4d3573b44b751f51b122d2fa1eccc3caf/third_party/blink/renderer/platform/mhtml/mhtml_parser_test.cc

Project Member

Comment 11 by ClusterFuzz, Apr 21 2018

ClusterFuzz has detected this issue as fixed in range 552355:552375.

Detailed report: https://clusterfuzz.com/testcase?key=5910493500538880

Fuzzer: libFuzzer_mhtml_parser_fuzzer
Job Type: libfuzzer_chrome_asan_debug
Platform Id: linux

Crash Type: CHECK failure
Crash Address: 
Crash State:
  ms <= kMaximumECMADateInMs in date_math.cc
  blink::ParseDate
  blink::MIMEHeader::ParseHeader
  
Sanitizer: address (ASAN)

Regressed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=547821:547831
Fixed: https://clusterfuzz.com/revisions?job=libfuzzer_chrome_asan_debug&range=552355:552375

Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5910493500538880

See https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/reference.md for more information.

If you suspect that the result above is incorrect, try re-doing that job on the test case report page.
Project Member

Comment 12 by ClusterFuzz, Apr 21 2018

Labels: ClusterFuzz-Verified
Status: Verified (was: Assigned)
ClusterFuzz testcase 5910493500538880 is verified as fixed, so closing issue as verified.

If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
Project Member

Comment 13 by bugdroid1@chromium.org, Apr 25 2018

Labels: merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e1ab01f21e2493c51060fb7d623e6e001f3cae3

commit 2e1ab01f21e2493c51060fb7d623e6e001f3cae3
Author: Justin DeWitt <dewittj@chromium.org>
Date: Wed Apr 25 16:09:53 2018

[MHTML] Fix crashes in date header parsing.

Previously the MHTML parser would use ParseDate which has bugs for several
values of the date string.  This issue is fixed by using
WTF::Time::FromString instead.  Fuzzer group link:
https://clusterfuzz.com/v2/testcases?q=group%3A6154265509494784

Note that this does not fix the underlying issue in ParseDate.

TBR=dewittj@chromium.org

(cherry picked from commit ee1a7ef4d3573b44b751f51b122d2fa1eccc3caf)

Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Bug:  831864 , 834629 
Change-Id: Ib0f8210e42667bb031dd13b0c6c86204bab08822
Reviewed-on: https://chromium-review.googlesource.com/1020362
Commit-Queue: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Kent Tamura <tkent@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552365}
Reviewed-on: https://chromium-review.googlesource.com/1028355
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#292}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/2e1ab01f21e2493c51060fb7d623e6e001f3cae3/third_party/blink/renderer/platform/mhtml/mhtml_parser.cc
[modify] https://crrev.com/2e1ab01f21e2493c51060fb7d623e6e001f3cae3/third_party/blink/renderer/platform/mhtml/mhtml_parser_test.cc

Sign in to add a comment