Bug 1227052 (CVE-2016-1957)

stagefright delete array

RESOLVED FIXED in Firefox 45

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jmamj90, Assigned: gerald)

Tracking

({sec-low})

Trunk
mozilla45
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [post-critsmash-triage][adv-main45+])

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
Group: core-security
Component: Untriaged → Audio/Video: Playback
Keywords: sec-critical
Product: Firefox → Core
(Reporter)

Comment 1

4 years ago
mm I don't think it will crash, but it leaks memory, I'm interested in your opinion
Keywords: sec-criticalsec-low
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.
Assignee: nobody → gsquelart
Use delete[] for sinf->IPMPData, which was allocated with |new char[...]|.
Attachment #8690724 - Flags: review?(jyavenard)
(Reporter)

Comment 4

4 years ago
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
(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.
Attachment #8690724 - Flags: review?(jyavenard) → review+
https://hg.mozilla.org/mozilla-central/rev/347bf2a3d48d
Status: UNCONFIRMED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main45+]
Alias: CVE-2016-1957
(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 security@mozilla.org 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.
Flags: sec-bounty?
I don't see how delete vs delete[] could ever cause a crash or be exploitable in any ways shape or forms.
(Reporter)

Comment 10

3 years ago
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"
it won't leak memory either as per comment 2
(Reporter)

Comment 12

3 years ago
"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
we re-implement the operator new / delete / delete[] (so that we can use jemalloc)
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
(Reporter)

Comment 15

3 years ago
I understand
Thank you
This isn't eligible for bounty based on the severity of the issue.
Flags: sec-bounty? → sec-bounty-
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.