1198435 - (CVE-2015-4509) HTMLVideoElement Use-After-Free Remote Code Execution (ZDI-CAN-3176)

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P1)

Description

[Daniel Veditz [:dveditz]]

Attachment: ZDI-CAN-3176.zip (pw: ZDI-CAN-3176)

ZDI sent this vulnerability to our security reporting address. Have not yet confirmed it (need to fire up the windows machine) bug eip=41414141 is a pretty reliable sign of exploitability.

-- VULNERABILITY DETAILS ------------------------

Tested against Firefox 40.0.2 and a nightly build from August 17th on Windows 8.1

Crash info against the release build:

```
[JavaScript Error: "Win error 2 during operation open on file C:\Users\ZDI\AppData\Local\Mozilla\Firefox\Profiles\sg8qwckf.default\directoryLinks.json (The system cannot find the file specified.
)" {file: "resource:///modules/DirectoryLinksProvider.jsm" line: 558}]
[JavaScript Warning: "Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://snippets.cdn.mozilla.net/4/Firefox/40.0.2/20150812163655/WINNT_x86-msvc/en-US/release/Windows_NT%206.3/default/default/. (Reason: CORS request failed)."]
[JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 228}]
[JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 278}]
[JavaScript Warning: "An IndexedDB transaction that was not yet complete has been aborted due to page navigation." {file: "chrome://browser/content/abouthome/aboutHome.js" line: 289}]
1439870619576   Browser.SelfSupportBackend  WARN    onLocationChange - There was a problem fetching the SelfSupport URL (attempt 0).
[JavaScript Warning: "Key event not available on some keyboard layouts: key="r" modifiers="accel,alt"" {file: "chrome://browser/content/browser.xul" line: 0}]
[JavaScript Warning: "Key event not available on some keyboard layouts: key="c" modifiers="accel,alt"" {file: "chrome://browser/content/browser.xul" line: 0}]
[JavaScript Warning: "Key event not available on some keyboard layouts: key="i" modifiers="accel,alt,shift"" {file: "chrome://browser/content/browser.xul" line: 0}]
[JavaScript Error: "TelemetryStopwatch: key "PLACES_AUTOCOMPLETE_URLINLINE_DOMAIN_QUERY_TIME_MS" was already initialized" {file: "resource://gre/modules/TelemetryStopwatch.jsm" line: 52}]
[JavaScript Warning: "<source> element has no "src" attribute. Media resource load failed." {file: "http://172.16.208.3/PoC-EIP-0x41414141/poc.html" line: 0}]
[JavaScript Warning: "All candidate resources failed to load. Media load paused." {file: "http://172.16.208.3/PoC-EIP-0x41414141/poc.html" line: 0}]
[JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://172.16.208.3/PoC-EIP-0x41414141/poc.html" line: 0}]
[JavaScript Warning: "HTTP load failed with status 404. Load of media resource http://172.16.208.3/PoC-EIP-0x41414141/nothing failed." {file: "http://172.16.208.3/PoC-EIP-0x41414141/poc.html" line: 0}]
[JavaScript Warning: "All candidate resources failed to load. Media load paused." {file: "http://172.16.208.3/PoC-EIP-0x41414141/poc.html" line: 0}]
1439870649798   Browser.SelfSupportBackend  WARN    onLocationChange - There was a problem fetching the SelfSupport URL (attempt 1).
(b38.fcc): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0c1626d0 ebx=11a1f400 ecx=41414141 edx=41414141 esi=00000000 edi=0078eec4
eip=41414141 esp=0078ee98 ebp=00000008 iopl=0         nv up ei ng nz ac pe cy
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00210297
41414141 41              inc     ecx
0:000> kv
ChildEBP RetAddr  Args to Child              
WARNING: Frame IP not in any known module. Following frames may be wrong.
0078ee94 5cc0453f 41414141 0c164ca0 0078eec4 0x41414141
0078eebc 5cc03d6e 0befa8a0 12530ccc 0c1651a0 xul!mozilla::dom::HTMLMediaElement::LookupMediaElementURITable+0x8e (FPO: [1,2,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 2034]
0078efb0 5cc03b8d 00000000 12530c00 02142cf0 xul!mozilla::dom::HTMLMediaElement::LoadResource+0x11f (FPO: [0,55,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 1190]
0078f2b8 5cc09ebd 02142cf0 0be18c40 12530c00 xul!mozilla::dom::HTMLMediaElement::LoadFromSourceChildren+0x27b (FPO: [Non-Fpo]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 1028]
0078f3cc 5cc09edf 0a8abd00 5bc7d922 5cc096f3 xul!mozilla::dom::HTMLMediaElement::SelectResource+0x197 (FPO: [Non-Fpo]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 903]
0078f3d4 5bc7d922 5cc096f3 0a8abc40 00000010 xul!mozilla::dom::HTMLMediaElement::SelectResourceWrapper+0x8 (FPO: [0,0,4]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 841]
0078f3d8 5cc096f3 0a8abc40 00000010 5bda58b8 xul!nsRunnableMethodImpl<void (__thiscall mozilla::dom::quota::QuotaManager::*)(void),1>::Run+0x15 (FPO: [1,0,0]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\dist\include\nsthreadutils.h @ 811]
0078f3e4 5bda58b8 0a8abd00 021411d0 02142cf4 xul!mozilla::dom::nsSyncSection::Run+0x19 (FPO: [1,0,4]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\dom\html\htmlmediaelement.cpp @ 764]
0078f400 5bda584e 00000001 00000000 5d6814e0 xul!nsBaseAppShell::RunSyncSectionsInternal+0x68 (FPO: [2,1,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\widget\nsbaseappshell.cpp @ 370]
0078f410 5bda4ee6 02142cf4 021411d0 00000000 xul!nsBaseAppShell::AfterProcessNextEvent+0x21 (FPO: [4,1,0]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\widget\nsbaseappshell.cpp @ 434]
0078f534 5bfcceac 021411d0 00000001 0078f54f xul!nsThread::ProcessNextEvent+0x6f6 (FPO: [Non-Fpo]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\xpcom\threads\nsthread.cpp @ 904]
0078f550 5bda848d 02161300 02161300 0078f5ac xul!NS_ProcessNextEvent+0x1a (FPO: [Non-Fpo]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\xpcom\glue\nsthreadutils.cpp @ 265]
0078f574 5bda75b5 02161300 3a3eb85d 02142cf0 xul!mozilla::ipc::MessagePump::Run+0xb7 (FPO: [1,4,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\glue\messagepump.cpp @ 128]
0078f5ac 5bda7747 021411d0 00000001 5bf32b00 xul!MessageLoop::RunHandler+0x20 (FPO: [SEH]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_loop.cc @ 227]
0078f5cc 5bda5bfb 0a8488c0 00000000 5bda677e xul!MessageLoop::Run+0x19 (FPO: [Non-Fpo]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\chromium\src\base\message_loop.cc @ 201]
0078f5d8 5bda677e 02142cf0 0a8488c0 5bdcf0af xul!nsBaseAppShell::Run+0x32 (FPO: [1,0,0]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\widget\nsbaseappshell.cpp @ 167]
0078f5e4 5bdcf0af 02142cf0 0078f85d 0fa26260 xul!nsAppShell::Run+0x1b (FPO: [1,0,4]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\widget\windows\nsappshell.cpp @ 178]
0078f5f4 5c0118fc 0a8488c0 0078f764 0078f780 xul!nsAppStartup::Run+0x20 (FPO: [1,0,0]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\components\startup\nsappstartup.cpp @ 281]
0078f6d0 5c0127ee 00000001 0078f8a8 0211c160 xul!XREMain::XRE_mainRun+0x499 (FPO: [Non-Fpo]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nsapprunner.cpp @ 4079]
0078f6ec 5c137e6c 00000000 018a5ff8 0078f800 xul!XREMain::XRE_main+0x1b2 (FPO: [3,2,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nsapprunner.cpp @ 4170]
0078f860 000a1635 00000001 018a5ff8 0078f8a8 xul!XRE_main+0x3e (FPO: [4,87,0]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nsapprunner.cpp @ 4260]
0078f9f4 000a12dc 0211c160 018a9ff8 ffffc000 firefox!do_main+0x125 (FPO: [1,92,0]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\browser\app\nsbrowserapp.cpp @ 214]
0078fa88 000a10dc 000a2521 000a2521 0078fae4 firefox!NS_internal_main+0xec (FPO: [Non-Fpo]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\browser\app\nsbrowserapp.cpp @ 480]
0078fa9c 000a24a4 018a5ff8 0184ff98 01851f70 firefox!wmain+0xbc (FPO: [2,0,0]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nswindowswmain.cpp @ 138]
0078fae4 758b4198 7f7df000 758b4170 1a81bdec firefox!__tmainCRTStartup+0xfe (FPO: [Non-Fpo]) (CONV: cdecl) [f:\dd\vctools\crt\crtw32\startup\crt0.c @ 255]
0078faf8 779132d1 7f7df000 9a3e1a74 00000000 KERNEL32!BaseThreadInitThunk+0x24 (FPO: [Non-Fpo])
0078fb40 7791329f ffffffff 7793f054 00000000 ntdll!__RtlUserThreadStart+0x2b (FPO: [SEH])
0078fb50 00000000 000a2521 7f7df000 00000000 ntdll!_RtlUserThreadStart+0x1b (FPO: [Non-Fpo])
0:000> !lmi xul
Loaded Module Info: [xul]
         Module: xul
   Base Address: 5bb60000
     Image Name: C:\Program Files\Mozilla Firefox\xul.dll
   Machine Type: 332 (I386)
     Time Stamp: 55cc02db Wed Aug 12 19:37:15 2015
           Size: 2401000
       CheckSum: 2368004
Characteristics: 2122  
Debug Data Dirs: Type  Size     VA  Pointer
             CODEVIEW    76, 1d9ea98, 1d9d498 RSDS - GUID: {B89D751B-2F9D-420E-AAEE-FD53D90CF5BF}
               Age: 2, Pdb: c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\toolkit\library\xul.pdb
                   ??    14, 1d9eb10, 1d9d510 [Data not mapped]
                CLSID     4, 1d9eb24, 1d9d524 [Data not mapped]
     Image Type: FILE     - Image read successfully from debugger.
                 C:\Program Files\Mozilla Firefox\xul.dll
    Symbol Type: PDB      - Symbols loaded successfully from image path.
                 z:\export\symbols\xul.pdb\B89D751B2F9D420EAAEEFD53D90CF5BF2\xul.pdb
       Compiler: Linker - front end [0.0 bld 0] - back end [12.0 bld 30723]
    Load Report: private symbols & lines, source indexed
                 z:\export\symbols\xul.pdb\B89D751B2F9D420EAAEEFD53D90CF5BF2\xul.pdb
0:000> lmvm xul
start    end        module name
5bb60000 5df61000   xul        (private pdb symbols)  z:\export\symbols\xul.pdb\B89D751B2F9D420EAAEEFD53D90CF5BF2\xul.pdb
    Loaded symbol image file: C:\Program Files\Mozilla Firefox\xul.dll
    Image path: C:\Program Files\Mozilla Firefox\xul.dll
    Image name: xul.dll
    Timestamp:        Wed Aug 12 19:37:15 2015 (55CC02DB)
    CheckSum:         02368004
    ImageSize:        02401000
    File version:     40.0.2.5702
    Product version:  40.0.2.5702
    File flags:       0 (Mask 3F)
    File OS:          4 Unknown Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0000.04b0
    CompanyName:      Mozilla Foundation
    ProductName:      Firefox
    InternalName:     Firefox
    OriginalFilename: xul.dll
    ProductVersion:   40.0.2
    FileVersion:      40.0.2
    FileDescription:  40.0.2
    LegalCopyright:   License: MPL 2
    LegalTrademarks:  Mozilla
    Comments:         Mozilla
0:000> vertarget
Windows 8 Version 9600 UP Free x86 compatible
Product: WinNt, suite: SingleUserTS
kernel32.dll version: 6.3.9600.17415 (winblue_r4.141028-1500)
Machine Name:
Debug session time: Mon Aug 17 21:05:17.968 2015 (UTC - 7:00)
System Uptime: 0 days 0:05:00.332
Process Uptime: 0 days 0:02:02.507
  Kernel time: 0 days 0:00:19.718
  User time: 0 days 0:00:02.234
```


-- CREDIT ---------------------------------------

This vulnerability was discovered by:

   Anonymous working with HP's Zero Day Initiative

-- FURTHER DETAILS ------------------------------

If supporting files were contained with this report they are provided within a password protected ZIP file. The password is the ZDI candidate number in the form: ZDI-CAN-XXXX where XXXX is the ID number.

Please confirm receipt of this report. We expect all vendors to remediate ZDI vulnerabilities within 120 days of the reported date. If you are ready to release a patch at any point leading up the the deadline please coordinate with us so that we may release our advisory detailing the issue. If the 120 day deadline is reached and no patch has been made available we will release a limited public advisory with our own mitigations so that the public can protect themselves in the absence of a patch. Please keep us updated regarding the status of this issue and feel free to contact us at any time:

Zero Day Initiative
[e-mail]

Comment 1

[Andrew McCreight [:mccr8]]

padenot, is this something you can look at? Thanks.

Comment 2

[Paul Adenot (:padenot)]

302 to Anthony, HTMLMediaElement is on the stack, he'll know who's best to fix this.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Ralph?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Adding cpearce (though he may be on PTO)

Comment 5

[Ralph Giles (:rillian)]

I could use some help with this one too; my vm seems to be borked. Gerald, please pass this on to Edwin if you get stuck.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

I could reproduce this on Mac. I believe it should in fact affect all platforms.

Debugging shows this sequence of interesting events:
0. HTMLMediaElements 'video' and 'vid' are created.
1. vid.LoadFromSourceChildren() sets vid.mLoadingSrc to "poc.wav"
2. vid.AddMediaElementToURITable() adds a gElementTable entry associating the object pointer (&vid) to "poc.wav"
3. vid.LoadFromSourceChildren() changes vid.mLoadingSrc to "nothing"
4? This step does *not* exist! I.e., the gElementTable is not modified, and therefore "poc.wav" still points to our object, and "nothing" is not added to the list!
Time passes...
5. vid.~HTMLMediaElement() calls vid.RemoveMediaElementFromURITable(), but because vid.mLoadingSrc=="nothing", our entry associated with "poc.wav" is *not* removed from the table!
6. 'video' (which is also pointing to "poc.wav") does a video.LookupMediaElementURITable(), which goes through all elements associated with "poc.wav" and calls the NodePrincipal() method on them, at which point our object 'vid' destroyed at step 5 is used.

I don't know enough to be sure, but I'm thinking that the missing step 4 is important.
Maybe at step 3 when mLoadingSrc is modified, the associated gElementTable entry should at least be removed.
And a catch-all fix would be to just remove *all* entries pointing to the object-to-be-destroyed from the destructor.

I'll prepare a patch soon, with my best attempt.
In the meantime, if someone (with better knowledge of HTMLMediaElement) reading this can provide feedback, please do so.

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Funnily enough, the documentation for RemoveMediaElementFromURITable() reads "Call this before clearing mLoadingSrc"!
So I guess we should enforce that.

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch

This patch simply calls RemoveMediaElementFromURITable() at the two locations where mLoadingSrc is overwritten with a new value.

Some notes:
- I've updated the documentation for RemoveMediaElementFromURITable to clarify that it should be called before any modification of mLoadingSrc (not just when "clearing" it).
- I've removed an assertion in RemoveMediaElementFromURITable, because it is now possible to be in a situation where it is called again when the element has already been removed from the table -- It actually happens when running the ZDI proof-of-concept; but at least it doesn't crash anymore.
- The two other locations where mLoadingSrc is actually cleared (set to nullptr) are preceded by a ShutdownDecoder() when needed, which takes care of calling RemoveMediaElementFromURITable, so we don't need to explicitly add it there.

For the upcoming sec-approval (and assuming r+), I believe this is a safe patch to approve and uplift as far as we want, as all it does is add a safe bit of extra work in two spots where mLoadingSrc is modified.

Do we need an actual test based on the ZDI proof-of-concept? (though if made public, the test would more likely reveal the issue than just this patch.)

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

If this patch is approved, I would suggest opening a separate bug related to the allegedly-missing step 4 in comment 6, as I would think the decoder should probably be shutdown after setting <source> to the non-existent file "nothing"; or maybe mLoadingSrc should revert to the source that the decoder is actually working with; or something else?

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-better-MediaElementTableCount.patch

This is an optional patch that adds a sanity check to the DEBUG-only function MediaElementTableCount().

The current function only returns the number of times an element is associated with a given URI (which should be only 0 or 1).
This patch also verifies that the element is not lurking under other URIs, which would be a bad thing indeed, as evidenced by the current ZDI.

It's not necessary to fix this issue, but would help in finding other situations that could result in similar crashes; hence the feedback request.
Please advise whether you think we should ship this patch here, and/or uplift, or move it to another bug later on.

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-blindly-remove-self-from-URITable.patch

Another patch!
This is more of a shotgun-style fix: RemoveMediaElementFromURITable() removes the element from the table regardless of the expected URI; and AddMediaElementToURITable() does a RemoveMediaElementFromURITable() to ensure we only have at most one pointer to the element.

It has the advantage of potentially catching other situations that could lead to the same crash.

The disadvantage is that such cases would be hidden by this patch, and so would not get caught and fixed; except that the "better MediaElementTableCount" in the previous patch could reveal these cases.
It also adds quite a bit of redundant work during each add&remove. But I think these occurrences are rare enough in real life that it shouldn't matter -- right?


Now that we (maybe) have too many choices:
- I think the 1st patch ("remove...") is better-targeted fix.
- The 3rd patch ("blindly...") is a broader fix and could potentially prevent more issues (but hide them).
- The 2nd patch ("better...") is not a fix, but could help with finding further issues, in combination with the 1st and/or 3rd patch/es.

And as per my comment 9, I still think the actual cause of this issue needs to be investigated.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 8655829 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch

Review of attachment 8655829 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. I think this and the diagnostic patch are a better approach, with a follow-up bug to resolve the missing step 4.

What about HTMLMediaElement::AbortExistingLoads()? It looks like mDecoder can be set there.

Roc, does this seem a good approach to you?

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 8655875 [details] [diff] [review]
1198435-better-MediaElementTableCount.patch

Review of attachment 8655875 [details] [diff] [review]:
-----------------------------------------------------------------

r=me seems like a good idea regardless.

::: dom/html/HTMLMediaElement.cpp
@@ +1969,5 @@
> +  for (auto it = gElementTable->ConstIter(); !it.Done(); it.Next()) {
> +    MediaElementSetForURI* entry = it.Get();
> +    uint32_t count = 0;
> +    for (uint32_t i = 0; i < entry->mElements.Length(); ++i) {
> +      HTMLMediaElement* elem = entry->mElements[i];

I think you can

for (auto elem = entry->mElements.begin(); elem != entry->mElements.end(); ++elem)

@@ +1977,2 @@
>      }
> +    if (URIEquals(aURI, entry->GetKey())) {

I'd avoid unnecessary worry by saying `if (aURI && URIEquals(aURI...))` here, just so people don't have to worry about the call being safe with a nullptr. Or call it aURISafeEquals() maybe.

Comment 14

[Ralph Giles (:rillian)]

Comment on attachment 8655933 [details] [diff] [review]
1198435-blindly-remove-self-from-URITable.patch

Review of attachment 8655933 [details] [diff] [review]:
-----------------------------------------------------------------

Patch does what it says, but as I said before, I think patches 1+2 are a better way to go. This seems like it could be more brittle in the future.

::: dom/html/HTMLMediaElement.cpp
@@ +1990,2 @@
>    if (!gElementTable)
>      return;

Would be good to wrap this return in { } while you're here.

@@ +1990,5 @@
>    if (!gElementTable)
>      return;
> +  for (auto it = gElementTable->Iter(); !it.Done(); it.Next()) {
> +    MediaElementSetForURI* entry = it.Get();
> +    entry->mElements.RemoveElement(this);

I worried about duplicate entries here, but it looksl ike MediaElementSetForURI is correct that it's a set, not a vector.

Comment 15

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Ralph Giles (:rillian) from comment #12)
> Comment on attachment 8655829 [details] [diff] [review]
> 1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch
> Review of attachment 8655829 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. I think this and the diagnostic patch are a better approach, with a
> follow-up bug to resolve the missing step 4.

Alright, will do.

> What about HTMLMediaElement::AbortExistingLoads()? It looks like mDecoder
> can be set there.

Assuming you meant 'mLoadingSrc', yes it's reset there, but just before that there's an |if (mDecoder) { ... ShutdownDecoder(); }|, which does the appropriate RemoveMediaElementFromURITable(). Please see my notes in comment 8.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8655829 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch

Review of attachment 8655829 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.cpp
@@ +1072,5 @@
>      }
>  
> +    if (mLoadingSrc && mDecoder) {
> +      RemoveMediaElementFromURITable();
> +    }

I suggest just moving these mLoadingSrc and mDecoder checks into RemoveMediaElementFromURITable.

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

Updated as per review/feedback
- from comment 12: Added call to RemoveMediaElementFromURITable before *all* modifications of mLoadingSrc, even when a previous ShutdownDecoder would have done it. It's safer in the long run (if ShutdownDecoder becomes 'too' clever), and cheap enough.
- from comment 14: Added some missing { }.
- from comment 16: Moved mLoadingSrc&mDecoder checks into RemoveMediaElementFromURITable.

Carrying r+ from comment 12.

Comment 18

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-better-MediaElementTableCount.patch v2

Updated as per review from comment 13: Using range-for to loop through nsTArray; Renamed URIEquals to URISafeEquals.
Carrying r+ from comment 13.

Comment 19

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8656340 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- The patch shows that we were not removing the element from gElementTable when modifying mLoadingSrc, I think it would be fairly easy to build a script that does that. Then exploiting this (reusing the dangling pointer after the element is destroyed) is of course harder but evidently possible as shown in the POC.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- The header comment change from "clearing" to "modifying" may be a small clue.
- The check-in comment does mention "so that a future LookupMediaElementURITable won't access this element anymore", this could be made more obscure.
- No tests.

Which older supported branches are affected by this flaw?
- Not sure how to determine that, earliest affected release would be 11 (based on bug 703379's "Target Milestone") when the element table was first introduced. This could be verified by opening the POC pages on older releases -- Please let me know if I should do that, and on which versions.

If not all supported branches, which bug introduced the flaw?
- Bug 703379 in November 2011.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- The patch is simple and could be backported as far as needed, with very low risk. Please let me know which versions we want to backport to, and I can prepare updated patches (if necessary).

How likely is this patch to cause regressions; how much testing does it need?
- Very unlikely I believe: The main method RemoveMediaElementFromURITable() is pretty much the same (assertions have become simple returns), it is just called from a few more places.
- Testing can be done by opening the POC pages before and after applying the patch. DOM tests may also be run for extra confidence that no regression has been introduced.

Comment 20

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8656342 [details] [diff] [review]
1198435-better-MediaElementTableCount.patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Much harder than with the first patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- The extra assertion is a small clue as to the problem, but once again it's not as obvious as with the first patch.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- This patch is not a fix, so I wouldn't worry about backporting it. It is mainly useful in finding similar issues in future development.

How likely is this patch to cause regressions; how much testing does it need?
- Very unlikely: This patch only touches DEBUG-only code. It does a similar job as before (going through a list and incrementing counters), but gives more information at the end.
- Running existing tests in debug mode should be enough to ensure its quality. Also, applying this patch alone (without the actual fix in the other patch) exercises the extra assertion and shows its usefulness.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk for this. We'll want ESR38, Aurora, and Beta patches made and nominated after it lands on trunk.

Comment 22

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1198435-remove-self-from-URITable-when-modifying-mLoadingSrc_esr38.patch

(In reply to Al Billings [:abillings] from comment #21)
> sec-approval+ for trunk for this.

Thank you.

> We'll want ESR38, Aurora, and Beta patches
> made and nominated after it lands on trunk.

I confirm that ESR38, Aurora and Beta had the same issue, and that the patch "1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2" fixes it.

The patch applies cleanly to Aurora and Beta.

ESR38 needs a rebased patch, attached here, requesting sec-approval.
No actual code changes; carrying r+ from comment 12.

Please note that I don't think we need "1198435-better-MediaElementTableCount.patch v2" uplifted, as it's not fixing anything, and only of real use to developers and testers to catch similar issues in the future.


Do I need to do anything else?
(It's my first sec patch; and I don't have push privileges yet)

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8656962 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc_esr38.patch

> Do I need to do anything else?

No, this is fine, it should be ready to be checked in now. (You also don't need sec-approval on different versions of the patch.)

Comment 24

[Carsten Book [:Tomcat]]

gerald: is there a try run for this changes ?

Comment 25

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Carsten Book [:Tomcat] from comment #24)
> gerald: is there a try run for this changes ?

No, I thought try runs were not advisable for sec-critical bugs?

I could sneak an uncommented patch into a try for another uninteresting bug.
Andrew, what would you recommend?

Comment 26

[Andrew McCreight [:mccr8]]

(In reply to Gerald Squelart [:gerald] from comment #25)
> No, I thought try runs were not advisable for sec-critical bugs?
> 
> I could sneak an uncommented patch into a try for another uninteresting bug.

In general, you are right that you don't want to do a try push with the bug number in it, and something like this would be the right thing to do. In this particular case, you've been approved for landing on trunk, so it doesn't matter quite as much. It would still be good to not include the bug number in your try push.

(There's some kind of vague plan for a more secure try server, but I'm not sure what the time line for that is.)

Comment 27

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The two patches are buried in this try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dceca7a77500

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08d1defbf501
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb16e1ad62ea

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Andrew, do you think this fix is safe to uplift to Beta41? We gtb 41 RC on Monday and given that this is sec-critical, we might be able to take a fix if it is low risk.

Comment 30

[Andrew McCreight [:mccr8]]

I don't know. That would be a question for the patch author or reviewer.

Comment 31

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Ritu Kothari (:ritu) from comment #29)
> Andrew, do you think this fix is safe to uplift to Beta41? We gtb 41 RC on
> Monday and given that this is sec-critical, we might be able to take a fix
> if it is low risk.

First, only the first patch "1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2" needs to be uplifted.

I believe it is very low risk:
- The existing method RemoveMediaElementFromURITable() was only changed to replace a few assertions into return statements, e.g. instead of asserting that some pointer should be null and then dereferencing it anyway, we just don't do anything with it (which is fine in this case).
- That method, which was already used, is now called in four more places where it should have been called anyway.
- The patch applies as-is to 42 and 41, and I've locally tested that it does fix the bug -- but only on one platform (Mac OS X 10.10). I could prepare try-runs if preferred.

Back to you Ritu.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Thanks Gerald for a quick follow up. Based on your assessment and a review of the patch it does seem straightforward. I discussed a bit with DVeditz and he believes it is low risk as well. 

Could you please nominate only "1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2" patch for beta uplift? I will discuss it with Sylvestre/Lawrence before making a call sometime tomorrow am PST. Thanks!

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Actually, please nominate uplifts for Aurora and ESR38 too.

Comment 34

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8656340 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

Approval Request Comment
[Feature/regressing bug #]: Bug 1198435
[User impact if declined]: Possible remote code execution in 42.
[Describe test coverage new/current, TreeHerder]: Only local testing! I could prepare try runs, as the risk of exposing this issue.
[Risks and why]: Very low, see comment 31.
[String/UUID change made/needed]: N/A

Comment 35

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8656340 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

Approval Request Comment
[Feature/regressing bug #]: Bug 1198435
[User impact if declined]: Possible remote code execution in 42.
[Describe test coverage new/current, TreeHerder]: Only local testing! I could prepare try runs, as the risk of exposing this issue.
[Risks and why]: Very low, see comment 31.
[String/UUID change made/needed]: N/A

Comment 36

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8656962 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc_esr38.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-critical
User impact if declined: Possible remote code execution in 38.
Fix Landed on Version: 43; also requested beta-42 and aurora-41.
Risk to taking this patch (and alternatives if risky): Very low, see comment 31. (The only difference with the other patch for 41/42/43 is rebased context.)
String or UUID changes made by this patch: N/A.

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08d1defbf501
https://hg.mozilla.org/mozilla-central/rev/cb16e1ad62ea

Comment 38

[Liz Henry (:lizzard) (relman/hg->git project)]

I'd like this for ESR as well if it is uplifted to beta. Ritu can you mark it for ESR if you take it this weekend?  Thanks!

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8656340 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

Let's take it for aurora at least.

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8656962 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc_esr38.patch

Approved for uplift to ESR38.

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8656340 [details] [diff] [review]
1198435-remove-self-from-URITable-when-modifying-mLoadingSrc.patch v2

[Triage Comment] Approving for uplift to m-r so it makes it into 41 RC build. I have discussed this fix with DVeditz and it is deemed safe to uplift to 41 RC.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-release/rev/df2dea566188
https://hg.mozilla.org/releases/mozilla-release/rev/0bfe8c7362cb

Comment 43

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Gerald, KWierso mentioned that your patch has caused a bustage in mozilla-release branch. He said Fennec builds are impacted. I believe we will need to back out unless you can quickly determine the cause and fix it. Thanks!

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out from release (41) in https://hg.mozilla.org/releases/mozilla-release/rev/ab4526a8de9c and putting off uplifting to esr38 until this gets sorted out.

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Was informed only one of those patches should have been uplifted, so relanded that one to see if it's still broken: https://hg.mozilla.org/releases/mozilla-release/rev/3662a8ec90d1

Comment 46

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Clearing the n-i on Gerald as things look good so far.

Comment 47

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Wes Kocher (:KWierso) from comment #45)
> Was informed only one of those patches should have been uplifted, so
> relanded that one to see if it's still broken:
> https://hg.mozilla.org/releases/mozilla-release/rev/3662a8ec90d1

Thank you Wes for relanding just the one needed patch. Pfew!
I just did a local build of m-r, it did compile, and survived the POC test.

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/13d8c5e90f0c
https://hg.mozilla.org/releases/mozilla-esr38/rev/606c3f14c0fd

Comment 49

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f0db999ec7d

Comment 50

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original crash using the poc from comment #0 with Win 7 x64 and Win 10 x64 VM's:

* http://archive.mozilla.org/pub/firefox/releases/40.0.2/win32/en-US/
** https://crash-stats.mozilla.com/report/index/110a745d-5150-445e-ad21-4fb7a2150917
** https://crash-stats.mozilla.com/report/index/b30e3e31-5a70-435f-aa9a-253a12150917
** https://crash-stats.mozilla.com/report/index/acffc48d-759a-416a-be8d-074c42150917

Crash address appears as 0x41414141 as in the crash reports as per comment #0

OS's used:

- Win 7 x64 VM -> reproduced
- Win 10 x64 VM -> reproduced
- OSX 10.10.5 -> reproduced

Went through verification using the following builds:
- https://archive.mozilla.org/pub/firefox/nightly/2015-09-17-03-02-29-mozilla-central/
- https://archive.mozilla.org/pub/firefox/nightly/2015-09-17-00-40-25-mozilla-aurora/
- https://archive.mozilla.org/pub/firefox/candidates/41.0-candidates/build2/
- https://archive.mozilla.org/pub/firefox/candidates/38.3.0esr-candidates/build2/

OS's Used:

* Win 10 x64 VM -> PASSED
** fx43 -> PASS
** fx42 -> PASS
** fx41 -> PASS
** fx38esr -> PASS

* Win 7 x64 VM -> PASSED
** fx43 -> PASS
** fx42 -> PASS
** fx41 -> PASS
** fx38esr -> PASS

* Ubuntu 14.04.3 x64 VM -> PASSED
** fx43 -> PASS
** fx42 -> PASS
** fx41 -> PASS
** fx38esr -> PASS

* OSX 10.10.5 x64 -> PASSED
** fx43 -> PASS
** fx42 -> PASS
** fx41 -> PASS
** fx38esr -> PASS

However, under Ubuntu.. I did get the following crash once but couldn't reproduce it again.. Most likely not related to this issue:
- https://crash-stats.mozilla.com/report/index/a23b517f-48c9-4a26-8de3-2c18e2150917