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

Michael Niedermayer michael at niedermayer.cc
Thu Apr 19 12:07:53 EEST 2018


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".

[...]

thanks
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- 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/20180419/1c92580a/attachment.sig>


More information about the ffmpeg-devel mailing list