Undefined-shift in double WTF::toDoubleType<unsigned char, |
|||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=5490180597481472 Fuzzer: inferno_twister Job Type: linux_ubsan_chrome Platform Id: linux Crash Type: Undefined-shift Crash Address: Crash State: double WTF::toDoubleType<unsigned char, bool blink::parseSimpleLength<unsigned char> blink::CSSParserFastPaths::maybeParseValue Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027 Minimized Testcase (10.10 Kb): https://cluster-fuzz.appspot.com/download/AMIfv97exKM4Rk1gvNFRtUqclmNhIq_NDWT7w6J35MyPiJ5f5DSixDbt0fS_dAJ0PGav3bCmYamp6dxYSUBvnepR8wdEe2UxnxmuujHTEv6zLLS5BxMZjwubZaebWIiAZU4EGg-EGkn6XkKy_8HiSLaFP22imNn_qg?testcase_id=5490180597481472 Additional requirements: Requires HTTP Filer: brajkumar See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Jul 19 2016
The stack in this bug is truncated at a really weird spot, here's a the useful one. The test case is confusing because it seems like it's just assigning a zero or a one value to the
../../third_party/WebKit/Source/wtf/dtoa/strtod.cc:271:15: runtime error: shift exponent 63 is too large for 32-bit type int
#0 0x7f1a96d50b24 in DiyFpStrtod third_party/WebKit/Source/wtf/dtoa/strtod.cc:271:15
#1 0x7f1a96d50b24 in WTF::double_conversion::Strtod(WTF::double_conversion::Vector<char const>, int) third_party/WebKit/Source/wtf/dtoa/strtod.cc:437
#2 0x7f1a96d4c41e in WTF::double_conversion::StringToDoubleConverter::StringToDouble(char const*, unsigned long, unsigned long*) third_party/WebKit/Source/wtf/dtoa/double-conversion.cc:596:28
#3 0x7f1a96d68102 in parseDouble third_party/WebKit/Source/wtf/dtoa.h:48:12
#4 0x7f1a96d68102 in double WTF::toDoubleType<unsigned char, (WTF::TrailingJunkPolicy)0>(unsigned char const*, unsigned long, bool*, unsigned long&) third_party/WebKit/Source/wtf/text/StringToNumber.cpp:217
#5 0x7f1a96d68070 in WTF::charactersToDouble(unsigned char const*, unsigned long, bool*) third_party/WebKit/Source/wtf/text/StringToNumber.cpp:233:12
#6 0x7f1a92f4014f in bool blink::parseSimpleLength<unsigned char>(unsigned char const*, unsigned int, blink::CSSPrimitiveValue::UnitType&, double&) third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:89:14
#7 0x7f1a92f3ec1f in parseSimpleLengthValue third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:107:14
#8 0x7f1a92f3ec1f in blink::CSSParserFastPaths::maybeParseValue(blink::CSSPropertyID, WTF::String const&, blink::CSSParserMode) third_party/WebKit/Source/core/css/parser/CSSParserFastPaths.cpp:1073
#9 0x7f1a92f3cf85 in blink::CSSParser::parseSingleValue(blink::CSSPropertyID, WTF::String const&, blink::CSSParserContext const&) third_party/WebKit/Source/core/css/parser/CSSParser.cpp:107:27
#10 0x7f1a93739886 in blink::SVGLength::setValueAsString(WTF::String const&) third_party/WebKit/Source/core/svg/SVGLength.cpp:142:24
#11 0x7f1a9373d202 in locus third_party/WebKit/Source/core/svg/SVGParsingError.h:77:37
#12 0x7f1a9373d202 in offsetWith third_party/WebKit/Source/core/svg/SVGParsingError.h:80
#13 0x7f1a9373d202 in blink::SVGParsingError blink::SVGLengthList::parseInternal<unsigned char>(unsigned char const*&, unsigned char const*) third_party/WebKit/Source/core/svg/SVGLengthList.cpp:88
#14 0x7f1a9373cd5b in blink::SVGLengthList::setValueAsString(WTF::String const&) third_party/WebKit/Source/core/svg/SVGLengthList.cpp:105:16
#15 0x7f1a93728fee in blink::SVGElement::parseAttribute(blink::QualifiedName const&, WTF::AtomicString const&, WTF::AtomicString const&) third_party/WebKit/Source/core/svg/SVGElement.cpp:583:48
The test case is minimized poorly but ends up being:
<text x="-0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001" />
in SVG, so it's just a super super long double which overflows the exponent code. As with the CSS overflow bugs it's not clear that fixing this is worth it, real code doesn't do this and it just makes things slower, but maybe. I'll let the SVG folks decide. :)
,
Jul 19 2016
,
Jul 19 2016
Since this is code that is shared between everyone that feels like doing string-to-double conversion (and as can be seen from the stack, this is essentially the CSS fastpath parser/tokenizer.) I checked the "original source" in V8, and it seemed to have this issue as well, so this can likely be triggered there as well. It appears the worst that could happen here is that something gets shifted into the signbit, which I suspect will result in a weird error estimate. I checked base::StringToDouble too for kicks, and it's using its own copy of DMG dtoa... https://cs.chromium.org/search/?q=file:dtoa.cc&sq=package:chromium&type=cs
,
Jul 19 2016
I'm all for WontFix these issues that only matter if there are security implications, and where there are no security implications. Basically, we can crash or do whatever we want if you have content that is beyond the range of inputs we wish to handle. I've asked about suppressing the test case in another bugs, and I ask again here: Do we need to suppress something?
,
Jul 21 2016
,
Aug 8 2016
,
Aug 8 2016
Issue 629742 has been merged into this issue.
,
Aug 9 2016
As far as I can tell, DiyFpStrtod is only called for numbers 38 digits or longer (kExactPowersOfTenSize + kMaxExactDoubleIntegerDecimalDigits = 38). It seems to only overflow 'error' when input.f_ is sufficiently small (10 digits or fewer). We could just clamp parsed doubles to 10^38 and delete DiyFpStrtod. However I think DiyFp.normalize is wrong and should be fixed. E.g. normalizing a DiyFp with f=7000000001 e=0 gives f=15032385538147483648 e=-31. That seems wrong.
,
Sep 7 2016
Issue 642516 has been merged into this issue.
,
Sep 7 2016
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5224911947956224 Fuzzer: inferno_twister Job Type: linux_ubsan_chrome Platform Id: linux Crash Type: Undefined-shift Crash Address: Crash State: double WTF::toDoubleType<unsigned char, blink::parseToDoubleForNumberType blink::NumberInputType::sanitizeValue Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027 Minimized Testcase (0.36 Kb): Download: https://cluster-fuzz.appspot.com/download/AMIfv957HtSVCNTbKUoTPbMMXCITGzBNcRoK7mV-inxl-NSXobpZVmNKv4I1rfgXpNahMljxxK32aCU9edgM0gPYoLVSjPWGKrpO1mU6cK4WXHWUIev12Aah3NMkYx90TjREzkGMOAIw07iHcewVGGV99qawyTrCYg?testcase_id=5224911947956224 <script src=../../../resources/js-test.js></script> <script> var input = document.createElement('input'); function setInputAttributes(min, max, step, value) { input.value = value; } function stepUp(value, step, max) { setInputAttributes(null, max, step, value); } input.type = 'number'; shouldBe('input.min = ""; stepUp("1e+308", "1", "", 999999)'); </script> See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Sep 7 2016
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=4749781945810944 Fuzzer: inferno_twister Job Type: linux_ubsan_chrome Platform Id: linux Crash Type: Undefined-shift Crash Address: Crash State: double WTF::toDoubleType<unsigned char, bool blink::parseTransformNumberArguments<unsigned char const> blink::CSSParserFastPaths::maybeParseValue Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027 Minimized Testcase (1.27 Kb): https://cluster-fuzz.appspot.com/download/AMIfv979ezbqi9_lS4c4cXREp8Vb0uI3U0_2nLxzVc9h5X2_byzZbsi7dYj6mV2HHOl5j3NLgUlyNyj7WyKrxLbZffTz7aCaOoeeJ1pCk7kVvXiRux9Jk7YsSGJvQc2bPF6AqoyWC-lIbyPB12O8E_O3d1juyz3U8w?testcase_id=4749781945810944 See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Sep 7 2016
* Is this distinct from bug 637044 ? * Please don't WontFix fuzzer bugs, they will just keep occurring in the future. It is not OK to "crash or do whatever we want if you have content that is beyond the range of inputs we wish to handle". Malicious webpage input should not be able to cause a crash even if that is not a security hole.
,
Sep 7 2016
Bug 637044 looks like the same thing (different shift amount, but that just equates to different length number.)
,
Sep 8 2016
Note that this causes an undefined behavior (in C++ sense), not a crash. Sounds like the browser will keep running anyway.
,
Oct 10 2016
,
Oct 21 2016
,
Oct 21 2016
,
Oct 22 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d74c8d750deee3b9412b978b1dc49e2d3f0d75d6 commit d74c8d750deee3b9412b978b1dc49e2d3f0d75d6 Author: esprehn <esprehn@chromium.org> Date: Sat Oct 22 04:47:30 2016 Fix undefined shift in DiyFpStrtod. This code is just copy pasta from v8, this patch copies a newer version of the pasta. https://chromium.googlesource.com/v8/v8/+/374a4da83e6f4e05d31640b23b5ca92f0bbf0586/src/strtod.cc#249 BUG= 637044 , 629034 Review-Url: https://chromiumcodereview.appspot.com/2438383003 Cr-Commit-Position: refs/heads/master@{#426973} [modify] https://crrev.com/d74c8d750deee3b9412b978b1dc49e2d3f0d75d6/third_party/WebKit/Source/wtf/dtoa/strtod.cc
,
Oct 22 2016
,
Oct 23 2016
ClusterFuzz testcase is verified as fixed, closing issue. If this is incorrect, please add ClusterFuzz-Wrong label and re-open the issue.
,
Nov 22 2016
Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by brajkumar@chromium.org
, Jul 18 2016Owner: esprehn@chromium.org
Status: Assigned (was: Available)