1216845 - stagefright: unchecked allocation in tx3g and others

Status: RESOLVED FIXED

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

Description

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

AndroidID-20923261, patched in May 2015, published in August 2015:
https://android.googlesource.com/platform/frameworks/av/+/f1ce97ddc2f82d844a6fb8341585eb7b2e655f44%5E!/#F0

The patch description there is only about a missing overflow check before a 'new' in 'tx3g', which we already do since bug 1158568.

But the other part of the actual patch reveals something we don't do: Check that the allocation succeeded before we dereference the pointer.
Other allocations should be checked too.

Probably just a crash risk with low security risk, but let's keep this hidden until we're sure.

Comment 1

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

Attachment: 1216845-check-all-allocs.patch

Check for null ptr after allocations.

- Check return value of every new/malloc/realloc.
- Consistently return -ENOMEM when allocations fail.
- MPEG4Extractor::getTrack() and getMetaData() can return null (because of failed parse or failed alloc); added missing checks in callers.

Note: Some allocs in the 2nd half of MPEG4Extractor have not been touched, as they are in unused code to be removed in bug 1210319.

Comment 2

[Andrew McCreight [:mccr8]]

Do we even need this? new is infallible by default, so I think we'll just crash the browser if such an allocation fails.

https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

Comment 3

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

Comment on attachment 8677845 [details] [diff] [review]
1216845-check-all-allocs.patch

(In reply to Andrew McCreight [:mccr8] from comment #2)
> Do we even need this? new is infallible by default, so I think we'll just
> crash the browser if such an allocation fails.
> 
> https://developer.mozilla.org/en-US/docs/Infallible_memory_allocation

Good point, thank you.

Arrays should be made fallible, as they could have arbitrary sizes. Stopping metadata-processing and recovering from that situation is useful.

At first I thought that single-object allocations, which are usually small, would be best left infallible and crash the browser. But then we could have this situation: A huge array is allocated, that uses almost all of the available memory, but then a subsequent single-object allocation fails because it just passed the limit.
At this point, a fallible allocation could be caught, stopping the processing and freeing all objects used during this processing, leaving the browser with plenty of memory to continue working (but without playing the probably-broken video).

This situation would certainly be a rare event, but since making new's "(fallible)" would be easy, and would add minimal work to the metadata processing, I think we should just do it.

I'll rework the patch in the next few days; please comment if you think the above is a bad idea.

Comment 4

[Andrew McCreight [:mccr8]]

You should of course do whatever think is best, but the general philosophy we use in much of Gecko is to not bother with making allocations fallible until the allocation site starts showing up frequently on crash-stats. As you say, it is pretty easy to make these allocations fallible, but the error handling code is difficult to test in automation. A null pointer that the code thinks is a valid array pointer is probably just going to result in a safe crash, although I did see a writeup of a Flash exploit a while ago where the requested array size was so gigantic that with a presumed-valid null pointer they were able to read and write basically anywhere in memory. We also had a pwn2own bug (bug 982957) due to incorrect OOM handling code in the JS engine.

Comment 5

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

Attachment: 1216845-check-all-fallible-allocs.patch v2

Check fallible allocations:
- Made externally-sized 'new' allocations fallible.
- Check return value of every new(fallible)/malloc/realloc.
- Consistently return -ENOMEM when allocations fail.
- MPEG4Extractor::getTrack() and getMetaData() can return null (because of
failed parse or failed alloc); added missing checks in callers.

Note: Some allocs in the 2nd half of MPEG4Extractor have not been touched, as
they are in unused code to be removed in bug 1210319.

(In reply to Andrew McCreight [:mccr8] from comment #4)
> You should of course do whatever think is best, but the general philosophy
> we use in much of Gecko is to not bother with making allocations fallible
> until the allocation site starts showing up frequently on crash-stats. As
> you say, it is pretty easy to make these allocations fallible, but the error
> handling code is difficult to test in automation. A null pointer that the
> code thinks is a valid array pointer is probably just going to result in a
> safe crash, although I did see a writeup of a Flash exploit a while ago
> where the requested array size was so gigantic that with a presumed-valid
> null pointer they were able to read and write basically anywhere in memory.
> We also had a pwn2own bug (bug 982957) due to incorrect OOM handling code in
> the JS engine.

I've only made a few new's fallible, where the size is taken from the input media file; at least it should prevent any odd file from bringing down the whole browser through trivial OOM.
Each fallible new (and alloc/realloc) is immediately followed by a test, which if failing would completely abort the current metadata processing, meaning potentially-null pointers shouldn't be used later on.

Comment 6

[Ralph Giles (:rillian)]

Comment on attachment 8679811 [details] [diff] [review]
1216845-check-all-fallible-allocs.patch v2

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

Fallibility taiting, eh? :-)

FWIW, I think making fallible both large allocations and those whose size is content-controlled is a useful guideline for the media code.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +100,5 @@
>    sp<MetaData> metaData = mPrivate->mMetadataExtractor->getMetaData();
>  
> +  if (metaData.get()) {
> +    UpdateCrypto(metaData.get());
> +  }

You could avoid the repetition with something like

if (auto p = metaData.get()) {
  UpdateCrypto(p);
}

But I suppose if it races it doesn't matter, it would crash either way.

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +415,5 @@
>  
>  sp<MetaData> MPEG4Extractor::getMetaData() {
>      status_t err;
>      if ((err = readMetaData()) != OK) {
> +        return NULL;

Can this be nullptr instead of NULL?

@@ +517,5 @@
>      }
>      if (psshsize) {
>          char *buf = (char*)malloc(psshsize);
>          if (!buf) {
> +            return -ENOMEM;

There's a NO_MEMORY enum for this in utils/Errors.h if you like that better.

Comment 7

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

(In reply to Ralph Giles (:rillian) from comment #6)
> Comment on attachment 8679811 [details] [diff] [review]
> 1216845-check-all-fallible-allocs.patch v2
> 
> Review of attachment 8679811 [details] [diff] [review]:
> -----------------------------------------------------------------

Thank you for the review.

> ...
> ::: media/libstagefright/binding/MP4Metadata.cpp
> @@ +100,5 @@
> >    sp<MetaData> metaData = mPrivate->mMetadataExtractor->getMetaData();
> >  
> > +  if (metaData.get()) {
> > +    UpdateCrypto(metaData.get());
> > +  }
> 
> You could avoid the repetition with something like
> 
> if (auto p = metaData.get()) {
>   UpdateCrypto(p);
> }
> 
> But I suppose if it races it doesn't matter, it would crash either way.

I don't think there's any race here, 'metaData' is a local variable, |.get()| is a member-access to the raw pointer, so though a bit repetitive I believe it is safe.
(And if there was an 'explicit operator bool()' member, the first null-testing 'get' wouldn't be needed, but would effectively still be there, just implicit.)

But I agree it's probably a pattern to be suspicious of in some circumstances.

> :::
> media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
> @@ +419,1 @@
> > +        return NULL;
> 
> Can this be nullptr instead of NULL?

I'm just following the style of this soon(?)-to-be-removed file, there are NULLs everywhere, it's horrible but consistent!

> @@ +511,1 @@
> > +            return -ENOMEM;
> 
> There's a NO_MEMORY enum for this in utils/Errors.h if you like that better.

Well-spotted! But same as NULL, I'm just following the local style.

Comment 8

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

Comment on attachment 8679811 [details] [diff] [review]
1216845-check-all-fallible-allocs.patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch shows places where the input media file gives a size of a buffer to be allocated, so it would be fairly easy to construct such files. But I believe the effect would only be a crash from infallible 'new's.
There is also the use of an object through a null pointer, this one would be harder to setup, but would also crash due to reads near 0x0.
(A sec expert should confirm this, please.)

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment points at the above issues.

But more importantly anyway: This is based on a patch that's already publicly visible on Android's repository for almost 3 months! (Though there is no corresponding public bug -- yet?)

Which older supported branches are affected by this flaw?
As old as stagefright, so at least ESR38.

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?
If the patch doesn't apply directly, it would be trivial to backport it as necessary.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely, it only adds allocation checks, including for the now-fallible new's. Also we have lots of media tests.

Comment 9

[Andrew McCreight [:mccr8]]

> But I believe the effect would only be a crash from infallible 'new's.
Yes, all of the existing |new|s that you are replacing are safe, because we just crash. So it is just the MP4MetaData changes that might cause a problem. I didn't dig enough to figure out if they are a problem.

Comment 10

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

Dan, this is rated as a sec-audit but it seems we need a better rating here. Is this just an unexploitable crash?

Comment 11

[Daniel Veditz [:dveditz]]

Seems fair without a lot more investigation, which is what sec-audit means.

Comment 12

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

Comment on attachment 8679811 [details] [diff] [review]
1216845-check-all-fallible-allocs.patch v2

Clearing sec-approval since this isn't a high or critical. It can just go in.

Comment 13

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

(In reply to Al Billings [:abillings] from comment #12)
> Comment on attachment 8679811 [details] [diff] [review]
> 1216845-check-all-fallible-allocs.patch v2
> 
> Clearing sec-approval since this isn't a high or critical. It can just go in.

Thank you.

However this patch depends on bug 1216748 going in first. I could rebase all patches to allow this patch here first, but it seems a lot of work for something going in in only a week anyway.
But please let me know if you'd prefer this patch to go in now.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98c4817458d1

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/98c4817458d1