1227052 - (CVE-2016-1957) stagefright delete array

Status: RESOLVED FIXED

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

Description

[jmamj90]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Safari/537.36

Steps to reproduce:


array creation
http://hg.mozilla.org/mozilla-central/file/tip/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#l689

element destruction (should be array destruction, delete[] instead of delete)
http://hg.mozilla.org/mozilla-central/file/tip/media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp#l405


Actual results:

crash


Expected results:

correct destruction of array

Comment 1

[jmamj90]

mm I don't think it will crash, but it leaks memory, I'm interested in your opinion

Comment 2

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

Yes, I noticed that one recently on Google's repository (it was sneaked in with another bug!), but haven't had a chance to work on it yet. Thank you for reporting it.

Even though delete[] is indeed required there, I thought that in this case delete would still work on most systems, because it's just an array of POD characters (i.e., no special destructors to call for each array element).
Did you actually see crashes because of this?

In any case I'll fix this soon.

Comment 3

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

Attachment: 1227052-use-array-delete.patch

Use delete[] for sinf->IPMPData, which was allocated with |new char[...]|.

Comment 4

[jmamj90]

you are right it won't crash

sorry to disturb, there is a missing check in SampleTable.cpp not yet fixed in Firefox
     mNumSampleSizes = U32_AT(&header[8]);
+    if (mNumSampleSizes > (UINT32_MAX - 12) / 16) {
+        return ERROR_MALFORMED;
+    }

http://hg.mozilla.org/mozilla-central/file/tip/media/libstagefright/frameworks/av/media/libstagefright/SampleTable.cpp#l305

if the issues are already in Google repository, should I ignore them? can they reach a reward? there are a few issues still exploitable I think

Comment 5

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

(In reply to jmamj90 from comment #4)
> sorry to disturb, there is a missing check in SampleTable.cpp not yet fixed
> in Firefox
>      mNumSampleSizes = U32_AT(&header[8]);
> +    if (mNumSampleSizes > (UINT32_MAX - 12) / 16) {
> +        return ERROR_MALFORMED;
> +    }
> 
> http://hg.mozilla.org/mozilla-central/file/tip/media/libstagefright/
> frameworks/av/media/libstagefright/SampleTable.cpp#l305

I would rather you create new bugs than adding to this one, please.
In this case, the potential issue was fixed (differently) in bug 1149605.

> if the issues are already in Google repository, should I ignore them? can
> they reach a reward? there are a few issues still exploitable I think

Sorry, I don't know the policies regarding rewards (aka bounty).

I am currently looking through Google's repository to see if we're missing anything.
But it's a slow process, and we are focusing on exploitable issues first, as we plan to stop using stagefright in (hopefully) the medium term.
You may of course continue to report potential issues, but I cannot promise that we will act on any/all of them; it's a case-by-case exercise.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/347bf2a3d48d

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/347bf2a3d48d

Comment 8

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

(In reply to jmamj90 from comment #4)

> if the issues are already in Google repository, should I ignore them? can
> they reach a reward? there are a few issues still exploitable I think

Just saw this. I work on bounties. I've marked this for bounty consideration so our committee can decide about this. In the future, email [e-mail] when you want a bug considered for the bug bounty program. One of us that gets those e-mails will mark the bug for consideration.

Comment 9

[Jean-Yves Avenard [:jya]]

I don't see how delete vs delete[] could ever cause a crash or be exploitable in any ways shape or forms.

Comment 10

[jmamj90]

right, in my second message I've corrected the specification
this is what is written (visible above in 2nd msg):
"mm I don't think it will crash, but it leaks memory, I'm interested in your opinion"

Comment 11

[Jean-Yves Avenard [:jya]]

it won't leak memory either as per comment 2

Comment 12

[jmamj90]

"behavior is undefined" according to it ISO/IEC 2014-11-19
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2014/n4296.pdf

5.3.5 Delete [expr.delete]
1 The delete-expression operator destroys a most derived object (1.8) or array created by a new-expression.
delete-expression:
::optdelete cast-expression
::optdelete [ ] cast-expression
The first alternative is for non-array objects, and the second is for arrays. Whenever the delete keyword
is immediately followed by empty square brackets, it shall be interpreted as the second alternative.79 The
operand shall be of pointer to object type or of class type. If of class type, the operand is contextually
implicitly converted (Clause 4) to a pointer to object type.80 The delete-expression’s result has type void.
2 If the operand has a class type, the operand is converted to a pointer type by calling the above-mentioned
conversion function, and the converted operand is used in place of the original operand for the remainder of this section. In the first alternative (delete object), the value of the operand of delete may be a null pointer
value, a pointer to a non-array object created by a previous new-expression, or a pointer to a subobject (1.8)
representing a base class of such an object (Clause 10). If not, the behavior is undefined.


maybe current implementation *could* let this bug survive, but it is a dormant undefined behaviour vulnerability which could be awaken on changes in compiler implementation

Comment 13

[Jean-Yves Avenard [:jya]]

we re-implement the operator new / delete / delete[] (so that we can use jemalloc)

Comment 14

[Jean-Yves Avenard [:jya]]

Don't get me wrong...

Yes, this bug should be fixed and thank you for reporting it.

I'm only assessing the severity of this bug

Comment 15

[jmamj90]

I understand
Thank you

Comment 16

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

This isn't eligible for bounty based on the severity of the issue.