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. + encryption_index->auxiliary_offsets = + av_malloc_array(sizeof(*encryption_index->auxiliary_offsets), entry_count); I believe you forgot to remove these two lines. 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. Carl Eugen _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel