1092370 - (CVE-2015-0825) Stack-buffer-underflow in mozilla::MP3FrameParser::ParseBuffer

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Atte Kettunen]

Attachment: repro-file.mp3

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1414786499/

ASAN-trace:

==22054==ERROR: AddressSanitizer: stack-buffer-underflow on address 0x7fb891d460b9 at pc 0x7fb8c5380eaa bp 0x7fb891d45ea0 sp 0x7fb891d45e98
READ of size 1 at 0x7fb891d460b9 thread T46 (Media Decode #1)
    #0 0x7fb8c5380ea9 in mozilla::MP3FrameParser::ParseBuffer(unsigned char const*, unsigned int, long, unsigned int*) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/MP3FrameParser.cpp:336:0
    #1 0x7fb8c53812a7 in mozilla::MP3FrameParser::Parse(char const*, unsigned int, unsigned long) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/MP3FrameParser.cpp:470:0
    #2 0x7fb8c5632a08 in mozilla::GStreamerReader::ParseMP3Headers() /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/gstreamer/GStreamerReader.cpp:285:0
    #3 0x7fb8c5632e63 in mozilla::GStreamerReader::ReadMetadata(mozilla::MediaInfo*, nsDataHashtable<nsCStringHashKey, nsCString>**) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/gstreamer/GStreamerReader.cpp:326:0
    #4 0x7fb8c53c98dd in mozilla::MediaDecoderStateMachine::DecodeMetadata() /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/MediaDecoderStateMachine.cpp:1948:0
    #5 0x7fb8c53c7162 in mozilla::MediaDecoderStateMachine::CallDecodeMetadata() /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/MediaDecoderStateMachine.cpp:1924:0
    #6 0x7fb8c54254b0 in nsRunnableMethodImpl<void (mozilla::MediaDecoderStateMachine::*)(), void, true>::Run() /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/dom/media/../../dist/include/nsThreadUtils.h:388:0
.
.
.
Address 0x7fb891d460b9 is located in stack of thread T46 (Media Decode #1) at offset 25 in frame
    #0 0x7fb8c563283f in mozilla::GStreamerReader::ParseMP3Headers() /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/gstreamer/GStreamerReader.cpp:272:0

  This frame has 2 object(s):
    [32, 4128) 'bytes' <== Memory access at offset 25 underflows this variable
    [4256, 4260) 'bytesRead'
Thread T46 (Media Decode #1) created by T45 (Media S~hine #1) here:
    #0 0x45e5e5 in __interceptor_pthread_create _asan_rtl_:0
    #1 0x7fb8ce970d8d in _PR_CreateThread /builds/slave/m-cen-l64-asan-000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c:453:0
    #2 0x7fb8ce97090a in PR_CreateThread /builds/slave/m-cen-l64-asan-000000000000000/build/nsprpub/pr/src/pthreads/ptthread.c:544:0
    #3 0x7fb8c15134db in nsThread::Init() /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsThread.cpp:455:0
    #4 0x7fb8c1518d3c in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsThreadManager.cpp:269:0
    #5 0x7fb8c151a298 in nsThreadPool::PutEvent(nsIRunnable*) /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsThreadPool.cpp:101:0
    #6 0x7fb8c151b9d9 in nsThreadPool::Dispatch(nsIRunnable*, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/xpcom/threads/nsThreadPool.cpp:261:0
    #7 0x7fb8c5418cbb in mozilla::MediaTaskQueue::DispatchLocked(mozilla::TemporaryRef<nsIRunnable>, mozilla::MediaTaskQueue::DispatchMode) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/media/MediaTaskQueue.cpp:53:0

Comment 1

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

Any ideas as to a potential dev owner for this?

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: bug1092370.patch

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8525037 [details] [diff] [review]
bug1092370.patch

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

::: dom/media/MP3FrameParser.cpp
@@ +337,5 @@
>          // Found an ID3 header. We don't care about the body of the header, so
>          // just skip past.
>          buffer = ch + mID3Parser.GetHeaderLength() - (ID3_HEADER_LENGTH - 1);
> +
> +        if (buffer <= ch) {

Maybe instead:

  if (buffer <= ch || buffer >= aBufferEnd) {
    return NS_ERROR_FAILURE;
  }

?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Needs sec-approval since it's a sec-high and it's unclear what branches are affected.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: bug1092370.patch

addressed review comment. carry over r+

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8544917 [details] [diff] [review]
bug1092370.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Could be straightforward to craft an MP3 that will segfault Firefox. Very little, if any, risk of much else.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Release and up.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.

Comment 7

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

Comment on attachment 8544917 [details] [diff] [review]
bug1092370.patch

sec-approval+ for trunk checkin on January 26. At that point, we should take it on the (then) Aurora and Beta branches as well if you nominate patches there.

Comment 8

[Andrew McCreight [:mccr8]]

The sec-high keyword got accidentally removed, so I'm just adding it back.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e50bc3e146bc

Please request Aurora/Beta approval on this when you get a chance.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest crashes/asserts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c4faddcc7e7

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

Comment 11

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8544917 [details] [diff] [review]
bug1092370.patch

Oops. Caused by blinding following review comment. |buffer| is meant to be allowed to overflow.

Comment 12

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8525037 [details] [diff] [review]
bug1092370.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 13

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Er, refer to sec-approval in comment 6.

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a1961f96d4a7

Comment 15

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8525037 [details] [diff] [review]
bug1092370.patch

Approval Request Comment
[Feature/regressing bug #]: MP3 playback
[User impact if declined]: could crash on particularly bad MP3s
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a1961f96d4a7

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e7c713e8e682
https://hg.mozilla.org/releases/mozilla-beta/rev/03be92be95c2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/e7c713e8e682
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/70b8982a523d

Comment 19

[Daniel Veditz [:dveditz]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #6)
> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
> Could be straightforward to craft an MP3 that will segfault Firefox. Very
> little, if any, risk of much else.

What if Firefox does NOT segfault, because an exploit manipulates memory to put this buffer near other buffers owned by Firefox? Would that random data be read as part of an MP3? Would that MP3 data be accessible to the page afterwards?

Comment 20

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Daniel Veditz [:dveditz] from comment #19)
> What if Firefox does NOT segfault, because an exploit manipulates memory to
> put this buffer near other buffers owned by Firefox? Would that random data
> be read as part of an MP3? Would that MP3 data be accessible to the page
> afterwards?

This patch causes us to bail immediately in the event of an underrun. Overruns are expected and handled.

Comment 21

[Daniel Veditz [:dveditz]]

I meant what happened _before_ the patch? If memory were arranged in a useful way so it didn't just get a simple access violation what is the code going to do with the bogus memory it accesses? Is it being read or written to? If it's a read is it just stream data, or is it reading objects that might be interpreted as pointers?

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Daniel Veditz [:dveditz] from comment #21)
> I meant what happened _before_ the patch? If memory were arranged in a
> useful way so it didn't just get a simple access violation what is the code
> going to do with the bogus memory it accesses? Is it being read or written
> to? If it's a read is it just stream data, or is it reading objects that
> might be interpreted as pointers?

Ah, right.

The data here is merely being read and parsed; no writing; no interpreting as pointers. In the unlikely event that the buffer pointer ends up on something that looks like MP3, there's still very (very) little risk of meaningful information leakage.

Comment 23

[Daniel Veditz [:dveditz]]

> In the unlikely event that the buffer pointer ends up on something that looks like MP3,
> there's still very (very) little risk of meaningful information leakage.

Assuming it does get past the MP3 parser, is there any way to get the data back into the page? Or does it just get sent off to media players? I think the WebAudio interface can probably get data, right?

Comment 24

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Daniel Veditz [:dveditz] from comment #23)
> Assuming it does get past the MP3 parser, is there any way to get the data
> back into the page? Or does it just get sent off to media players? I think
> the WebAudio interface can probably get data, right?

Not really, no. The parser here is only used to estimate the duration of a given MP3.

We can only rewind here by 9 bytes. There are two cases for the parser to keep going:

Case 1: It looks like another ID3 header. This doesn't matter too much. The length field will overlap with the original ID3 header, which will make our pointer positive again. Moreover, we don't expose any of the ID3 data from this parser.

Case 2: It looks like an MP3 frame. Even in this case, we don't expose much data. We only parse the MP3 frame for duration estimation. Whatever *does* leak out in the form of a duration will have gone through a series of lookup tables and arithmetic operations [1].

Each MP3 frame header is about 3 bytes long. If we suppose we can pack just the headers into the 9 bytes available, we end up with 3 headers. Naively combining the tables, we can get 1000 different values, or about 10 bits per frame of information leakage if we assume an even distribution.

This theoretically leaves us with an upper bound of 30 bits of information leaked. In practice, a zero-length frame body implies a fairly trivial header so we'd really only fit one or two frames (even two is pushing the bounds of usefulness).

If somebody were to somehow use 10 to 20 bits of munged up information from somewhere in the 9 bytes preceding our buffer against us, I would be very, very impressed.

[1] - https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3FrameParser.cpp#76

Comment 25

[Daniel Veditz [:dveditz]]

None of those cases look highly exploitable

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/70b8982a523d