xhwang: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

I'll take a look.

It seems we are still trying to use m_webAudioSourceProvider after it's freed (as part of WMPI dtor):

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?rcl=1479169028&l=4105

https://codereview.chromium.org/1522463003/ seems to be related. But from code inspection I don't see anything obvious.

I downloaded the binary and can repro when serving the test case through file:///. But it doesn't repro when I serve the test case through http://. Since the issue is "loadResource" related. This actually makes some sense.

I'll look more into this.

I can repro this in a debug build now :)

This can be repro'ed by:
- set SRC url
- internals.setMediaElementNetworkState(audio, 0); // NETWORK_EMPTY
- play()

During play() (playInternal()), since network state is EMPTY, we'll call invokeResourceSelectionAlgorithm() [0].

Then invokeResourceSelectionAlgorithm() will call scheduleNextSourceChild(), which will schedule the loaderTimer and finally trigger this callstack:

    #1 0xbb20e63 in blink::HTMLMediaElement::startPlayerLoad(blink::KURL const&) 
    #2 0xbb1ad15 in blink::HTMLMediaElement::loadResource(blink::WebMediaPlayerSource const&, blink::ContentType const&) 
    #3 0xbb19739 in blink::HTMLMediaElement::loadSourceFromAttribute() 
    #4 0xbb185b3 in blink::HTMLMediaElement::selectMediaResource() 
    #5 0xbb14c2c in blink::HTMLMediaElement::loadInternal()
    #6 0xbb0bb15 in blink::HTMLMediaElement::loadTimerFired(blink::TimerBase*) 

In startPlayerLoad() we'll reset m_webMediaPlayer [1], which is causing the use-after-free.

Clearly, startPlayerLoad() expects m_webMediaPlayer to be null at the beginning of the function [2]. So it seems to me the caller of invokeResourceSelectionAlgorithm() should make sure we have cleared/reset m_webMediaPlayer. This is true for invokeLoadAlgorithm(), but not the case for other callers like playInternal() and pauseInternal().

On the other hand, from the spec, NETWORK_EMPTY means "the element has not yet been initialized. All attributes are in their initial states". So it seems to me when network state is NETWORK_EMPTY, we should be able to dcheck m_webMediaPlayer is null. With that, it seems to me internals.setMediaElementNetworkState(audio, 0) should reset m_webMediaPlayer and other states. Does this make sense?

dalecurtis / foolip: WDYT?


[0] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?rcl=1479284732&l=2286
[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?rcl=1479284732&l=1173
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp?rcl=1479284732&l=1135

This sounds similar to the "FIXME: remove m_webMediaPlayer check once we figure out how m_webMediaPlayer is going out of sync with readystate" that we have in a few places, but instead for networkState.

Just based on what makes sense, I think that NETWORK_EMPTY should imply !m_webMediaPlayer, but the reverse need not be true, as one goes to NETWORK_NO_SOURCE before actually having a source to load.

As implemented, I'd expect both DCHECK(!m_webMediaPlayer) and DCHECK_EQ(m_networkState, kNetworkEmpty) to hold at the top of HTMLMediaElement::invokeResourceSelectionAlgorithm. If either can be made to fail without the use of internals, that sounds like something to fix.

Note: looking at "invoke the media element's resource selection algorithm" calls in https://html.spec.whatwg.org/multipage/embedded-content.html there is one case where the networkState assert wouldn't hold, but HTMLMediaElement::invokeLoadAlgorithm resets networkState where the spec doesn't, because the resource selection algorithm will change it again as its first step.

Thanks!

By looking at our code (well, from code search), Chromium will never set networkState to NetworkStateEmpty. So it seems to me this issue will not happen in production. I wonder whether this will change the priority and/or the security severity of this issue.

I think it makes a lot sense to reset m_webMediaPlayer when networkState becomes NetworkStateEmpty. This is consistent with the spec, and makes things much easier to reason about.

I'd like to keep this change simple, so that if needed, we can merge it back. Long term, after the FIXME is fixed, hopefully things will be in a much better shape.

I tried to clearMediaPlayer() when we set networkState to NETWORK_EMPTY. But it's causing other issues :(

There are multiple issues here.

# For the spec:

NETWORK_EMPTY (numeric value 0):
The element has not yet been initialized. All attributes are in their initial states.

play(): 
1. If the media element's networkState attribute has the value NETWORK_EMPTY, invoke the media element's resource selection algorithm.
2. If the playback has ended...

The question is, after step 1, why don't we return immediately? If "the element was not initialized and all attibutes are in their initial states", why do we care about "if the playback has ended"?

# For the implementation:

In both playInternal() and pauseInternal(), if networkState is NETWORK_EMPTY, we call invokeResourceSelectionAlgorithm() and continue. This maps the spec text. But then we call updatePlayState(), which is causing trouble.

updatePlayState() has logic like this:

void HTMLMediaElement::updatePlayState() {
  bool isPlaying = webMediaPlayer() && !webMediaPlayer()->paused();
  bool shouldBePlaying = potentiallyPlaying();

  if (shouldBePlaying) {
    if (!isPlaying) {
      webMediaPlayer()->setRate(playbackRate());
      ...
    }
  }

When webMediaPlayer() is null, isPlayering is false. But potentiallyPlaying() doesn't check webMediaPlayer(). So we could end up calling webMediaPlayer()->setRate() when webMediaPlayer() is null...

Does it make sense to check webMediaPlayer() in potentiallyPlaying()?

Personally I feel we need a lot of DCHECKs in HTMLMediaElement to make sure our design assumptions are legit and enforced.

foolip/dalecurtis: Please see the discussion above. Right now I don't have any good idea how to fix this given the complexity involved :)

Since this can only be triggered through "internals", I feel this should not be Security_Impact-Stable.

Also, does it make sense to disallow setting network state to EMPTY through "internals"? This is breaking a lot of assumptions we have in the code.

#15, in https://html.spec.whatwg.org/multipage/embedded-content.html#dom-media-play there isn't a return after "invoke the media element's resource selection algorithm" because the steps that follow (setting paused to false) will cause the media to eventually play, even though at the time the network request has (at best) just started.

When things blow up in HTMLMediaElement::updatePlayState(), why did potentiallyPlaying() return true? Was pausedToBuffer true? And what was readyState? Maybe  m_readyStateMaximum needs to be reset in some case where m_readyState is set back to 0?

foolip: I am really not familiar with the interaction of the states in HTMLMediaElement. Instead of trying to understand more how things work in this file, I feel I should reassign this issue to a better owner of this file. Do you mind taking this bug over? Or who do you think will be the best owner?

I volunteer mlamouri@, please reassign if that won't work.

mlamouri: Uh oh! This issue still open and hasn't been updated in the last 44 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

We commit ourselves to a 60 day deadline for fixing for high severity vulnerabilities, and have exceeded it here. If you're unable to look into this soon, could you please find another owner or remove yourself so that this gets back into the security triage queue?

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

Ping - is there any progress on resolving this issue?

Moving this to "Security_Impact-None" as it can only apply with the use of `internals`.

ClusterFuzz has detected this issue as fixed in range 471033:471049.

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

Fuzzer: inferno_twister
Job Type: linux_asan_content_shell_drt
Platform Id: linux

Crash Type: Heap-use-after-free READ 8
Crash Address: 0x60d00007d310
Crash State:
  blink::HTMLMediaElement::startPlayerLoad
  blink::HTMLMediaElement::loadResource
  blink::HTMLMediaElement::loadSourceFromAttribute
  
Sanitizer: address (ASAN)

Recommended Security Severity: High

Regressed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=381067:381276
Fixed: https://clusterfuzz.com/revisions?job=linux_asan_content_shell_drt&range=471033:471049

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


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.

ClusterFuzz testcase 5300755356712960 is verified as fixed, so closing issue.

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

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