Closed Bug 1289280 Opened 3 years ago Closed 3 years ago

FFMPEG: heap-buffer-overflow read in [@av_packet_split_side_data]

Categories

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

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox48 --- wontfix
firefox49 + fixed
firefox-esr45 49+ fixed
firefox50 + fixed
firefox51 + fixed

People

(Reporter: tsmith, Assigned: gerald)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [adv-main49+][adv-esr45.4+])

Attachments

(4 files, 3 obsolete files)

Attached video test_case.webm
I found this while fuzzing a nightly build of the browser not a standalone ffmpeg build.

This could be potentially nasty. I am seeing the following over read but also a much larger on at another location with the same test case. Not sure if it is the same issue or two separate issues.

==6943==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6190012f0d44 at pc 0x7fce58a7dcd9 bp 0x7fce514b4c60 sp 0x7fce514b4c58
READ of size 8 at 0x6190012f0d44 thread T57 (MediaPD~oder #1)
    #0 0x7fce58a7dcd8 in av_packet_split_side_data /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/ffvpx/libavcodec/avpacket.c:396:51
    #1 0x7fce58b42baa in avcodec_decode_video2 /builds/slave/m-in-l64-asan-0000000000000000/build/src/media/ffvpx/libavcodec/utils.c:2115:25
    #2 0x7fce8cf4960f in mozilla::FFmpegVideoDecoder<46465650>::DoDecode(mozilla::MediaRawData*, unsigned char*, int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:235:5
    #3 0x7fce8cf48fe1 in mozilla::FFmpegVideoDecoder<46465650>::DoDecode(mozilla::MediaRawData*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:187:17
    #4 0x7fce8cf469af in mozilla::FFmpegDataDecoder<46465650>::ProcessDecode(mozilla::MediaRawData*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/media/platforms/ffmpeg/FFmpegDataDecoder.cpp:122:11
    #5 0x7fce8cf4b6be in applyImpl<mozilla::FFmpegDataDecoder<LIBAV_VER>, void (mozilla::FFmpegDataDecoder<LIBAV_VER>::*)(mozilla::MediaRawData *), StorensRefPtrPassByPtr<mozilla::MediaRawData> , 0> /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:729:12
    #6 0x7fce8cf4b6be in apply<mozilla::FFmpegDataDecoder<LIBAV_VER>, void (mozilla::FFmpegDataDecoder<LIBAV_VER>::*)(mozilla::MediaRawData *)> /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:735
    #7 0x7fce8cf4b6be in mozilla::detail::RunnableMethodImpl<void (mozilla::FFmpegDataDecoder<46465650>::*)(mozilla::MediaRawData*), true, false, RefPtr<mozilla::MediaRawData> >::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsThreadUtils.h:764
...
See log.txt for entire log.
Attached file log.txt
See Also: → 1289282
The other less frequent over read is bug 1289282
Cannot reproduce this with command line FFmpeg (git master)

I dont have a mozilla development environment setup ATM here but
are the AVPacket->data and ->size fields valid that are passed into avcodec_decode_video2() from build/src/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp:235:5 ?
Adding ronald as this may (or may not) be related to the output from the vp9 parser
Do you have a link to the code that mozilla uses to interact with libavformat? I'm suspecting that it's slightly different from what ffmpeg.c does and that causes the issue. (This may still be an API violation in the vp9 parser, I'm not saying mozilla is buggy, I'm just saying I suspect it's different.)
(In reply to Ronald S. Bultje from comment #5)
> Do you have a link to the code that mozilla uses to interact with
> libavformat?

Ronald, I think the code you are looking can be found here: https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp.

jya can you please confirm this is correct?
Flags: needinfo?(jyavenard)
Blocks: grizzly
That's where we feed the decoder with compressed data. 

For the demuxing part, this is done by libnestegg and its wrapper found in dom/media/webm/WebMDemuxer.cpp

I'll try to extract what data is being fed to ffvpx
Flags: needinfo?(jyavenard)
Keywords: sec-high
Attached patch 1289280.patch (obsolete) — Splinter Review
While debugging, I noticed that the 'size' out-parameter was larger than the allowed input size, and that size is passed to DoDecode and then avcodec_decode_video2, which explains the buffer overrun there.

This patch just checks that the parsed buffer is totally contained inside the given input buffer, and if not triggers a DECODE_ERROR.

Note that this does not correct the actual issue, which happens inside FFmpeg's av_parser_parse2, but at least we can catch this problem before continuing with the buffer overrun, before FFmpeg is properly fixed.
Assignee: nobody → gsquelart
Attachment #8776451 - Flags: review?(jyavenard)
Blocks: 1289282
Comment on attachment 8776451 [details] [diff] [review]
1289280.patch

assuming that av_parser_parse2 always return a pointer within the original data (which seems likely, otherwise we would leak)
Attachment #8776451 - Flags: review?(jyavenard) → review+
Attached patch 1289280.patch (obsolete) — Splinter Review
After discussion with Jean-Yves:

Previous patch didn't account for a possible data==nullptr in some cases.

So now we only test that the output size is never bigger than the input size, *if* there is a non-zero size.
And I don't assume that the output buffer must be inside the input buffer, in case it gets copied elsewhere (in which case there might well be a leak, but it wouldn't be a security issue, so we'll handle that later if that ever happens.)

Carrying r+ from comment 9.
Attachment #8776451 - Attachment is obsolete: true
Attachment #8776457 - Flags: review+
Attached patch 1289280.patch (obsolete) — Splinter Review
Sorry, I messed up the previous patch, by comparing the output size to inputSize *after* inputSize was modified.

To avoid further embarrassment, I'm requesting another review, just to be sure.
(I've tested locally with the fuzzing POC from this bug and bug 1289282, and mochitests.)
Attachment #8776457 - Attachment is obsolete: true
Attachment #8776461 - Flags: review?(jyavenard)
Attachment #8776461 - Flags: review?(jyavenard) → review+
Attachment #8776461 - Flags: review+ → superreview?
Attachment #8776461 - Flags: superreview? → review+
Comment on attachment 8776461 [details] [diff] [review]
1289280.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not that easily I believe, because it would require knowledge of what FFmpeg's av_parser_parse2 function does, to be able to tailor a bad webm file that will produce a too-big packet size.
Also, even after achieving this, further knowledge of FFmpeg would be required to exploit the buffer overrun happening later in avcodec_decode_video2.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch and check-in comment point at the fact that the output size was not checked, there's not much we can do about that. But it doesn't make it obvious how it could be exploited next.

Which older supported branches are affected by this flaw?
All: Landed in FF43 (bug 1197319), so ESR45 and later are affected.

If not all supported branches, which bug introduced the flaw?
(See above)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Rebase will be needed, but it is trivial to create backports as needed, without risks.

How likely is this patch to cause regressions; how much testing does it need?
At worst it could prevent playing some videos that could theoretically play today (despite the buffer overrun), but this really shouldn't happen in real life as it would indicate a corrupt file with a potential exploit in it.
Attachment #8776461 - Flags: sec-approval?
The patch below might help but if it helps then there might be something inconsistent or at least suboptimal in how the parser is fed.


commit 82ff29f1b76787b11d73b1959ac74c8496067de4
Author: Michael Niedermayer <michael@niedermayer.cc>
Date:   Mon Aug 1 12:32:37 2016 +0200

    avcodec/vp9_parser: Check the input frame sizes for being consistent
    
    Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>

diff --git a/libavcodec/vp9_parser.c b/libavcodec/vp9_parser.c
index 2e9235e..aa06091 100644
--- a/libavcodec/vp9_parser.c
+++ b/libavcodec/vp9_parser.c
@@ -88,6 +88,12 @@ static int parse(AVCodecParserContext *ctx,
         return 0;
     }
 
+    if (s->n_frames > 0 && s->size[s->n_frames - 1] > size) {
+        av_log(avctx, AV_LOG_ERROR, "Inconsistent input frame sizes %d %d\n",
+               s->size[s->n_frames - 1], size);
+        s->n_frames = 0;
+    }
+
     if (s->n_frames > 0) {
         *out_data = data;
         *out_size = s->size[--s->n_frames];
Comment on attachment 8776461 [details] [diff] [review]
1289280.patch

We've missed the release train anyway, so I'm removing the sec-approval request from this patch as I'd like to take the time and look at Michael's proposed fix (or something like it), which is probably much better than mine!
Flags: needinfo?(gsquelart)
Attachment #8776461 - Flags: sec-approval?
I think the way the parser is called in dom/media/platforms/ffmpeg/FFmpegVideoDecoder.cpp would loose all frames of a packet after a decode error without reseting/closing and reopening the parser. This might be the source of the inconsistency as the vp9 parser ATM without any patches does not expect this
See Also: → 1290902
Take 4: This is an import of a public patch from ffmpeg.org, it fixes the core issue by catching size mismatches during parsing.

I've tested it locally with the POCs and mochitests.

I'm obsoleting my previous patch, as I don't think it is required anymore, and at worst it could have prevented some corner cases from playing.
Attachment #8776461 - Attachment is obsolete: true
Flags: needinfo?(gsquelart)
Attachment #8776841 - Flags: review?(jyavenard)
Attachment #8776841 - Flags: review?(jyavenard) → review+
Comment on attachment 8776841 [details] [diff] [review]
1289280-ffvpx-vp9_parser.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not that easily I believe, because it would require knowledge of where FFmpeg's parse_frame function is used inside Firefox, to be able to tailor a bad webm file that will produce a too-big packet size.
Also, even after achieving this, further knowledge of FFmpeg would be required to exploit the buffer overrun happening later in Firefox's separate call to avcodec_decode_video2.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch and check-in comment point at the fact that frame individual sizes inside a superframe was not checked. But it doesn't make it obvious that it could be exploited next (except by being associated with this sec bug).

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Bug 1214462, landed in FF46.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch applies cleanly on Aurora and Beta.

How likely is this patch to cause regressions; how much testing does it need?
At worst it could prevent playing some videos that could theoretically play today (despite the buffer overrun), but this really shouldn't happen in real life as it would indicate a corrupt file with a potential exploit in it.
Attachment #8776841 - Flags: sec-approval?
(In reply to Gerald Squelart [:gerald] from comment #17)

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
> Bug 1214462, landed in FF46.

Your previous sec-approval said that 43 and higher, including ESR45, were affected. Now you're saying 46 and higher. Can you clarify, please?
Flags: needinfo?(gsquelart)
(In reply to Al Billings [:abillings] from comment #18)
> (In reply to Gerald Squelart [:gerald] from comment #17)
> 
> > Which older supported branches are affected by this flaw?
> > If not all supported branches, which bug introduced the flaw?
> > Bug 1214462, landed in FF46.
> 
> Your previous sec-approval said that 43 and higher, including ESR45, were
> affected. Now you're saying 46 and higher. Can you clarify, please?

Well-spotted, thank you.

The source of the overrun found by fuzzing is in ffvpx, which we only imported in FF46 (bug 1214462). That is why this patch can only be ported up to 46.

Before that, on Linux we would have either used ffmpeg (if installed) or libvpx.
A system-provided ffmpeg could of course have the same issue. I'm guessing that users of ESR will probably not update their system much either, and therefore would miss ffmpeg fixes, so we should try and fix them.
Discussing with Jean-Yves, he thinks the best solution there would be to just disable VP9 decoding through ffmpeg, which would protect us from this issue and also from others, including unreliable VP9 decoding compared to libvpx.

I will submit a patch shortly for the ESR45 case.
Flags: needinfo?(gsquelart)
What does "unreliable VP9 decoding compared to libvpx" mean?
(In reply to Ronald S. Bultje from comment #20)
> What does "unreliable VP9 decoding compared to libvpx" mean?

I would presume that most people using ESR45, are actually also on older version, and likely to use older version of LibAV.
As you know, LibAV has never upgraded their code with all the ffmpeg fixes.
Oh, I see, you mean the project Libav, not the libav* libraries in FFmpeg, sorry about that, I misunderstood.
Yes, it is unfortunate that the majority of users are still using LibAV at present. Though we did limit vp9 decoding to libavcodec >= 55

The alternative was to use :gerald's original patch and exit early if the return value of the parser was nonsensical. But we're afraid of some unknown consequences that could break playback
I don't think you want to use Libav's VP9 decoder for any reason. Most of its code is C, so it will be substantially slower than libvpx. FFmpeg's decoder, on the other hand, will be much faster than libvpx or Libav's VP9 decoder.
Don't use libav to decode VP9.
Attachment #8777632 - Flags: review?(jyavenard)
Comment on attachment 8776841 [details] [diff] [review]
1289280-ffvpx-vp9_parser.patch

sec-approval+
Attachment #8776841 - Flags: sec-approval? → sec-approval+
Attachment #8777632 - Flags: review?(jyavenard) → review+
Comment on attachment 8777632 [details] [diff] [review]
1289280-no-libav-vp9-ESR45.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very hard, as the patch is just disabling all of VP9/libav, without hint as to why

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?
ESR45

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?
This patch is only for ESR45

How likely is this patch to cause regressions; how much testing does it need?
No regressions expected, we'll fallback to libvpx to play VP9.


[ESR45 Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high

User impact if declined: Potential exploit
Fix Landed on Version: Not yet, should land on 51n, 40a, 49b
Risk to taking this patch (and alternatives if risky): Can't see a risk, we're just disabling VP9 decoding through libav
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8777632 - Flags: sec-approval?
Attachment #8777632 - Flags: approval-mozilla-esr45?
Attachment #8777632 - Flags: sec-approval?
Attachment #8777632 - Flags: sec-approval+
Attachment #8777632 - Flags: approval-mozilla-esr45?
Attachment #8777632 - Flags: approval-mozilla-esr45+
Comment on attachment 8776841 [details] [diff] [review]
1289280-ffvpx-vp9_parser.patch

I've just noticed that I had not officially requested uplift to aurora and beta. Just in case it is required:

Approval Request Comment
[Feature/regressing bug #]: webm playback
[User impact if declined]: OOB access, at worst a potential sec issue
[Describe test coverage new/current, TreeHerder]: Ran POC and webm mochitests locally
[Risks and why]: Low risk as it's only adding a sanity check for sizes. Main risk would be to refuse playing some corrupted (but previously still playable) videos.
[String/UUID change made/needed]: None
Attachment #8776841 - Flags: approval-mozilla-beta?
Attachment #8776841 - Flags: approval-mozilla-aurora?
Attachment #8776841 - Flags: approval-mozilla-beta?
Attachment #8776841 - Flags: approval-mozilla-beta+
Attachment #8776841 - Flags: approval-mozilla-aurora?
Attachment #8776841 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/e734cf260908
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Duplicate of this bug: 1289282
Group: media-core-security → core-security-release
Whiteboard: [adv-main49+][adv-esr45.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.