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 <ceffm...@gmail.com> wrote:
> > 2018-01-08 23:34 GMT+01:00 Jacob Trimble
> > <modmaker-at-google....@ffmpeg.org>:
> >> On Fri, Jan 5, 2018 at 3:41 PM, Carl Eugen Hoyos <ceffm...@gmail.com>
> >> wrote:
> >>> 2018-01-05 23:58 GMT+01:00 Jacob Trimble
> >>> <modmaker-at-google....@ffmpeg.org>:
> >>>> On Fri, Jan 5, 2018 at 2:01 PM, Carl Eugen Hoyos <ceffm...@gmail.com>
> >>>> wrote:
> >>>>> 2018-01-05 22:29 GMT+01:00 Jacob Trimble
> >>>>> <modmaker-at-google....@ffmpeg.org>:
> >>>>>> On Fri, Jan 5, 2018 at 12:41 PM, Carl Eugen Hoyos <ceffm...@gmail.com>
> >>>>>> wrote:
> >>>>>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble
> >>>>>>> <modmaker-at-google....@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@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 <modma...@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 <modma...@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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
