CHECK failure: ms <= kMaximumECMADateInMs in date_math.cc |
|||||
Issue descriptionDetailed 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.
,
Apr 11 2018
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.
,
Apr 12 2018
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.
,
Apr 12 2018
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.
,
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().
,
Apr 16 2018
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
,
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.
,
Apr 19 2018
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.
,
Apr 19 2018
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.
,
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
,
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.
,
Apr 21 2018
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.
,
Apr 25 2018
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 |
|||||
Comment 1 by ClusterFuzz
, Apr 11 2018Labels: Test-Predator-Auto-Components