ZDI-CAN-3852: Apple Safari HTMLVideoElement Use-After-Free Remote Code Execution Vulnerability

• Component: Media
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

ZDI-CAN-3852: Apple Safari HTMLVideoElement Use-After-Free Remote Code Execution Vulnerability

[David Kilzer (:ddkilzer)]

<rdar://problem/27313234>

[Eric Carlson]

Created attachment 285151 [details]
Proposed patch.

[David Kilzer (:ddkilzer)]

Comment on attachment 285151 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=285151&action=review

r=me

> Source/WebCore/html/track/AudioTrack.cpp:170
> +    ASSERT(element());

Did you mean to do this instead?  Maybe it doesn't matter.

    ASSERT(mediaElement());

> LayoutTests/ChangeLog:4
> +        Need a short description (OOPS!).
> +        Need the bug URL (OOPS!).

Oops!  Copy and paste title and URL from Source/WebCore/ChangeLog.

[Eric Carlson]

Comment on attachment 285151 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=285151&action=review

>> Source/WebCore/html/track/AudioTrack.cpp:170
>> +    ASSERT(element());
> 
> Did you mean to do this instead?  Maybe it doesn't matter.
> 
>     ASSERT(mediaElement());

They are functionally equivalent, but your suggestion is semantically correct. Fixed.

>> LayoutTests/ChangeLog:4
>> +        Need the bug URL (OOPS!).
> 
> Oops!  Copy and paste title and URL from Source/WebCore/ChangeLog.

OOPS! Fixed.

[Eric Carlson]

Committed r204050: https://trac.webkit.org/r204050

[Darin Adler]

There is a mistake in this patch.

The call to clearElement in TrackListBase::~TrackListBase will not call TextTrackList::clearElement, because as part of the destruction process we have already turned the TextTrackList into a TrackListBase.

[Darin Adler]

Comment on attachment 285151 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=285151&action=review

> Source/WebCore/html/track/AudioTrack.cpp:172
> +    if (mediaElement())
> +        mediaElement()->removeAudioTrack(*this);

I do not understand why we are adding this null check as part of this patch. I don’t see the relevance.

If the media element can be null, then the assertion is incorrect. If it can’t, then it seems peculiar to add the code as part of this security fix that we would like to keep as simple and minimal as possible.

> Source/WebCore/html/track/TrackListBase.cpp:49
> +    clearElement();

This will not call TextTrackList::clearElement. We will have to do something in ~TextTrackList if we want to be sure that code runs when destroying the text track list because by the time this destructor is called, the virtual table will be the one from TrackListBase.

[Eric Carlson]

Comment on attachment 285151 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=285151&action=review

>> Source/WebCore/html/track/AudioTrack.cpp:172
>> +        mediaElement()->removeAudioTrack(*this);
> 
> I do not understand why we are adding this null check as part of this patch. I don’t see the relevance.
> 
> If the media element can be null, then the assertion is incorrect. If it can’t, then it seems peculiar to add the code as part of this security fix that we would like to keep as simple and minimal as possible.

Agreed, it isn't necessary.

>> Source/WebCore/html/track/TrackListBase.cpp:49
>> +    clearElement();
> 
> This will not call TextTrackList::clearElement. We will have to do something in ~TextTrackList if we want to be sure that code runs when destroying the text track list because by the time this destructor is called, the virtual table will be the one from TrackListBase.

Fixed.

[Eric Carlson]

Created attachment 285231 [details]
Updated Patch.

Add

[Brent Fulgham]

Comment on attachment 285231 [details]
Updated Patch.

Looks good! r=me.

[Eric Carlson]

Committed r204089: https://trac.webkit.org/r204089