[FFmpeg-devel] [PATCH 2/3] avformat/mov: Fix parsing of saio/siaz atoms in encrypted content.
Jacob Trimble
modmaker at google.com
Fri Apr 20 01:04:57 EEST 2018
On Thu, Apr 19, 2018 at 2:07 AM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Tue, Jan 09, 2018 at 10:27:28AM -0800, Jacob Trimble wrote:
>> On Mon, Jan 8, 2018 at 5:39 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>> > 2018-01-08 23:34 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>> >> On Fri, Jan 5, 2018 at 3:41 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>> >>> 2018-01-05 23:58 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>> >>>> On Fri, Jan 5, 2018 at 2:01 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>> >>>>> 2018-01-05 22:29 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>> >>>>>> On Fri, Jan 5, 2018 at 12:41 PM, Carl Eugen Hoyos <ceffmpeg at gmail.com> wrote:
>> >>>>>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble <modmaker-at-google.com at ffmpeg.org>:
>> >>>>>>>
>> >>>>>>>> + entry_count = avio_rb32(pb);
>> >>>>>>>> + encryption_index->auxiliary_offsets = av_malloc_array(sizeof(size_t), entry_count);
>> >>>>>>>
>> >>>>>>> (sizeof(variable) instead of sizeof(type), please.)
>> >>>>>>>
>> >>>>>>> But since this could be used for a dos attack, please change this
>> >>>>>>> to something similar to 1112ba01.
>> >>>>>>> If it is easy to avoid it, very short files should not allocate
>> >>>>>>> gigabytes.
>> >>>>>>
>> >>>>>> Switched to calculating the size based on the number of remaining
>> >>>>>> bytes and returning an error if it doesn't match what is read.
>> >>>>>
>> >>>>> Sorry if I miss something:
>> >>>>>
>> >>>>>> + entry_count = (atom.size - 8 - (has_saio_type ? 8 : 0)) / (version == 0 ? 4 : 8);
>> >>>>>> + if (avio_rb32(pb) != entry_count) {
>> >>>>>> + av_log(c->fc, AV_LOG_ERROR, "incorrect entry_count in saio\n");
>> >>>>>> + return AVERROR_INVALIDDATA;
>> >>>>>> + }
>> >>>>>> + encryption_index->auxiliary_offsets =
>> >>>>>> + av_malloc_array(sizeof(*encryption_index->auxiliary_offsets), entry_count);
>> >>>>>
>> >>>>> Does this avoid a 1G allocation for a file of a few bytes?
>> >>>>>
>> >>>>> Didn't you simply increase the number of needed bytes to change in a valid
>> >>>>> mov file to behave maliciously from one to two?
>> >>>>
>> >>>> From what I can tell, the mov_read_default method (which is what reads
>> >>>> child atoms) will verify "atom.size" to fit inside the parent atom.
>> >>>> This means that for "atom.size" to be excessively large for the file
>> >>>> size, the input would need to be non-seekable (since I think the
>> >>>> top-level atom uses the file size) and all the atoms would need to
>> >>>> have invalid sizes.
>> >>>
>> >>> (I did not check this but I am not convinced the sample I fixed last
>> >>> week is so sophisticated.)
>> >>>
>> >>>> But technically I guess it is possible.
>> >>>
>> >>> Thank you.
>> >>>
>> >>>> But this is basically malloc some number of bytes then read that many
>> >>>> bytes. The only alternative I can think of (in the face of
>> >>>> non-seekable inputs) is to try-read in chunks until we hit EOF or the
>> >>>> end of the expected size. This seems like a lot of extra work that is
>> >>>
>> >>>> not mirrored elsewhere.
>> >>>
>> >>> On the contrary.
>> >>>
>> >>> But you are right, I forgot to write that you have to add an "if (!eof)"
>> >>> to the reading loops, see the function that above commit changed.
>> >>>
>> >>>> There are several cases where this isn't explicitly checked.
>> >>>
>> >>> Yes, and they will be fixed, please don't add another one.
>> >>>
>> >>>> Also, how does this attack work? If the number is way too big, well
>> >>>> get EOM and error out.
>> >>>
>> >>> Which not only causes dos but also makes checking for other (if you
>> >>> like: more dangerous) issues more difficult which is bad. We already
>> >>> know that there are cases where the issue is hard to avoid, I believe
>> >>> this is not such a case.
>> >>>
>> >>> It is possible to create (valid) samples that allocate a huge amount
>> >>> of memory but very small files should not immediately allocate an
>> >>> amount of several G.
>> >>
>> >> Done.
>> >
>> > + entry_count = avio_rb32(pb);
>> >
>> > This has to be checked for later overflow, same in other spots.
>>
>> Done
>>
>> >
>> > + encryption_index->auxiliary_offsets =
>> > + av_malloc_array(sizeof(*encryption_index->auxiliary_offsets),
>> > entry_count);
>> >
>> > I believe you forgot to remove these two lines.
>>
>> Yep, done.
>>
>> >
>> > Note that I chose "1024*1024" arbitrarily to avoid a speed-loss for
>> > (most likely) all valid files when fixing the dos in 1112ba01.
>> > I don't know what a good lower limit for your use-case can be, or
>> > if only using av_fast_realloc() - without the high starting value -
>> > makes sense.
>>
>> Ok, for the saio atoms, it will likely be small (changed it to 1024)
>> since it will either be the number of tenc atoms or the number of
>> chunks or 1. Later code actually only supports one offset, but I
>> didn't want to hard-code that requirement here.
>>
>> >
>> > Carl Eugen
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel at ffmpeg.org
>> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>> isom.h | 6 +
>> mov.c | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 243 insertions(+)
>> 0c952e937ee35e8f95d1183047dd8d4f4bb1a7a2 0002-avformat-mov-Fix-parsing-of-saio-v4.patch
>> From 76c6870513481c14c5c134f1b8e7e9a91e17e6b5 Mon Sep 17 00:00:00 2001
>> From: Jacob Trimble <modmaker at google.com>
>> Date: Wed, 6 Dec 2017 16:17:54 -0800
>> Subject: [PATCH 2/3] avformat/mov: Fix parsing of saio/siaz atoms in encrypted
>> content.
>>
>> This doesn't support saio atoms with more than one offset.
>>
>> Signed-off-by: Jacob Trimble <modmaker at google.com>
>
> Seems not to apply:
>
> Applying: avformat/mov: Fix parsing of saio/siaz atoms in encrypted content.
> error: sha1 information is lacking or useless (libavformat/isom.h).
> error: could not build fake ancestor
> Patch failed at 0001 avformat/mov: Fix parsing of saio/siaz atoms in encrypted content.
> Use 'git am --show-current-patch' to see the failed patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
I have never used |git am| before, but I am able to apply it fine using patch:
patch -i 0002*.patch -p1 --no-backup-if-mismatch
Maybe you tried to apply without the "remove old encryption info
methods" patch (which was part of the previous email)? I can apply it
against the current master just fine. Here is a version against the
latest master in case there was some hash problems.
> [...]
>
> thanks
> --
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> During times of universal deceit, telling the truth becomes a
> revolutionary act. -- George Orwell
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-avformat-mov-Fix-parsing-of-saio-v5.patch
Type: text/x-patch
Size: 11148 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180419/2c67a794/attachment.bin>
More information about the ffmpeg-devel
mailing list