1197280 - Integer Overflow in Android's SharedBuffer class (via libstagefright)

Status: RESOLVED WORKSFORME

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

Description

[Joshua J. Drake]

Attachment: SharedBuffer_alloc_intof.md

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.155 Safari/537.36

Steps to reproduce:

I located an additional vulnerability due to code from Android brought into Mozilla's tree.


Actual results:

An exploitable heap overflow.


Expected results:

An error should be returned and no ill behavior should be witnessed.

Comment 1

[Joshua J. Drake]

Attachment: proposed patch

Comment 2

[Andrea Marchesini [:baku]]

Comment on attachment 8651133 [details] [diff] [review]
proposed patch

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

::: system/core/libutils/SharedBuffer.cpp
@@ +26,4 @@
>  
>  SharedBuffer* SharedBuffer::alloc(size_t size)
>  {
> +    if (size >= SIZE_MAX - sizeof(SharedBuffer))

include "mozilla/CheckedInt.h"
and then do:

CheckedInt<size_t> allocSize = size;
allocSize += sizeof(SharedBuffer);
if (!allocSize.isValid())
    return NULL;

SharedBuffer* sb = static_cast<SharedBuffer*>(malloc(allocSize.value());
...

Comment 3

[Daniel Veditz [:dveditz]]

Are we using SharedBuffer still? It's used in the stagefright string classes which jya says we don't use, and used by VectorImpl which we recently replaced with our own nsTArray.

Comment 4

[Jean-Yves Avenard [:jya]]

We haven't replaced all of stagefright::Vector.

While I have submitted total removal of the stagefright::vector and it's been r+ I didn't push it as my level of confidence wasn't 100%.

We've removed the use of vectors only in the management of the sample table (which I think was our own code to start with) and where the stagefright::Sort was used and was crashing (and terribly poor sort algorithm to start with, a bubble sort on an array typically with > 10,000 elements is awful).

Having said that, while the SharedBuffer may have an overflow issue; it can't happen with the way we use it as we check for overflow *before* calling any sharedbuffer allocation routines and we limit the size to 2GB so that particular overflow can't ever happen.

Comment 5

[Joshua J. Drake]

I think you'll find the situation different once you look into 1196525, but who knows. I fixed that one and then this one triggers with basically the same input.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Joshua J. Drake from comment #5)
> I think you'll find the situation different once you look into 1196525, but
> who knows. I fixed that one and then this one triggers with basically the
> same input.

stagefright is only used to parse the ftyp+moov atom in a mp4. so bug 1196525 and its MP3 will never reach this code.

Our only use of editResize is from the stagefright::vector ; and following bug 1185115 ; all sizes are checked *before* editResize is called (2GiB limit).

As far as the use of String8/String16 ; they can't ever be created with data > 2GiB following bug 1128939.

So while you may have seen this vulnerability in stagefright in chrome ; the vulnerability isn't in mozilla's stagefright.

I may have missed something, however my level of confidence is pretty high.

Comment 7

[Daniel Veditz [:dveditz]]

jya: would we want to take this patch anyway, on the presumption that upstream stagefright will and it'll make merging easier? (or would it actually be harder). On the presumption that Jean-Yves is right we'll call this sec-other to keep it hidden for now, but if Joshua comes up with a way to reach this in Firefox then obviously we're wrong.

Comment 8

[Jean-Yves Avenard [:jya]]

We will never perform a merge with upstream stagefright. The plan is to remove stagefright completely and our current copy has diverged way too much now.

Bug 1196525 doesn't apply to our code.

You can take this code if you want, but it adds nothing

Comment 9

[Ralph Giles (:rillian)]

Attachment: Use CheckedInt instead.

If we're going to have the code in tree, we might as well fix it while we're here looking at it. Ok with you, Jean-Yves?

This patch implements baku's CheckedInt suggestion on top of Joshua's fix.

Comment 10

[Ralph Giles (:rillian)]

Attachment: Use CheckedInt instead. v2

This version actually compiles.

I tried to clean up the logic of SharedBuffer::editResize a bit. In particular, I think the fall through to !ownedSize() case is wrong. That's a way to complete allocation when the extended object wouldn't fit, but I don't think it will be detected properly in other calls, since onlyOwner() just returns whether the reference count == 1. Seems safer to fail outright.

Comment 11

[Jean-Yves Avenard [:jya]]

Comment on attachment 8660175 [details] [diff] [review]
Use CheckedInt instead. v2

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

Bug 1185115 has part 2 patch, needs to be applied

It completely remove the use of stagefright vector and use nsTArray instead.

Also, I don't see how we could get into this overflow as we have barrier in place *before* that code is used

Comment 12

[Jean-Yves Avenard [:jya]]

Just like the other bug Joshua reported ; it doesn't apply to our copy of stagefright following bug 1185115.

This is an invalid bug

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8660175 [details] [diff] [review]
Use CheckedInt instead. v2

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

I'm giving my r+ just about the use of CheckedInt.

::: media/libstagefright/system/core/libutils/SharedBuffer.cpp
@@ +28,5 @@
>  SharedBuffer* SharedBuffer::alloc(size_t size)
>  {
> +    mozilla::CheckedInt<size_t> allocSize = size;
> +    allocSize += sizeof(SharedBuffer);
> +    if (!allocSize.isValid()) {

if (!allocSize.isValid() || allocSize.value() >= SIZE_MAX)

@@ +68,5 @@
>          SharedBuffer* buf = const_cast<SharedBuffer*>(this);
>          if (buf->mSize == newSize) return buf;
> +        mozilla::CheckedInt<size_t> reallocSize = newSize;
> +        reallocSize += sizeof(SharedBuffer);
> +        if (reallocSize.isValid()) {

&& reallocSize.value() < SIZE_MAX

Comment 14

[Jean-Yves Avenard [:jya]]

With the last big of bug 1185115.

stagefright's SharedBuffer are no longer used.

Comment 15

[Ralph Giles (:rillian)]

Attachment: Use CheckedInt instead. v3 rollup

Updated patch incorporating comments, rebased to apply to current m-c. Carrying forward r=baku.

Gerald, does this look ok to you? Jean-Yves didn't think we should land this change since it's dead code. I'd rather have dead code without vulnerabilities than with.

Since this is dead code, this can land without security approval and ride the trains; no need to uplift to older branches.

Comment 16

[Joshua J. Drake]

I'd be more inclined to remove it entirely if possible. That and any other unused code forked out of Android.

Comment 17

[Jean-Yves Avenard [:jya]]

(In reply to Joshua J. Drake from comment #16)
> I'd be more inclined to remove it entirely if possible. That and any other
> unused code forked out of Android.

Agreed.

all the vectors and shared buffers could be removed.

(I should point also, that even when that code was active, the vulnerability described here didn't apply)

Comment 18

[Ralph Giles (:rillian)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51994c27948c

Comment 19

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

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/62983bc471ac for test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=14210478&repo=mozilla-inbound

Comment 20

[Joshua J. Drake]

So then it looks like it's still used after all?!

Comment 21

[Ralph Giles (:rillian)]

sure does.

Comment 22

[Jean-Yves Avenard [:jya]]

It's an ASAN error and at no stage is SharedBuffer in the stack trace!

Comment 23

[Joshua J. Drake]

(In reply to Jean-Yves Avenard [:jya] from comment #22)
> It's an ASAN error and at no stage is SharedBuffer in the stack trace!

Does this excerpt from that logviewer.html mean it is indeed used?

13:45:06     INFO -  0x6080003531f8 is located 0 bytes to the right of 88-byte region [0x6080003531a0,0x6080003531f8)
 13:45:06     INFO -  allocated by thread T978 (MediaPl~back #5) here:
 13:45:06     INFO -      #0 0x4724ab in realloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:95
 13:45:06     INFO -      #1 0x7f8865bdca93 in stagefright::SharedBuffer::editResize(unsigned long) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/system/core/libutils/SharedBuffer.cpp:73
 13:45:06     INFO -      #2 0x7f8865b7a0af in stagefright::VectorImpl::_grow(unsigned long, unsigned long) /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/system/core/libutils/VectorImpl.cpp:396
 13:45:06     INFO -      #3 0x7f8865b7a7d5 in insertAt /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/system/core/libutils/VectorImpl.cpp:149
 13:45:06     INFO -      #4 0x7f8865b7a7d5 in stagefright::VectorImpl::push(void const*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/system/core/libutils/VectorImpl.cpp:236
 13:45:06     INFO -      #5 0x7f8865bd0903 in push /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/system/core/include/utils/Vector.h:324
 13:45:06     INFO -      #6 0x7f8865bd0903 in stagefright::SampleIterator::seekTo(unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/libstagefright/frameworks/av/media/libstagefright/SampleIterator.cpp:115

Comment 24

[Jean-Yves Avenard [:jya]]

While it is used; the vulnerability you mentioned (which is an overflow) just can't happen

The only place this is used is MPEG4Source::exportIndex()

exportIndex() will do :
  if (!index.SetCapacity(mSampleTable->countSamples(), mozilla::fallible)) {
    return index;
  }

and SetCapacity() will return false if countSamples() is > 2GiB

You can not have an overflow happening. Everywhere we use the stagefright::Vector (and sharedbuffer) , the check for an overflow is done *prior* calling the vector.

The reason for doing so *before* reaching the code is because nowhere in the stagefright code is there proper test of the returned value of the vector's operations. Testing the return value everywhere was found to be too much

So if you had stagefright::Vector / SharedBuffer returning an error , all you would get instead is either a null-deref or worse, it will continue to work in the old allocated buffer (which would be too small).

there's more details in bug 1185115

Comment 25

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

To add to jya's analysis, there is actually another spot where SharedBuffer is used, and that's in MetaData::mItems.
However I've looked at all the modifying callsites, and they too are properly checked before use, or are of predetermined small size.

So in the end I agree with jya's conclusion that the overflow could not actually happen.

Comment 26

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

We may want to consider closing this, or at least removing/downgrading the P1

Comment 27

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

(In reply to Gerald Squelart [:gerald] (may be slow to respond) from comment #25)
> So in the end I agree with jya's conclusion that the overflow could not
> actually happen.