[FFmpeg-devel] [PATCH 2/3] avformat/mov: Fix parsing of saio/siaz atoms in encrypted content.

Michael Niedermayer michael at niedermayer.cc
Sat Apr 21 01:46:17 EEST 2018


On Thu, Apr 19, 2018 at 03:04:57PM -0700, Jacob Trimble wrote:
> 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.

yes, that applies

will apply

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180421/60443edc/attachment.sig>


More information about the ffmpeg-devel mailing list