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

RESOLVED WORKSFORME

Status

()

defect
P1
normal
RESOLVED WORKSFORME
4 years ago
2 years ago

People

(Reporter: mozilla-bugs, Assigned: rillian)

Tracking

({sec-other})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Posted patch proposed patch (obsolete) — Splinter Review
Component: General → Audio/Video
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());
...
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.
Flags: sec-bounty?
Flags: needinfo?(jyavenard)
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.
Flags: needinfo?(jyavenard)
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.
(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.
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.
Keywords: sec-other
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
Assignee: nobody → giles
Priority: -- → P1
Group: core-security → media-core-security
Posted patch Use CheckedInt instead. (obsolete) — Splinter Review
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.
Attachment #8660162 - Flags: review?(amarchesini)
Attachment #8660162 - Flags: feedback?(jyavenard)
Posted patch Use CheckedInt instead. v2 (obsolete) — Splinter Review
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.
Attachment #8660162 - Attachment is obsolete: true
Attachment #8660162 - Flags: review?(amarchesini)
Attachment #8660162 - Flags: feedback?(jyavenard)
Attachment #8660175 - Flags: review?(amarchesini)
Attachment #8660175 - Flags: feedback?(jyavenard)
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
Attachment #8660175 - Flags: feedback?(jyavenard) → feedback-
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 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
Attachment #8660175 - Flags: review?(amarchesini) → review+
With the last big of bug 1185115.

stagefright's SharedBuffer are no longer used.
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.
Attachment #8651133 - Attachment is obsolete: true
Attachment #8660175 - Attachment is obsolete: true
Attachment #8661473 - Flags: review?(gsquelart)
Attachment #8661473 - Flags: review?(gsquelart) → review+
I'd be more inclined to remove it entirely if possible. That and any other unused code forked out of Android.
(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)
Blocks: 1205369
So then it looks like it's still used after all?!
sure does.
Flags: needinfo?(giles)
It's an ASAN error and at no stage is SharedBuffer in the stack trace!
Flags: sec-bounty? → sec-bounty-
(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
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
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.
We may want to consider closing this, or at least removing/downgrading the P1
Component: Audio/Video → Audio/Video: Playback
(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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
Group: media-core-security
You need to log in before you can comment on or make changes to this bug.