On Thu, Apr 19, 2018 at 03:04:57PM -0700, Jacob Trimble wrote: > On Thu, Apr 19, 2018 at 2:07 AM, Michael Niedermayer > <[email protected]> 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 <[email protected]> > >> wrote: > >> > 2018-01-08 23:34 GMT+01:00 Jacob Trimble > >> > <[email protected]>: > >> >> On Fri, Jan 5, 2018 at 3:41 PM, Carl Eugen Hoyos <[email protected]> > >> >> wrote: > >> >>> 2018-01-05 23:58 GMT+01:00 Jacob Trimble > >> >>> <[email protected]>: > >> >>>> On Fri, Jan 5, 2018 at 2:01 PM, Carl Eugen Hoyos <[email protected]> > >> >>>> wrote: > >> >>>>> 2018-01-05 22:29 GMT+01:00 Jacob Trimble > >> >>>>> <[email protected]>: > >> >>>>>> On Fri, Jan 5, 2018 at 12:41 PM, Carl Eugen Hoyos > >> >>>>>> <[email protected]> wrote: > >> >>>>>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble > >> >>>>>>> <[email protected]>: > >> >>>>>>> > >> >>>>>>>> + 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 > >> > [email protected] > >> > 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 <[email protected]> > >> 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 <[email protected]> > > > > 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
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
