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

Issue 676709 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
NOT IN USE
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux
Pri: 2
Type: Bug



Sign in to add a comment

Integer-overflow in blink::compareAspectRatioValue

Project Member Reported by ClusterFuzz, Dec 22 2016

Issue description

Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5574437900648448

Fuzzer: inferno_twister
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  blink::compareAspectRatioValue
  blink::MediaQueryEvaluator::eval
  blink::MediaQueryEvaluator::eval
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027

Minimized Testcase (0.42 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97Z3ciNFYwTK81tFJUv0nwAKdoY7g1cdZ9JQAfaNl6gwsdfa3wl5Xo2KUQPNV2aJneSsmgb0k6tNlq-4ygzSynvvHnrdia9u1NbaIogIQiJ6UyLrHfXK0aU2OoJaPfWT-r1OnRoNL4hwQbl_sEUU-T_ASYN4w?testcase_id=5574437900648448
<head id=myhead>
<script>
    var headElement = document.getElementById("myhead");
    var linkElement = document.createElement("link");
    linkElement.rel = "stylesheet";
        width = 2147483548;
        height = 2147483575;
    linkElement.media = "screen and (max-device-aspect-ratio: " + width + "/" + height + ")";
    linkElement.href = "resources/device-aspect-ratio.css";
    headElement.appendChild(linkElement);
</script>


Issue filed automatically.

See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
 
Cc: msrchandra@chromium.org
Components: Blink>CSS
Labels: Test-Predator-Wrong-CLs
Owner: pilgrim@chromium.org
Status: Assigned (was: Untriaged)
Find it and CL did not provide any possible suspect.
Using Code Search for the file, "MediaQueryEvaluator.cpp" assigning to the concern owner.
Suspecting Commit# 
https://chromium.googlesource.com/chromium/src/+/8066fbcdc10ddb7020340708a84df4aeb0099c1e

@pilgrim-- Could you please look into the issue, kindly re-assign if this is not related to your changes.
Thank You.
Cc: pilgrim@chromium.org
Labels: Update-Weekly
Owner: ----
Status: Available (was: Assigned)
pilgrim: Did you make any progress investigating this?

Comment 3 by suzyh@chromium.org, Mar 27 2017

Status: Unconfirmed (was: Available)
Setting to Unconfirmed to send this back through Blink>CSS daily triage.
Requested CF redo the blame.
Status: Available (was: Unconfirmed)
I guess the clang change...

But.. seems simple enough, height is 2147483575 and that can't fit once multiplied by 1280

../../third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:192:35: runtime error: signed integer overflow: 1280 * 2147483575 cannot be represented in type int
#0 0x7f7b2fc63827 in blink::compareAspectRatioValue(blink::MediaQueryExpValue const&, int, int, blink::MediaFeaturePrefix) third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:192:35

Comment 7 by suzyh@chromium.org, Mar 28 2017

Owner: suzyh@chromium.org
Status: Assigned (was: Available)

Comment 8 by suzyh@chromium.org, Mar 28 2017

Cc: suzyh@chromium.org
Owner: y...@yoav.ws
compareAspectRatioValue takes two int arguments. In one location it is called with two ints. In the other location it is called with two doubles and so in that case presumably has an implicit conversion from double to int.

These int arguments are each multiplied by static_cast<int>(x) where x is of type unsigned. Prior to https://codereview.chromium.org/240453010 (from 2014), x was of type float.

This is all kinds of confusing, and not knowing the media query code, I'm cautious about making assertions about what numerical types should be used here.

yoav: As author of the patch above, plus the recent https://codereview.chromium.org/1904523002 that deals with numeric types in this code, would you mind taking a look?

Comment 9 by y...@yoav.ws, Mar 28 2017

Cc: r...@opera.com
> compareAspectRatioValue takes two int arguments. In one location it is called with two ints. In the other location it is called with two doubles and so in that case presumably has an implicit conversion from double to int.

The implicit conversion seems bad, and is most probably observable. We should fix it.

> These int arguments are each multiplied by static_cast<int>(x) where x is of type unsigned. Prior to https://codereview.chromium.org/240453010 (from 2014), x was of type float.

x is supposed to be unsigned (see https://drafts.csswg.org/mediaqueries/#aspect-ratio)
At the same time, the static_cast from unsigned to int seems unnecessary.

Adding Rune, as he's more active is this code than myself
Cc: y...@yoav.ws
Owner: r...@opera.com
Rune, can you take a look?
Project Member

Comment 11 by bugdroid1@chromium.org, Apr 27 2017

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

commit cf115265d17e7eec4e8c6c2d4441942988dc84db
Author: rune <rune@opera.com>
Date: Thu Apr 27 07:13:49 2017

Better overflow handling for aspect-ratio MQ.

This change fixes two issues:

1. Clamp instead of casting double values from parser to internal
   unsigned storage.
2. Promote width/height/numerator/denominator multiplications to double
   to avoid integer overflow for large numerator/denominators.

R=yoav@yoav.ws,suzyh@chromium.org
BUG= 676709 , 714079 

Review-Url: https://codereview.chromium.org/2836613002
Cr-Commit-Position: refs/heads/master@{#467615}

[add] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/LayoutTests/fast/media/mq-aspect-ratio-overflow.html
[modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp
[modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryEvaluatorTest.cpp
[modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryExp.cpp

Comment 12 by r...@opera.com, Apr 27 2017

Status: Fixed (was: Assigned)
Project Member

Comment 13 by ClusterFuzz, Apr 28 2017

ClusterFuzz has detected this issue as fixed in range 467606:467617.

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

Fuzzer: inferno_twister
Job Type: linux_ubsan_chrome
Platform Id: linux

Crash Type: Integer-overflow
Crash Address: 
Crash State:
  blink::compareAspectRatioValue
  blink::MediaQueryEvaluator::eval
  blink::MediaQueryEvaluator::eval
  
Sanitizer: undefined (UBSAN)

Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=370022:370027
Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=467606:467617

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


See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.

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

Sign in to add a comment