ZDI-CAN-3852: Apple Safari HTMLVideoElement Use-After-Free Remote Code Execution Vulnerability
<rdar://problem/27313234>
Created attachment 285151 [details] Proposed patch.
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.
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.
Committed r204050: https://trac.webkit.org/r204050
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.
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.
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.
Created attachment 285231 [details] Updated Patch. Add
Comment on attachment 285231 [details] Updated Patch. Looks good! r=me.
Committed r204089: https://trac.webkit.org/r204089