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

[Jacob Trimble]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-avformat-mov-Fix-parsing-of-saio-v4.patch
Type: text/x-patch
Size: 11100 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180109/2b348e3a/attachment.bin>