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

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Oct 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment

ASSERTION FAILED: curr->isRenderInline(), UNKNOWN in blink::RenderInline::splitInlines

Project Member Reported by ClusterFuzz, Sep 18 2014

Issue description

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

Fuzzer: Bj_broddelwerk
Job Type: Linux_asan_chrome_v8_arm

Crash Type: UNKNOWN
Crash Address: 0xfbadbeef
Crash State:
  blink::RenderInline::splitInlines
  blink::RenderInline::splitFlow
  blink::RenderInline::addChildIgnoringContinuation
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_v8_arm&range=295079:295250

Minimized Testcase (0.07 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv95730883Z0-aU2-WFc-2L-DsACnNNc9bOrWhHOHWi7mGIb5EBC6srmuSy_vazXOx-eMixq0ma88SS_TW4AMUXZt-HwijrlUp_ndV-RvTAQjoragO31B9h_-sEtO_7PWLOP4r3p0oVvLcSEuCPGIf_Vk9DT9zA
<video><style>
*::-webkit-media-controls{vertical-align:sub;display:inline;


Filer: inferno
 
Owner: aber...@chromium.org
Status: Assigned
looks like regression from https://chromium.googlesource.com/chromium/blink/+/69fca698891bbd8376f59de45b327acd19cc7943
Project Member

Comment 2 by ClusterFuzz, Sep 18 2014

Labels: M-39 Cr-Blink-Rendering Pri-1
Project Member

Comment 3 by ClusterFuzz, Sep 18 2014

Labels: ReleaseBlock-Beta
This medium+ severity security issue is a regression on trunk.

Please fix this asap. If you are unable to look into this soon, please revert your change.

- Your friendly ClusterFuzz
Probably not a good idea to simply revert, since this CL was fixing a crash. I will investigate.
https://chromium.googlesource.com/chromium/blink/+/69fca698891bbd8376f59de45b327acd19cc7943 is not the problem. I have just tried reverting it and doing so makes no difference.

I also have https://chromium.googlesource.com/chromium/blink/+/ea0529c82496b6f9d7bc6dfc76cc9ec0cd6edb6a in the range, which also makes some changes in this area (although not as obviously related to this precise problem), so I will check whether that is the cause.
The cause is within https://chromium.googlesource.com/chromium/blink/+/ea0529c82496b6f9d7bc6dfc76cc9ec0cd6edb6a. I will investigate further.
Status: Started
Cc: infe...@chromium.org
It looks as if my CL was only a secondary cause. The relevant effect of my CL is that it means a video elements always have a shadow DOM containing a hidden element with style -webkit-media-controls, even if they don't have video controls. If I modify the test case slightly, by adding a "controls" attribute to the video element, I get exactly the same crash without either of my CLs. 

The test case that shows this is:
<video controls><style>
*::-webkit-media-controls{display:inline;

(note that 'vertical-align:sub' isn't needed).

Fixing that crash will almost certainly fix the ClusterFuzz crash. 
Cc: -infe...@chromium.org aber...@chromium.org
Owner: infe...@chromium.org
Status: Assigned
Assigning back to inferno@ for re-assignment. While my CL exposed this crash in some new circumstances it is not the fundamental cause of the bug. I don't know the render code, and can't search security bugs to find out if this is already known.
Owner: acolwell@chromium.org
Aaron, this looks media element related. can you please take a look or help with an owner.
Note that if you want to test this without my CLs you will also need to revert the Chromium CL https://codereview.chromium.org/291163004/, otherwise Chrome won't build.
Owner: scherkus@chromium.org
Assigning to scherkus@ since today is my last day on the team and I'm not likely to get to this today.
Project Member

Comment 14 by ClusterFuzz, Sep 22 2014

Summary: Bad-cast to blink::RenderInline from blink::RenderVideo;RenderInline.h:190:1 (was: ASSERTION FAILED: !object || (object->isRenderInline()), UNKNOWN in blink::RenderInline::splitInlines)
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6065899841781760

Fuzzer: Bj_broddelwerk
Job Type: Linux_ubsan_vptr_chrome

Crash Type: Bad-cast
Crash Address: 0x31b6fee28000
Crash State:
  Bad-cast to blink::RenderInline from blink::RenderVideo
  RenderInline.h:190:1
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_vptr_chrome&range=295079:295250

Minimized Testcase (0.08 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv96ud8eB1GDfvpsJCtwYCTvhWbty-BDpazg1gXHqAL05_YHKNT4yRW06637oTGgCIQOG0mJWEMzRGyqaJY_JY5LnCkTA-3Xz8y0VFGt8CpXFGpDKPG3zp59z8QF1g2gOI4T0LJBetOvtPWt6dGGJLtRAZboGdA
<video>><style>
*::-webkit-media-controls{text-transform:capitalize;display:initial;


Filer: inferno
Cc: dsinclair@chromium.org scherkus@chromium.org
Owner: jchaffraix@chromium.org
Looking at more repros, this looks like regression from https://chromium.googlesource.com/chromium/blink/+/2e9adcfad90d77e48174b9dab77ff5a7d129d3bf. Julien, can you please take a look.
jchaffraix@, what is the status here?  this is a beta blocker for M39, so it'll need to be closed shortly.
 Issue 415348  has been merged into this issue.
Cc: jchaffraix@chromium.org timloh@chromium.org
Owner: igsolla@chromium.org
Actually this bug and other one 415348 look specific to -webkit-media-controls and this one looks related.

8a6d9e9  Hide video controls after touch from hideMediaControlsTimerFired. by igsolla@chromium.org - 5 weeks ago
 Issue 415348  has been merged into this issue.
Project Member

Comment 20 by ClusterFuzz, Sep 28 2014

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

Fuzzer: Bj_broddelwerk
Job Type: Linux_asan_chrome_mp

Crash Type: UNKNOWN
Crash Address: 0x00009f7537dd
Crash State:
  blink::RenderInline::splitInlines
  blink::RenderInline::splitFlow
  blink::RenderInline::addChildIgnoringContinuation
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=295079:295250

Minimized Testcase (0.07 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv97kl5h8ccK0klUdey4w2JT7fkcJ0ZGNQ9IMxqsCAdOAQrzziOj1JO4B3tO8ctqgwUrMwL40L37GZeAYntmmHXNZZliWBezwLmOHuXBBwXZLOE0tZ4dI-vSbXDkXVuk9ukZscMwIT2RehsdRlheZ08vijLvh_Q
<style>
*::-webkit-media-controls{display:initial;></style>
<video>>


Filer: inferno
Project Member

Comment 21 by ClusterFuzz, Sep 29 2014

Labels: -Security_Impact-Head Security_Impact-Beta
My changes are unrelated. The crash happens when loading the page, and my changes only affect the visibility of controls after waiting for 3 seconds after a tap.

I have confirmed that the crash still happens after reverting both:
https://codereview.chromium.org/441193003/ and
https://codereview.chromium.org/453493002/

Per #6 in crbug/415348, it looks like aberent's change has exposed a bug that had remained unnoticed until then, but we don't know since when. It could have been there for a very long time, so looking at recent changes to find the culprit does not make a lot of sense. Somebody familiar with this code should debug aberent's test failure to find the cause.

scherkus@: are you the right person? Otherwise please assign to inferno for re-assignment.

Cc: igsolla@chromium.org
Owner: scherkus@chromium.org
Project Member

Comment 24 by ClusterFuzz, Sep 29 2014

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Owner: infe...@chromium.org
reassigning to inferno@ as acolwell@ dumped this on me but I have no idea what's going on here
Project Member

Comment 26 by ClusterFuzz, Oct 7 2014

Labels: Nag
inferno@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 27 by ClusterFuzz, Oct 7 2014

ClusterFuzz is analyzing your testcase. Chromium developers can follow the progress at https://cluster-fuzz.appspot.com/testcase?key=5667893525086208
Project Member

Comment 28 by ClusterFuzz, Oct 7 2014

Summary: ASSERTION FAILED: curr->isRenderInline(), UNKNOWN in blink::RenderInline::splitInlines (was: Bad-cast to blink::RenderInline from blink::RenderVideo;RenderInline.h:190:1)
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=5667893525086208

Uploader: aarya@google.com
Job Type: Linux_asan_chrome_mp

Crash Type: UNKNOWN
Crash Address: 0x00009f7537dd
Crash State:
  blink::RenderInline::splitInlines
  blink::RenderInline::splitFlow
  blink::RenderInline::addChildIgnoringContinuation
  
Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_asan_chrome_mp&range=295079:295250

Minimized Testcase (0.05 Kb):
Download: https://cluster-fuzz.appspot.com/download/AMIfv974VFviPq7UkpDaxyKNLQ-yXkpUG1f1_tfBDScKK4-G2HaMPVcSgtu84VbJHzLMgSw21-TAKaLy8erWorKVuSiHrqh2iIwq6_RM-pR4x23t49dG-2BKBGOrelvXzyyMWN204HyNKHjSRS8MblP7gdYDMptvEg
<video><style>
*::-webkit-media-controls{display:inline;



Owner: shinyak@chromium.org
This looks like a shadow dom issue, assigning to Shinyak@.
Project Member

Comment 30 by ClusterFuzz, Oct 19 2014

shinyak@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Cc: shinyak@chromium.org
Owner: hayato@chromium.org
Ohya, I didn't notice this.

Currently I'm not working for blink itself, I reassign this to hayato@.

Cc: hayato@chromium.org
Owner: infe...@chromium.org
I regret to say that I'm not familiar with RenderInline nor the implementation of the <video> element. I have no idea what is happening here.

I could reproduce this, however, it's hard for me to judge whether this issue is related to Shadow DOM in general or specific to the implementation of <video> element. The render tree for <video> element looks complex to me...

I would like to hear the opinion from <video> element's implementor at first.

reassigning to inferno@, could you assign this to the owner of the <video> element at first?

Owner: vcarbune@chromium.org
vcarbune@, can you please help to take a look at this -webkit-media-controls issue
Owner: phil...@opera.com
Victor replied over email that he is no longer working in this area and philipj@opera is a good contact.

Comment 35 by phil...@opera.com, Oct 22 2014

The problem is that page author style can override the user agent style. Adding !important everywhere in mediaControls.css would get rid of this assert and similar ones, but really it shouldn't be possible for pages to interfere at all.

The solution should be to use ::-internal-media-controls instead.
This is a security bug, an attacker can pick which ever css property is avaliable on the webpage. do you mean -webkit-media-controls shouldn't be exposed to the web ?

Comment 37 by phil...@opera.com, Oct 22 2014

Yes exactly, I'm looking into using psuedo element names like "-internal-media-controls" and "-internal-media-controls-overlay-play-button" instead and making sure that these are only recognized when parsing user agent style sheets. This is already done for CSS property names.

Comment 38 by phil...@opera.com, Oct 22 2014

Something like https://codereview.chromium.org/662243003 but that's still missing the crucial part of only allowing ::-internal-* in user agent style sheets.

Comment 39 by phil...@opera.com, Oct 24 2014

Status update:

There's a bit of a problem with my proposed fix:
https://codereview.chromium.org/662243003#msg8

I don't think what is described in http://css-tricks.com/custom-controls-in-html5-video-full-screen/ should be supported, neither ::-webkit-media-controls{display:none;} should hide native controls nor should z-index be able to position anything above the fullscreen element. One should put the videos parent element in fullscreen instead.

Fixing this by unexposing ::-webkit-media-controls is not without risk. I'm going to add an API OWNER to the discussion.

Comment 40 by phil...@opera.com, Oct 28 2014

Cc: dglazkov@chromium.org

Comment 41 by phil...@opera.com, Oct 28 2014

I posted to blink-dev about getting rid of ::-webkit-media-controls*:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/YCIaYPa_DhI/RyDlcHmD1_wJ

The initial reactions were not positive.

Comment 42 by phil...@opera.com, Oct 28 2014

jchaffraix@, maybe you can help shed some light on a C++ fix?

I've very little experience in layout/rendering, but with trial-and-error I've been able to generalize the assertion and move it up a few levels in the stack:
https://codereview.chromium.org/687643002/

This assert is never hit in existing LayoutTests (as shown by the bots) but it is hit by the test case in https://codereview.chromium.org/662243003/

So what does this mean? Are nested RenderInlines never supposed to happen? One thing that's unusual about the test case that I don't think can ever happen otherwise is that we have an inline as a child of replaced content (video). I guess normally if you have inline children of <img> or similar it would just never be rendered, but the media controls are special.
Cc: esprehn@chromium.org

Comment 44 by phil...@opera.com, Oct 29 2014

Cc: msten...@opera.com r...@opera.com
Project Member

Comment 45 by bugdroid1@chromium.org, Oct 30 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=184652

------------------------------------------------------------------
r184652 | philipj@opera.com | 2014-10-30T14:38:00.770030Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html?r1=184652&r2=184651&pathrev=184652
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/webkit-media-controls-webkit-appearance.html?r1=184652&r2=184651&pathrev=184652
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderMedia.cpp?r1=184652&r2=184651&pathrev=184652
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/webkit-media-controls-display-expected.html?r1=184652&r2=184651&pathrev=184652
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/media/webkit-media-controls-display.html?r1=184652&r2=184651&pathrev=184652
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/rendering/RenderMedia.h?r1=184652&r2=184651&pathrev=184652

Disallow non-flexbox child renderers for RenderMedia

Inline children of replaced content is not handled and triggered the
ASSERT(curr->isRenderInline()) UNKNOWN in RenderInline::splitInlines().

The render tree at the point of failure was:

RenderView 0xd0c23404010                #document 0x2be215c04b48
  RenderBlock 0xd0c2341c010             HTML 0x2be215c101a8
    RenderBody 0xd0c2341c110            BODY 0x2be215c102b8
      RenderBlock (anonymous) 0xd0c2341c310
        RenderVideo 0xd0c23420010       VIDEO 0x2be215c48010
*         RenderInline (relative positioned) 0xd0c23424010 DIV 0x2be215c68010
      RenderBlock (anonymous) 0xd0c2341c210
      RenderBlock (anonymous) 0xd0c2341c410

Making ::-webkit-media-controls* internal would have fixed this:
https://codereview.chromium.org/662243003/
https://groups.google.com/a/chromium.org/d/msg/blink-dev/YCIaYPa_DhI/RyDlcHmD1_wJ

Unfortunately, that is not without risk. This fix allows
::-webkit-media-controls { display: none; } to keep working until the
usage can be measured.

BUG= 415407 ,  426759 

Review URL: https://codereview.chromium.org/689613002
-----------------------------------------------------------------
Status: Fixed
Project Member

Comment 47 by ClusterFuzz, Oct 30 2014

Labels: -Restrict-View-SecurityTeam Merge-Triage Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz

Comment 48 by phil...@opera.com, Nov 8 2014

Labels: -Merge-Triage Merge-Requested
Requesting merge to M39, this reached the dev channel with 40.0.2209.0 on November 4.
Labels: -Merge-Requested Merge-Approved
merge approved for m39 branch 2171

Comment 50 by phil...@opera.com, Nov 11 2014

Labels: -Merge-Approved Merge-Merged
Merged to M39.

Comment 51 by phil...@opera.com, Nov 11 2014

Labels: -Merge-Merged Merge-Requested M-38
Requesting merge to M38, as the minimized test case will crash M38 too if the controls attribute is added. The bug was found after https://codereview.chromium.org/291163004 (not in M38) but the root cause is older.
Project Member

Comment 52 by ClusterFuzz, Nov 11 2014

Labels: -Security_Impact-Beta -M-38 Security_Impact-Stable
Project Member

Comment 53 by bugdroid1@chromium.org, Nov 11 2014

Labels: merge-merged-2171
The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=185110

------------------------------------------------------------------
r185110 | philipj@opera.com | 2014-11-11T08:36:53.914009Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/media/webkit-media-controls-display-expected.html?r1=185110&r2=185109&pathrev=185110
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/media/webkit-media-controls-display.html?r1=185110&r2=185109&pathrev=185110
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/Source/core/rendering/RenderMedia.h?r1=185110&r2=185109&pathrev=185110
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/media/webkit-media-controls-webkit-appearance-expected.html?r1=185110&r2=185109&pathrev=185110
   A http://src.chromium.org/viewvc/blink/branches/chromium/2171/LayoutTests/media/webkit-media-controls-webkit-appearance.html?r1=185110&r2=185109&pathrev=185110
   M http://src.chromium.org/viewvc/blink/branches/chromium/2171/Source/core/rendering/RenderMedia.cpp?r1=185110&r2=185109&pathrev=185110

Merge 184652 "Disallow non-flexbox child renderers for RenderMedia"

> Disallow non-flexbox child renderers for RenderMedia
> 
> Inline children of replaced content is not handled and triggered the
> ASSERT(curr->isRenderInline()) UNKNOWN in RenderInline::splitInlines().
> 
> The render tree at the point of failure was:
> 
> RenderView 0xd0c23404010                #document 0x2be215c04b48
>   RenderBlock 0xd0c2341c010             HTML 0x2be215c101a8
>     RenderBody 0xd0c2341c110            BODY 0x2be215c102b8
>       RenderBlock (anonymous) 0xd0c2341c310
>         RenderVideo 0xd0c23420010       VIDEO 0x2be215c48010
> *         RenderInline (relative positioned) 0xd0c23424010 DIV 0x2be215c68010
>       RenderBlock (anonymous) 0xd0c2341c210
>       RenderBlock (anonymous) 0xd0c2341c410
> 
> Making ::-webkit-media-controls* internal would have fixed this:
> https://codereview.chromium.org/662243003/
> https://groups.google.com/a/chromium.org/d/msg/blink-dev/YCIaYPa_DhI/RyDlcHmD1_wJ
> 
> Unfortunately, that is not without risk. This fix allows
> ::-webkit-media-controls { display: none; } to keep working until the
> usage can be measured.
> 
> BUG= 415407 ,  426759 
> 
> Review URL: https://codereview.chromium.org/689613002

TBR=philipj@opera.com

Review URL: https://codereview.chromium.org/716733002
-----------------------------------------------------------------
Labels: m39-ignore
Adding M39 ignore so this doesn't continue to show in my merge request queries for M39.  Note that the odds of M38 taking this are very low given where they are in the release cycle.

Comment 55 by phil...@opera.com, Nov 11 2014

OK, I took comment #47 to mean that I should request merge for all channels in turn, and M39 could be two weeks away perhaps. How would you usually decide whether or not to request merge to the current stable channel?
In general we would want to merge fixes for high severity security bugs back if it didn't seem too risky to do so, but we probably won't have another patch with security fixes for M38 given how close M39 is. There's no need to worry about merging this back to M38.

Comment 57 by phil...@opera.com, Nov 11 2014

OK, thanks for explaining!
Labels: -Merge-Requested -m39-ignore Release-0-M39
Labels: -Cr-Blink-Rendering Cr-Blink-Layout
Migrate from Cr-Blink-Rendering to Cr-Blink-Layout
Project Member

Comment 60 by ClusterFuzz, Feb 5 2015

Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member

Comment 61 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 62 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic

Sign in to add a comment