New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 629034 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Undefined-shift in double WTF::toDoubleType<unsigned char,

Project Member Reported by ClusterFuzz, Jul 18 2016

Issue description

Detailed 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.
 
Labels: -Pri-1 findit-for-crash Te-Logged Pri-2
Owner: esprehn@chromium.org
Status: Assigned (was: Available)
No CL in the regression range changes the crashed files. The result is the blame information.

Author: esprehn
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/7dc7c0fc073b84f3050726b52ceec1bbcb304ba3
Time: Fri Jun 10 04:35:59 2016
The CL last changed line 233 of file StringToNumber.cpp, which is stack frame 5.

Suspected Project: chromium
===============================
esprehn@: Could you please look into this issue if it is related to your change, else please help us in assigning it to the right owner.

Thanks!
Components: Blink>SVG
Owner: ----
Status: Untriaged (was: Assigned)
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. :)
Labels: -OS-Linux OS-All

Comment 4 by f...@opera.com, Jul 19 2016

Components: -Blink>SVG Blink>Internals>WTF
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
Cc: schenney@chromium.org
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?


Comment 6 by f...@opera.com, Jul 21 2016

Cc: f...@opera.com
 Issue 630231  has been merged into this issue.
Cc: timloh@chromium.org shans@chromium.org
 Issue 629744  has been merged into this issue.
 Issue 629742  has been merged into this issue.
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.
 Issue 642516  has been merged into this issue.
Project Member

Comment 11 by ClusterFuzz, 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.
Project Member

Comment 12 by ClusterFuzz, 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.
* 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.

Comment 14 by f...@opera.com, Sep 7 2016

 Bug 637044  looks like the same thing (different shift amount, but that just equates to different length number.)
Note that this causes an undefined behavior (in C++ sense), not a crash.
Sounds like the browser will keep running anyway.

Comment 16 by f...@opera.com, Oct 10 2016

Cc: nyerramilli@chromium.org
 Issue 654326  has been merged into this issue.
Owner: esprehn@chromium.org
Status: Assigned (was: Untriaged)
Cc: esprehn@chromium.org whesse@chromium.org
 Issue 653695  has been merged into this issue.
Project Member

Comment 19 by bugdroid1@chromium.org, 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

Comment 20 by whesse@google.com, Oct 22 2016

Cc: floitsch@google.com
Project Member

Comment 21 by ClusterFuzz, Oct 23 2016

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

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

Comment 22 by sheriffbot@chromium.org, Nov 22 2016

Labels: -Restrict-View-EditIssue
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