1185115 - (CVE-2015-4479) MPEG4 saio Chunk Integer Overflow (libstagefright) (ZDI-CAN-2966)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Daniel Veditz [:dveditz]]

Attachment: PoC from ZDI (zip, no password)

The security alias received the following information from HP's Zero Day Initiative. According to their "upcoming" list they initially sent it to us on 2015-05-28 but we did not receive it. When I saw Mozilla on their upcoming list I asked them to resend and got this on 2015-07-13 (delayed filing due to travel). I have not yet confirmed this problem. We recently fixed several similar-sounding libstagefright issues so we'll need to first test on the old builds they claim are affected.
 
 
-- VULNERABILITY DETAILS ------------------------
 
Tested against Firefox 38.0.1 and the Nightly from 20150518 on Windows 8.1
 
There is an integer overflow within media/libstagefright/system/core/libutils/VectorImpl.cpp if the attacker can control the size:
 
```
ssize_t VectorImpl::setCapacity(size_t new_capacity)
{
    size_t current_capacity = capacity();
    ssize_t amount = new_capacity - size();
    if (amount <= 0) {
        // we can't reduce the capacity
        return current_capacity;
    }
    SharedBuffer* sb = SharedBuffer::alloc(new_capacity * mItemSize);
    if (sb) {
        void* array = sb->data();
        _do_copy(array, mStorage, size());
        release_storage();
        mStorage = const_cast<void*>(array);
    } else {
        return NO_MEMORY;
    }
    return new_capacity;
}
```
 
There is a second integer overflow within media/libstagefright/system/core/libutils/SharedBuffer.cpp!SharedBuffer::alloc if the attacker can control the size:
 
```
SharedBuffer* SharedBuffer::alloc(size_t size)
{
    SharedBuffer* sb = static_cast<SharedBuffer *>(malloc(sizeof(SharedBuffer) + size));
    if (sb) {
        sb->mRefs = 1;
        sb->mSize = size;
    }
    return sb;
}
```
 
Both of these are reachable through the MPEG4 parser within media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp.
 
Specifically, if a 'saio' chunk is seen while within parseChunk, execution continues to media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp where the controlled value is read and used to allocate memory
 
 
Debug log:
 
```
(998.900): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=91919191 ebx=007af700 ecx=03a4c300 edx=00000001 esi=03a10a60 edi=03a10a70
eip=5e8784cf esp=007af504 ebp=03a4c300 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00210202
xul!mozilla::ipc::MessagePump::Run+0x7b:
5e8784cf ff5008          call    dword ptr [eax+8]    ds:0023:91919199=????????
0:000> kv
ChildEBP RetAddr  Args to Child               
007af524 5e87844e 03a4c300 92946523 03a40fd0 xul!mozilla::ipc::MessagePump::Run+0x7b (FPO: [1,4,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\ipc\glue\messagepump.cpp @ 117]
007af55c 5e878118 03a401d0 00000001 5e8a0200 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]
007af57c 5e8788ec 0ac5e340 00000000 5e87951d 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]
007af588 5e87951d 03a40fd0 0ac5e340 5e6b3162 xul!nsBaseAppShell::Run+0x32 (FPO: [1,0,0]) (CONV: stdcall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\widget\nsbaseappshell.cpp @ 166]
007af594 5e6b3162 03a40fd0 007af80d 0d0a4180 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]
007af5a4 5e9d5f7c 0ac5e340 007af714 007af730 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 @ 282]
007af680 5e9d6e6f 00000001 007af858 03a42160 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 @ 4228]
007af69c 5ea8336b 00000000 018c5ff8 007af800 xul!XREMain::XRE_main+0x1b6 (FPO: [3,2,0]) (CONV: thiscall) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nsapprunner.cpp @ 4308]
007af810 00101635 00000001 018c5ff8 007af858 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 @ 4528]
007af9a4 001012dc 03a42160 018c9ff8 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 @ 294]
007afa38 001010dc 001024f1 001024f1 007afa94 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 @ 669]
007afa4c 00102474 018c5ff8 0186ff98 01871f70 firefox!wmain+0xbc (FPO: [2,0,0]) (CONV: cdecl) [c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\toolkit\xre\nswindowswmain.cpp @ 124]
007afa94 76274198 7f51d000 76274170 c98e43d2 firefox!__tmainCRTStartup+0xfe (FPO: [Non-Fpo]) (CONV: cdecl) [f:\dd\vctools\crt\crtw32\startup\crt0.c @ 255]
007afaa8 770f32b1 7f51d000 7498c8a6 00000000 KERNEL32!BaseThreadInitThunk+0x24 (FPO: [Non-Fpo])
007afaf0 770f327f ffffffff 7711f08b 00000000 ntdll!__RtlUserThreadStart+0x2b (FPO: [SEH])
007afb00 00000000 001024f1 7f51d000 00000000 ntdll!_RtlUserThreadStart+0x1b (FPO: [Non-Fpo])
0:000> !lmi xul
Loaded Module Info: [xul]
         Module: xul
   Base Address: 5e520000
     Image Name: C:\Program Files\Mozilla Firefox\xul.dll
   Machine Type: 332 (I386)
     Time Stamp: 55541969 Wed May 13 20:41:29 2015
           Size: 2268000
       CheckSum: 21a3860
Characteristics: 2122   
Debug Data Dirs: Type  Size     VA  Pointer
             CODEVIEW    76, 1bff998, 1bfe198 RSDS - GUID: {77095B44-069F-4E0E-B81F-F5E718AEA6C9}
               Age: 2, Pdb: c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\toolkit\library\xul.pdb
                   ??    14, 1bffa10, 1bfe210 [Data not mapped]
                CLSID     4, 1bffa24, 1bfe224 [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\77095B44069F4E0EB81FF5E718AEA6C92\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\77095B44069F4E0EB81FF5E718AEA6C92\xul.pdb
0:000> lmvm xul
start    end        module name
5e520000 60788000   xul        (private pdb symbols)  z:\export\symbols\xul.pdb\77095B44069F4E0EB81FF5E718AEA6C92\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 May 13 20:41:29 2015 (55541969)
    CheckSum:         021A3860
    ImageSize:        02268000
    File version:     38.0.1.5611
    Product version:  38.0.1.5611
    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:   38.0.1
    FileVersion:      38.0.1
    FileDescription:  38.0.1
    LegalCopyright:   License: MPL 2
    LegalTrademarks:  Mozilla
    Comments:         Mozilla
0:000> !lmi firefox
Loaded Module Info: [firefox]
         Module: firefox
   Base Address: 00100000
     Image Name: firefox.exe
   Machine Type: 332 (I386)
     Time Stamp: 55540a1a Wed May 13 19:36:10 2015
           Size: 5f000
       CheckSum: 676cb
Characteristics: 122   
Debug Data Dirs: Type  Size     VA  Pointer
             CODEVIEW    76, 19a1c,   18a1c RSDS - GUID: {DEF0A7B6-E41C-4723-87E8-80346BEC2FA0}
               Age: 2, Pdb: c:\builds\moz2_slave\rel-m-rel-w32_bld-000000000000\build\obj-firefox\browser\app\firefox.pdb
                   ??    14, 19a94,   18a94 [Data not mapped]
                CLSID     4, 19aa8,   18aa8 [Data not mapped]
     Image Type: FILE     - Image read successfully from debugger.
                 C:\Program Files\Mozilla Firefox\firefox.exe
    Symbol Type: PDB      - Symbols loaded successfully from image path.
                 z:\export\symbols\firefox.pdb\DEF0A7B6E41C472387E880346BEC2FA02\firefox.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\firefox.pdb\DEF0A7B6E41C472387E880346BEC2FA02\firefox.pdb
0:000> lmvm firefox
start    end        module name
00100000 0015f000   firefox    (private pdb symbols)  z:\export\symbols\firefox.pdb\DEF0A7B6E41C472387E880346BEC2FA02\firefox.pdb
    Loaded symbol image file: C:\Program Files\Mozilla Firefox\firefox.exe
    Image path: firefox.exe
    Image name: firefox.exe
    Timestamp:        Wed May 13 19:36:10 2015 (55540A1A)
    CheckSum:         000676CB
    ImageSize:        0005F000
    File version:     38.0.1.5611
    Product version:  38.0.1.0
    File flags:       0 (Mask 3F)
    File OS:          4 Unknown Win32
    File type:        2.0 Dll
    File date:        00000000.00000000
    Translations:     0000.04b0
    CompanyName:      Mozilla Corporation
    ProductName:      Firefox
    InternalName:     Firefox
    OriginalFilename: firefox.exe
    ProductVersion:   38.0.1
    FileVersion:      38.0.1
    FileDescription:  Firefox
    LegalCopyright:   ©Firefox and Mozilla Developers; available under the MPL 2 license.
    LegalTrademarks:  Firefox is a Trademark of The Mozilla Foundation.
    Comments:         Firefox is a Trademark of The Mozilla Foundation.
0:000> dc @ebp
03a4c300  91919191 91919191 91919191 91919191  ................
03a4c310  91919191 91919191 91919191 91919191  ................
03a4c320  91919191 91919191 91919191 91919191  ................
03a4c330  91919191 91919191 91919191 91919191  ................
03a4c340  91919191 91919191 91919191 91919191  ................
03a4c350  91919191 91919191 91919191 91919191  ................
03a4c360  91919191 91919191 91919191 91919191  ................
03a4c370  91919191 91919191 91919191 91919191  ................
 
```
 
-- CREDIT ---------------------------------------
 
This vulnerability was discovered by:
 
   Anonymous working with HP's Zero Day Initiative

Comment 1

[Daniel Veditz [:dveditz]]

Can definitely reproduce on win 8.1, even on current Nightly so this wasn't fixed by the other recent libstagefright integer overflow fixes. I'm having trouble getting the crash reporter to come up so I have no links.

Comment 2

[Daniel Veditz [:dveditz]]

ZDI has an embargo deadline of 2015-09-25 on this one which means we need the fix in Firefox 41 if not sooner. Also there is a libstagefright talk at blackhat (early August) about previously fixed bugs and might trigger more focus on this library so a fix in 40 would be even better.

Comment 3

[Jean-Yves Avenard [:jya]]

I'll do those tomorrow.

Comment 4

[Kamil Jozwiak [:kjozwiak]]

I can also reproduce it pretty easily with the latest m-c. I tried it on Win 8.1 x64, Win 7 x64 and Win Vista x64 but like Dan mentioned per comment # 1, the crash reporter doesn't seem to be appearing. I tried it with both e10s and non-e10s with the same results and no crash report.

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Ensure we have enough memory to allocate required capacity.

This should do.

I limited the size to SSIZE_MAX, as a vector of char, with a size > SSIZE_MAX would convert to a negative number oncast cast into a ssize_t.
Plus 2GB is more than enough (movie would need to have a duration > 800 days)

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Ensure we have enough memory to allocate required capacity.

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Ensure we have enough memory to allocate required capacity.

will get there... forgot to do git add

Comment 8

[Jean-Yves Avenard [:jya]]

Comment on attachment 8638519 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity.

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
Craft special MP4 with a samples table using invalid size

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. Was careful to use a commit message only describing the outcome ; not the reason for doing so.

> Which older supported branches are affected by this flaw?
35 and all using libstagefright, affects ESR38.

> If not all supported branches, which bug introduced the flaw?
We have several bugs at play, introducing different flaws.
libstagefright was introduced in bug 908503. However it was only made active by default on windows for FF 35 (bug 1057879)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch will apply on all branches.

> How likely is this patch to cause regressions; how much testing does it need?
Can't think of any.

Comment 9

[Jean-Yves Avenard [:jya]]

Attachment: Ensure we have enough memory to allocate required capacity.

SSIZE_MAX isn't defined on some Windows, add definition if necessary

Comment 10

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

Sec-approval+ for checkin on trunk. We'll want Aurora, Beta, and ESR38 patches made and nominated ASAP.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8638895 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:  Crashes
Fix Landed on Version: Not yet. Wanting to do them all within a very short timeframe.
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: None

Approval Request Comment
[Feature/regressing bug #]: 908503
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: try run, local test
[Risks and why]: Very low
[String/UUID change made/needed]: None

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Looks like ESR38 was marked unaffected in error.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8638895 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity.

Approved for Beta, Aurora, and ESR38.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c10dc4f9f3ec

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c10dc4f9f3ec

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8c9e70f89722

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/535b6bb28d80

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/b9904d709491

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c9cf0f212fac
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/c9cf0f212fac
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/a3d2740e93f1

Comment 20

[Daniel Veditz [:dveditz]]

Jean-Yves: what about the potential for overflow from other callers of SharedBuffer::alloc() ? There's _grow() and _shrink() in VectorImpl and calls in String8.cpp and String16.cpp. Wouldn't it be worthwhile insurance to check and return null if the requested size isn't < (SSIZE_MAX - sizeof(SharedBuffer))?

Comment 21

[Jean-Yves Avenard [:jya]]

Yes.. those are buggy too :(

However, I'm not sure on how exploitable those are.

VectorImpl::_grow/_shrink() are use for building the "PathAdder" which is the path containing the atom's and that stagefright is currently looking, and the key index.

To get the overflow with PathAdder, you would need to add 2^32 / 4 sub-atoms to a moov.

The moov atom itself would be at least 8,589,934,592 bytes long that assuming that all atoms in the moov simply descend into its children.
On a 64 bits system, there's no hard drive currently big enough to hold that file.

For the key dictionary, this is controlled by stagefright only so it's safe.

String8/String16 are unused other than for debugging purposes and aren't controllable by the mp4.

Now, I don't believe it can be easily fixed however; while _grow can return (ssize_t)NO_MEMORY if it runs out of memory ; there's absolutely nothing in the calling path *anywhere* checking that it didn't fail or that the returned index would be == to (ssize_t)NO_MEMORY.
It would be a big effort to check all return values. The vectors are really used as infallible.
The design is very bad, because an OOM would return an index ultra big and it would write well outside the allocated array.

I think the best we can do is crash.

If you're happy with that solution, let me know and I'll go ahead

Comment 22

[Kamil Jozwiak [:kjozwiak]]

I quickly went through verification and noticed that opening the POC is still instantly crashing fx (both e10s/non-e10s). This time around it actually produces a crash report which are linked below. The crash reports Exploitability field appears as "none" so I'm not really sure if this crash is related to the original one. None the less, we're still crashing.

Builds Used:
------------

* https://ftp.mozilla.org/pub/firefox/nightly/2015-07-30-03-02-08-mozilla-central/
* https://ftp.mozilla.org/pub/firefox/nightly/2015-07-30-00-40-09-mozilla-aurora/
* https://ftp.mozilla.org/pub/firefox/candidates/40.0b8-candidates/build1/

Using Win 8.1 x64 (VM):
-----------------------

* fx42 -> https://crash-stats.mozilla.com/report/index/efad661a-46b9-4b5a-8e78-996112150730
* fx41 -> https://crash-stats.mozilla.com/report/index/ffbf66f5-05e0-443b-80e8-ad4722150730
* fx40 -> https://crash-stats.mozilla.com/report/index/26508bb6-fe68-454d-b1d5-735de2150730

Using Win 7 x64:
----------------

* fx42 -> https://crash-stats.mozilla.com/report/index/3062295f-9e04-4c04-96ca-5ef542150730

Dan, is this the same/related crash? Or is this a consequence of this fix?

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to Jean-Yves Avenard [:jya] from comment #21)
> It would be a big effort to check all return values. The vectors are really
> used as infallible. The design is very bad, because an OOM would return an
> index ultra big and it would write well outside the allocated array.
> 
> I think the best we can do is crash.
> 
> If you're happy with that solution, let me know and I'll go ahead

I'm happier with a crash-on-malicious-input than a possibly exploitable one. If we're treating insertion as infallible then we need to make it truly infallible by crashing. If we end up crashing too much then we were wrong about its infallibility and need to check return values everywhere instead, but that's better than lots of real content ending up in possibly exploitable conditions.

Comment 24

[Daniel Veditz [:dveditz]]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #22)
> The crash reports
> Exploitability field appears as "none" so I'm not really sure if this crash
> is related to the original one. None the less, we're still crashing.
  ...
> Dan, is this the same/related crash? Or is this a consequence of this fix?

It's definitely a consequence of the fix, but I'm going to have to pass it over to jya as to whether this is safe or not.

http://hg.mozilla.org/mozilla-central/annotate/62469b20ec84/media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp#l596

At line 583 we call setCapacity(cencOffsetCount) which returns NO_MEMORY, but we're not checking errors and continue on. Later on we loop up to cencOffsetCount (which we never got the memory for) and crash writing to null in the getUInt64() call on the highlighted line above.

I'm a little mystified why crash-stats thinks were crashing in the readAt() call, which writes into a local tmp variable, rather than a few lines down where it writes the value into the out param.

http://hg.mozilla.org/releases/mozilla-aurora/annotate/047d1e5ffaa3/media/libstagefright/frameworks/av/media/libstagefright/DataSource.cpp#l73

I think in this spot the bogus values when setCapacity fails will always be null and "safe" crashes, but it makes me worry about other callsites. Maybe it would be safer to make setCapacity() itself fail (crash) when its out of memory. Do any callsites check for the NO_MEMORY return value? If not are they all safe? If they're safe because they crash like this site do we want to rely on that rather than catching the problem where it starts?

Comment 25

[Jean-Yves Avenard [:jya]]

Attachment: Ensure we have enough memory to allocate required capacity. v2

Allright version 2.

I've taken a similar approach as in MPEG4Extractor: we never allocate a vector whose size would be > 2GiB.
For area where the sizes are controlled by a potential attacker , then the return value is tested and would abort early marking the mp4 as corrupted.
For where it's within stagefright ; and plain use of a vector, allocation is now really infallible and will just crash.

Comment 26

[Jean-Yves Avenard [:jya]]

this code is a nightmare, just looking at it.
Going to avoid some of it having the stagefright::Vector use nsTArray instead. will be much faster to code

Comment 27

[Jean-Yves Avenard [:jya]]

Attachment: P2. replace all stagefright::Vector with nsTArray.

The previous patch should be enough in that it will crash when non-expected data will be found.
I'm suggesting we could do this instead.

Comment 28

[Daniel Veditz [:dveditz]]

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

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

Like the return value checking for setCapacity(). The assert()s only help us in debug mode and only if we hit the invalid conditions so we'll have to work with the fuzzing team to make sure we hammer this. f+=dveditz

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 8641652 [details] [diff] [review]
P2. replace all stagefright::Vector with nsTArray.

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

wow, great! Thanks for fixing all these.

Comment 30

[Jean-Yves Avenard [:jya]]

(In reply to Daniel Veditz [:dveditz] from comment #28)
> Comment on attachment 8641562 [details] [diff] [review]
> Ensure we have enough memory to allocate required capacity. v2
> 
> Review of attachment 8641562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Like the return value checking for setCapacity(). The assert()s only help us
> in debug mode and only if we hit the invalid conditions so we'll have to
> work with the fuzzing team to make sure we hammer this. f+=
This isn't MOZ_ASSERT

But system assert as defined in assert.h

It is not dependent on debug build for crashing. It will always terminate the program by calling abort()

Comment 31

[Jean-Yves Avenard [:jya]]

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?
Craft special MP4 with a samples table using invalid size. The first patch removed some of the security risk ; it did not however fix the callers and it would cause a null deref instead.

>Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. Was careful to use a commit message only describing the outcome ; not the reason for doing so.

> Which older supported branches are affected by this flaw?
35 and all using libstagefright, affects ESR38.

> If not all supported branches, which bug introduced the flaw?
We have several bugs at play, introducing different flaws.
libstagefright was introduced in bug 908503. However it was only made active by default on windows for FF 35 (bug 1057879)

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patch will apply on all branches.

> How likely is this patch to cause regressions; how much testing does it need?
Can't think of any. (but neither did I for the first patch)

Comment 32

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

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

Sec-approval+.

I assume that we need to take this on the release branch (and other branches) to fix the crash identified? Is it absolutely necessary or can we live with the crash until next release? How safe is the new crash?

Comment 33

[Sylvestre Ledru [:Sylvestre]]

We will be doing a second RC for 40. If the code lands in the next hours, we can take the patch.
However, before doing it, I would like to see an uplift request (especially the risk evaluation).

Comment 34

[Bobby Holley (:bholley)]

This patch looks quite safe to me. I can't comment on how well it actually fixes the problem, but I don't think it adds much risk.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Landed on inbound at the request of the release drivers.

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a44d30db09

Comment 36

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

The backout patch will need to be uplifted to ESR38 as well. Resetting tracking flags.

Comment 37

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ritu Kothari (:ritu) from comment #36)
> The backout patch will need to be uplifted to ESR38 as well. Resetting
> tracking flags.

Follow-up patch, not a backout :)

https://hg.mozilla.org/mozilla-central/rev/74a44d30db09

Might it make sense to spin off attachment 8641652 [details] [diff] [review] to a new bug if it isn't strictly necessary to fix this one?

Comment 38

[Jean-Yves Avenard [:jya]]

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

This is a follow up patch. A "better" fix. The original patch removed the vulnerability, but it only shifted the problem elsewhere (albeit with a lesser severity)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:  Crashes
Fix Landed on Version: Not yet. Wanting to do them all within a very short timeframe.
Risk to taking this patch (and alternatives if risky): Very low
String or UUID changes made by this patch: None

Approval Request Comment
[Feature/regressing bug #]: 908503
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: try run, local test
[Risks and why]: Very low
[String/UUID change made/needed]: None

Comment 39

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

Jean-Yves, can you please add a link to the try run? I don't think I see one for the patch that needs uplift approval.

Comment 40

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=74a44d30db09

It's on inbound

Comment 41

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

Jean-Yves, thanks for the link. I looked at the try push. It looks mostly clean except a few intermittent failures. 

The one that stood out was a MochiTest intermittent failure tracked by bug 1177070, that bug title is about "CapturedVideo". Do you see a concern/correlation here?

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Note comment 37 - this was already merged to m-c. No, it hasn't caused any issues in our CI. It will be in tomorrow's trunk nightly.

Comment 43

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

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

Approving uplift to Aurora. The sooner we land this to Aurora the better.

Comment 44

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

Are we going to take this on Release and ESR38?

Comment 45

[Sylvestre Ledru [:Sylvestre]]

> User impact if declined:  Crashes
Which were occurring in 39?
How frequent these crashes are?
For now, not sure I want to do a RC3 for this...

Comment 46

[Jean-Yves Avenard [:jya]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #45)
> > User impact if declined:  Crashes
> Which were occurring in 39?
> How frequent these crashes are?
> For now, not sure I want to do a RC3 for this...

This is a security patch... It fixes holes in stagefright.

Yes, you could crash 39 easily; and could do much more in fact without those two patches.

As soon as the advisory is out, I'm expecting people will try to start poking at it, even if the advisory text is very non-descriptive

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

OK, let's do it then.

We will do a new RC with this.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92a911aecda1
https://hg.mozilla.org/releases/mozilla-beta/rev/2a94276e3979
https://hg.mozilla.org/releases/mozilla-release/rev/2a94276e3979
https://hg.mozilla.org/releases/mozilla-esr38/rev/88195eea270f

Comment 49

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

Comment on attachment 8641562 [details] [diff] [review]
Ensure we have enough memory to allocate required capacity. v2

Just to be consistent even though the patch is already in moz-esr38 branch. :)

Comment 50

[Daniel Veditz [:dveditz]]

(In reply to Jean-Yves Avenard [:jya] from comment #30)
> This isn't MOZ_ASSERT But system assert as defined in assert.h
> It is not dependent on debug build for crashing. It will always terminate
> the program by calling abort()

Do we re-define it somewhere? Standard assert() is a macro (stealth lowercase) that is compiled out if NDEBUG is defined, and we define that in opt builds unless something's changed (I still see it in build logs). http://www.cplusplus.com/reference/cassert/assert/

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/01b7e483a783
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/01b7e483a783
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/d2210942ac3d

Comment 52

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following builds:
- https://ftp.mozilla.org/pub/firefox/nightly/2015-08-07-03-02-10-mozilla-central/
- https://ftp.mozilla.org/pub/firefox/nightly/2015-08-07-00-40-08-mozilla-aurora/
- https://ftp.mozilla.org/pub/firefox/candidates/40.0-candidates/build4/
- https://ftp.mozilla.org/pub/firefox/candidates/38.2.0esr-candidates/build2/

OS's Used:

- Win 10 x64 (VM) -> PASSED
- Win 8.1 x64 (VM) -> PASSED
- Win 7 x64 (VM) -> PASSED
- Ubuntu 14.04.2 x64 (VM) -> PASSED
- OSX 10.10.4 x64 -> PASSED

Test Cases Used:

- opened the poc in several tabs/windows in both e10s/non-e10s
- opened the poc in several tabs/windows via Private Browsing
- ensured fx displayed the "No video with supported format and MME type found" error when opening the poc

Comment 53

[Jean-Yves Avenard [:jya]]

Pushed the remaining bit:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c4899a67e4

Comment 54

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f3c4899a67e4

Comment 55

[Jean-Yves Avenard [:jya]]

Comment on attachment 8641652 [details] [diff] [review]
P2. replace all stagefright::Vector with nsTArray.

Approval Request Comment
[Feature/regressing bug #]: 908503
[User impact if declined]: Potential future security issues. While not required per say ; this change remove all doubts that further security issues may be discovered in stagefright memory management.
[Describe test coverage new/current, TreeHerder]: try run, local test, in central
[Risks and why]: Moderately low ; we are now using our long trusted nsTArray over stagefright::Vector implementation
[String/UUID change made/needed]: None

Comment 56

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

Comment on attachment 8641652 [details] [diff] [review]
P2. replace all stagefright::Vector with nsTArray.

Do we need to take this on trunk or is this covered elsewhere?

Comment 57

[Jean-Yves Avenard [:jya]]

it's already on central.

Comment 58

[Kamil Jozwiak [:kjozwiak]]

does this need to be re-tested with the changes pushed via comment #55?