Closed Bug 1232069 (CVE-2016-1946) Opened 4 years ago Closed 4 years ago

Truncation in MoofParser::Metadata causes writing beyond buffer's end

Categories

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

42 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 + verified
firefox45 + verified
firefox46 + verified
firefox-esr38 --- unaffected
firefox-esr45 --- fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- affected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: q1, Assigned: gerald)

References

Details

(Keywords: sec-critical, Whiteboard: [adv-main44+])

Attachments

(2 files, 1 obsolete file)

MoofParser::Metadata (media\libstagefright\binding\MoofParser.cpp) can experience an integer overflow. When it does, it allocates a buffer that is smaller than it ought to be. The function then reads data directly from a specially-contrived MP4 file's FTYP atom into that memory, overwriting whatever else is there. The attacker can exactly control the data written and the amount of overwriting -- from 1 byte to ~4 GB -- by adjusting the lengths and contents of the file's FTYP and MOOV atoms.

I think the bug is practically exploitable only on 32-bit builds (because of the required file length), and the discussion below assumes this condition.

The bug appears still to be present in trunk, though there have been minor changes to the affected file since FF 42.0 was released: http://hg.mozilla.org/mozilla-central/file/tip/media/libstagefright/binding/MoofParser.cpp .


Details
-------

The bug is in line 194, which doesn't check for overflow:

184: already_AddRefed<mozilla::MediaByteBuffer>
185: MoofParser::Metadata()
186: {
187:   MediaByteRange ftyp;
188:   MediaByteRange moov;
189:   ScanForMetadata(ftyp, moov);
190:   if (!ftyp.Length() || !moov.Length()) {
191:     return nullptr;
192:   }
193:   nsRefPtr<MediaByteBuffer> metadata = new MediaByteBuffer();
194:   if (!metadata->SetLength(ftyp.Length() + moov.Length(), fallible)) {
195:     // OOM
196:     return nullptr;
197:   }
198: 
199:   nsRefPtr<mp4_demuxer::BlockingStream> stream = new BlockingStream(mSource);
200:   size_t read;
201:   bool rv =
202:     stream->ReadAt(ftyp.mStart, metadata->Elements(), ftyp.Length(), &read);
203:   if (!rv || read != ftyp.Length()) {
204:     return nullptr;
205:   }
206:   rv =
207:     stream->ReadAt(moov.mStart, metadata->Elements() + ftyp.Length(), moov.Length(), &read);
208:   if (!rv || read != moov.Length()) {
209:     return nullptr;
210:   }
211:   return metadata.forget();
212: }

If the MP4 file is contrived so that the length fields of the FTYP and MOOV atoms overflow a size_t (4 bytes in 32-bit builds), line 194 overflows and allocates a too-small buffer. In the attached POC, the FTYP length field is 0x10008, and the MOOV length field is 0xffff0000. The total, computed as a 32-bit value, is thus 8, which is what line 194 passes to |metadata->SetLength()|, which then allocates that buffer. Line 202 then reads 0x10008 bytes into that buffer, overwriting 0x10000 bytes of memory beyond its end. The data that is written comes directly from the MP4 file. Line 207 (and therefore the attempt to play the file) fails, but that doesn't matter because the overwriting has already occurred.

An attacker can control what is written by adjusting the contents of the FTYP atom. The contents of the MOOV atom are immaterial, and are needed only to pad the file out to the required length.

An attacker can control the amount of overwriting by adjusting the lengths of the FTYP and MOOV atoms. For example, she can overwrite only 32 bytes by reducing the length of the FTYP atom to 0x28, and increasing the length of the MOOV atom to 0xffffffe0.

Note that the "8" in this discussion (e.g., "0x10008" and "0x28") is contrived so that MoofParser::Metadata copies the first 8 bytes of the FTYP atom into the allocated portion of the buffer, leaving the attacker with complete freedom to determine which data is written beyond its end.

An attacker can embed the MP4 file as a 1x1 pixel video so that the victim is unaware that she is "viewing" it.


Use the POC by putting a breakpoint on line 194, then loading the POC. Examine the computation of the sum (view disassembly) and notice that it's 8. Then step up to but not into line 202, open a memory window to the memory at |metadata->Elements()|, and step through line 202. Notice that 0x10000 bytes of attack data are written beyond its end. One or more threads might (or might not) crash when this occurs.
The POC also crashes FF 43.0b9 in a similar way to FF 42.0.
Flags: sec-bounty?
The attacker can also use this bug to overwrite an arbitrary number of static variables inside xul.dll (though with a fixed start position). She can do this by taking advantage of the fact that (1) |metadata| isa nsTArray, and (2) a zero-length nsTArray's Elements() function returns a pointer to the byte immediately following the static variable nsTArrayHeader::sEmptyHdr.

To do this, the attacker simply creates an MP4 whose FTYP and MOOV atoms' sizes overflow to 0, rather than 8 as in the original POC, above. When she loads such a file, line 202 copies the entire FTYP atom (including its 8-byte header) over the static variables that begin immediately following nsTArrayHeader::sEmptyHdr.

Since the order of static variables is fixed when the image is built, this attack gives the attacker excellent control over what is written where. The only shortcoming from the attacker's viewpoint is that the 8-byte FTYP atom header gets written into whatever immediately follows nsTArrayHeader::sEmptyHdr.

You can easily simulate this attack by loading the POC, taking the breakpoint on line 194, and modifying the allocation length from 8 to 0. Then step as normal and notice that line 202 overwrites static variables as described.
Summary: Overflow in MoofParser::Metadata causes writing beyond heap buffer's end → Overflow in MoofParser::Metadata causes writing beyond buffer's end
Technically, this bug isn't exactly an overflow. Rather, it's a truncation, because in

194:   if (!metadata->SetLength(ftyp.Length() + moov.Length(), fallible)) {

ftyp.Length() and moov.Length() are |int64_t|s, but SetLength accepts a size_t, which is only 32 bits in 32-bit builds.

I have changed the bug's title accordingly.
Summary: Overflow in MoofParser::Metadata causes writing beyond buffer's end → Truncation in MoofParser::Metadata causes writing beyond buffer's end
It turns out that there can _also_ be an overflow in line 194 if the MP4 atoms use 8-byte length fields, but it should be impossible ever to reach, because Box::Box (media\libstagefright\binding\box.cpp) rejects atoms > 0x7fffffffffffffff long:

97:    if (size > INT64_MAX) {
98:       return;
99:    }

Still the potential overflow should be fixed, as by using something like CheckedInt, and the truncation remains an active bug.
(In reply to q1 from comment #2)
> The attacker can also use this bug to overwrite an arbitrary number of
> static variables inside xul.dll (though with a fixed start position). She
> can do this by taking advantage of the fact that (1) |metadata| isa
> nsTArray, and (2) a zero-length nsTArray's Elements() function returns a
> pointer to the byte immediately following the static variable
> nsTArrayHeader::sEmptyHdr.

BTW, one might make similar bugs less dangerous by putting sEmptyHdr into a section whose pages the OS marks as read-only, or by making Elements() crash (or return nullptr) if called on a zero-length nsTArray.
(In reply to q1 from comment #1)
> The POC also crashes FF 43.0b9 in a similar way to FF 42.0.

And also FF 43.0 release.
(In reply to q1 from comment #6)
> (In reply to q1 from comment #1)
> > The POC also crashes FF 43.0b9 in a similar way to FF 42.0.
> 
> And also FF 43.0 release.

...but sometimes it says that the video is corrupt. Click the reload icon and it usually crashes in the same way as FF 42.0.
BTW, FF does not need to read the entire 4GB file to invoke this bug. It needs only to obtain the file's length and to read the atoms' headers and the contents of the (relatively short) FTYP atom. You can see this by putting the POC on a webserver (use MIME type "video/mp4"), flushing FF's cache, and loading the POC: it loads almost instantly, and the webserver's log shows only a modest number of bytes transferred.
Attached patch 1232069-check-box-sizes.patch (obsolete) — Splinter Review
Check box sizes before alloc&copy.
Assignee: nobody → gsquelart
Attachment #8700297 - Flags: review?(jyavenard)
Comment on attachment 8700297 [details] [diff] [review]
1232069-check-box-sizes.patch

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

Could restrict the size far more. A ftyp or moov that is 2GB In size is totally unreasonable. They are typically less than a 100kB

A ftyp > 2gb wouldn't have passed stage fright though.
Attachment #8700297 - Flags: review?(jyavenard) → review+
IIRC, we have a limit of 16MB for the size of the moov. You should use that same value there too.
Comment on attachment 8700297 [details] [diff] [review]
1232069-check-box-sizes.patch

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

r=me with size limited to 32MB as this is the maximum box size the mood parser will ever read:
https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/Box.cpp#169
Addressed review comments, by exposing 32MB limit from Box and using it as max allowed in MoofParser, this way lengths cannot overflow size_t on 32-bit systems.
Attachment #8700297 - Attachment is obsolete: true
Attachment #8700397 - Flags: review+
Comment on attachment 8700397 [details] [diff] [review]
1232069-check-box-sizes.patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not obvious to see the issue, but it points to the 'ftyp' and 'moov' boxes; after that, it would be easy to construct a bad file.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not directly: The check-in comment talks about checking sizes, and the code just adds an arbitrary 32MB read limit (which was present in a later part of the parsing code).

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Bug 1196398 for that specific line (effectively moving code from bug 1159027), both landed in 41.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Very easy and low-risk to backport as far as we want.

How likely is this patch to cause regressions; how much testing does it need?
Very low risk: It is only adding two more tests that stop further MP4 parsing. And we have lots of media tests.
Attachment #8700397 - Flags: sec-approval?
This has sec-approval to land on 12/29, two weeks into the next cycle. We'll want Aurora and Beta patches prepared and nominated as well.
Attachment #8700397 - Flags: sec-approval? → sec-approval+
Note to self: Prepare&nominate aurora&beta patches, see comment 16.
Flags: needinfo?(gsquelart)
Comment on attachment 8700397 [details] [diff] [review]
1232069-check-box-sizes.patch v2

This patch applies cleanly to aurora and beta.

Approval Request Comment
[Feature/regressing bug #]: MP4 playback.
[User impact if declined]: Potential exploit.
[Describe test coverage new/current, TreeHerder]: Specific issue tested manually with POC attached to bug; too big to check in. Lots of media tests otherwise.
[Risks and why]: Very low risk, only adding two more tests to end metadata-parsing early when boxes have unexpected sizes.
[String/UUID change made/needed]: None.
Flags: needinfo?(gsquelart)
Attachment #8700397 - Flags: approval-mozilla-beta?
Attachment #8700397 - Flags: approval-mozilla-aurora?
Comment on attachment 8700397 [details] [diff] [review]
1232069-check-box-sizes.patch v2

Sec-crit issue, taking it. Beta44+, Aurora45+
Attachment #8700397 - Flags: approval-mozilla-beta?
Attachment #8700397 - Flags: approval-mozilla-beta+
Attachment #8700397 - Flags: approval-mozilla-aurora?
Attachment #8700397 - Flags: approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+
https://hg.mozilla.org/mozilla-central/rev/2a57c0a0cf19
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: core-security → core-security-release
I downloaded the mp4 file and opened it across builds (42.0 to reproduce the issue, 44 beta 8, latest DevEdition and latest Nightly) but I could not see differences between builds. 
Is this something I can manually test? If so can you provide some steps to reproduce the initial issue and then to verify the fix?
Flags: needinfo?(gsquelart)
Unfortunately the proof-of-concept file doesn't have any visible effects (see comment #0).

Testing (without modifying code) would require a debugger under win32, similar to what is described in comment #0:

- Add a breakpoint in media/libstagefright/binding/MoofParser.cpp, line 185 (tests starting with |if (!ftyp.Length()...|).
- Start debugging Firefox.
- Open the unzipped file (from poc.7z).
The debugger should stop at line 185.

* In pre-fix builds, the test will just go through, not returning yet.
Keep stepping-over until line 189 (|if (!metadata->SetLength(...|).
Now examine the 'ftyp' and 'moov' variables, calculate their sizes with 'mEnd'-'mStart', and add both by hand, which should be bigger than 2^32.
Then step-into SetLength() and examine the first parameter 'aNewLen', it will different, smaller than 2^32.
(That's the bug, the 'metadata' buffer ends up smaller than it should be, but later on will be force-fed the full expected size.)

* In post-fix builds, one of the tests at line 186 will succeed, so the function will just |return nullptr|, avoiding further unsafe code.

Please let me know if that method is not suitable for you. It might be possible to create another POC file that would have visible side-effects.
Flags: needinfo?(gsquelart)
If you load something heavy (e.g., a Youtube video) in one or more windows, then load the POC in a different window, it's likely to cause pre-fix FF to crash, though you might need to try it 5-10 times. The same procedure should cause no visible effects in post-fix FF.
(In reply to Gerald Squelart [:gerald] from comment #25)
> Please let me know if that method is not suitable for you. It might be
> possible to create another POC file that would have visible side-effects.

You could extend the length of the FTYP atom to, say, 1 GB. That'd definitely make it crash, unless it causes an OOM instead.
(In reply to Gerald Squelart [:gerald] from comment #25)
> Unfortunately the proof-of-concept file doesn't have any visible effects
> (see comment #0).
> 
> Testing (without modifying code) would require a debugger under win32,
> similar to what is described in comment #0:
> 
> - Add a breakpoint in media/libstagefright/binding/MoofParser.cpp, line 185
> (tests starting with |if (!ftyp.Length()...|).
> - Start debugging Firefox.
> - Open the unzipped file (from poc.7z).
> The debugger should stop at line 185.
> 
> * In pre-fix builds, the test will just go through, not returning yet.
> Keep stepping-over until line 189 (|if (!metadata->SetLength(...|).
> Now examine the 'ftyp' and 'moov' variables, calculate their sizes with
> 'mEnd'-'mStart', and add both by hand, which should be bigger than 2^32.
> Then step-into SetLength() and examine the first parameter 'aNewLen', it
> will different, smaller than 2^32.
> (That's the bug, the 'metadata' buffer ends up smaller than it should be,
> but later on will be force-fed the full expected size.)
> 
> * In post-fix builds, one of the tests at line 186 will succeed, so the
> function will just |return nullptr|, avoiding further unsafe code.
> 
> Please let me know if that method is not suitable for you. It might be
> possible to create another POC file that would have visible side-effects.

This method is kind of to complicated for me, I could simply crash 42.0RC with the steps provided in comment 26 each and every time I tried (With build pre-fix) on Windows 7 32-bit.
bp-350e4e1d-8655-4cc5-b5f0-a2d6d2160118
bp-fd02b2ab-6ac1-4bfe-9a25-a6cc82160118

I could not crash it using Firefox 44 beta 9, latest Developer Edition and Nightly though.

Is this sufficient to call this verified or do I need to follow your method? If so is is possible to give further guidance?
Flags: needinfo?(gsquelart)
Whiteboard: [adv-main44+]
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #28)
> This method is kind of to complicated for me, I could simply crash 42.0RC
> with the steps provided in comment 26 each and every time I tried (With
> build pre-fix) on Windows 7 32-bit.
> bp-350e4e1d-8655-4cc5-b5f0-a2d6d2160118
> bp-fd02b2ab-6ac1-4bfe-9a25-a6cc82160118
> 
> I could not crash it using Firefox 44 beta 9, latest Developer Edition and
> Nightly though.
> 
> Is this sufficient to call this verified or do I need to follow your method?
> If so is is possible to give further guidance?

Yes, the method in comment 26 (thank you q1) shows the extended effects of the bug in pre-fix builds (writing a lot of garbage in memory, which impacts later code), that don't happen anymore after the fix.
Flags: needinfo?(gsquelart)
(In reply to Gerald Squelart [:gerald] from comment #29)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #28)
> > This method is kind of to complicated for me, I could simply crash 42.0RC
> > with the steps provided in comment 26 each and every time I tried (With
> > build pre-fix) on Windows 7 32-bit.
> > bp-350e4e1d-8655-4cc5-b5f0-a2d6d2160118
> > bp-fd02b2ab-6ac1-4bfe-9a25-a6cc82160118
> > 
> > I could not crash it using Firefox 44 beta 9, latest Developer Edition and
> > Nightly though.
> > 
> > Is this sufficient to call this verified or do I need to follow your method?
> > If so is is possible to give further guidance?
> 
> Yes, the method in comment 26 (thank you q1) shows the extended effects of
> the bug in pre-fix builds (writing a lot of garbage in memory, which impacts
> later code), that don't happen anymore after the fix.

Thanks Gerald, marking this as verified fixed on the tested builds from comment 28.
Alias: CVE-2016-1946
Gerald, did this affect Android? Did we tell them if so?

Dan and I are wondering if we're about to 0day Android if we write an advisory for this issue.
Flags: needinfo?(gsquelart)
(In reply to Al Billings [:abillings] from comment #31)
> Gerald, did this affect Android? Did we tell them if so?
> 
> Dan and I are wondering if we're about to 0day Android if we write an
> advisory for this issue.

Good catch. No, I haven't told anyone about this, I don't even know how I would do that! (I'd only dealt with 0days from them so far)

q1, have you reported this to Android as well?
Flags: needinfo?(gsquelart) → needinfo?(q1)
(In reply to Gerald Squelart [:gerald] from comment #32)
> (In reply to Al Billings [:abillings] from comment #31)
> > Gerald, did this affect Android? Did we tell them if so?
> > 
> > Dan and I are wondering if we're about to 0day Android if we write an
> > advisory for this issue.
> 
> Good catch. No, I haven't told anyone about this, I don't even know how I
> would do that! (I'd only dealt with 0days from them so far)
> 
> q1, have you reported this to Android as well?

Is this Android code? From the copyright, and the fact that it's in media\libstagefright\binding , I thought it was purely Mozilla code, so I haven't reported it to anyone else.
Flags: needinfo?(q1)
Yes, this is our code not applicable to android (other than our own browser there)
Oops, of course that's our code, sorry for the confusion.
(In reply to Jean-Yves Avenard [:jya] from comment #34)
> Yes, this is our code not applicable to android (other than our own browser
> there)

Thanks, and good: I don't want to zero-day anyone, especially not with this nasty bug.
Blocks: 1253471
No longer blocks: 1253471
Depends on: 1253471
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.