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
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>
---
libavformat/isom.h | 6 ++
libavformat/mov.c | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 243 insertions(+)
diff --git a/libavformat/isom.h b/libavformat/isom.h
index 3794b1f0fd..3de8053da2 100644
--- a/libavformat/isom.h
+++ b/libavformat/isom.h
@@ -114,6 +114,12 @@ typedef struct MOVEncryptionIndex {
// settings will be used.
unsigned int nb_encrypted_samples;
AVEncryptionInfo **encrypted_samples;
+
+ uint8_t* auxiliary_info_sizes;
+ size_t auxiliary_info_sample_count;
+ uint8_t auxiliary_info_default_size;
+ size_t* auxiliary_offsets; ///< Absolute seek position
+ size_t auxiliary_offsets_count;
} MOVEncryptionIndex;
typedef struct MOVFragmentStreamInfo {
diff --git a/libavformat/mov.c b/libavformat/mov.c
index 56459742bc..750b8a0860 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5863,6 +5863,228 @@ end:
return ret;
}
+static int mov_parse_auxiliary_info(MOVContext *c, MOVStreamContext *sc, AVIOContext *pb, MOVEncryptionIndex *encryption_index)
+{
+ AVEncryptionInfo **sample, **encrypted_samples;
+ int64_t prev_pos;
+ size_t sample_count, sample_info_size, i;
+ int ret = 0;
+ unsigned int alloc_size = 0;
+
+ if (encryption_index->nb_encrypted_samples)
+ return 0;
+ sample_count = encryption_index->auxiliary_info_sample_count;
+ if (encryption_index->auxiliary_offsets_count != 1) {
+ av_log(c->fc, AV_LOG_ERROR, "Multiple auxiliary info chunks are not supported\n");
+ return AVERROR_PATCHWELCOME;
+ }
+ if (sample_count >= INT_MAX / sizeof(*encrypted_samples))
+ return AVERROR(ENOMEM);
+
+ prev_pos = avio_tell(pb);
+ if (!(pb->seekable & AVIO_SEEKABLE_NORMAL) ||
+ avio_seek(pb, encryption_index->auxiliary_offsets[0], SEEK_SET) != encryption_index->auxiliary_offsets[0]) {
+ av_log(c->fc, AV_LOG_INFO, "Failed to seek for auxiliary info, will only parse senc atoms for encryption info\n");
+ goto finish;
+ }
+
+ for (i = 0; i < sample_count && !pb->eof_reached; i++) {
+ unsigned int min_samples = FFMIN(FFMAX(i, 1024 * 1024), sample_count);
+ encrypted_samples = av_fast_realloc(encryption_index->encrypted_samples, &alloc_size,
+ min_samples * sizeof(*encrypted_samples));
+ if (!encrypted_samples) {
+ ret = AVERROR(ENOMEM);
+ goto finish;
+ }
+ encryption_index->encrypted_samples = encrypted_samples;
+
+ sample = &encryption_index->encrypted_samples[i];
+ sample_info_size = encryption_index->auxiliary_info_default_size
+ ? encryption_index->auxiliary_info_default_size
+ : encryption_index->auxiliary_info_sizes[i];
+
+ ret = mov_read_sample_encryption_info(c, pb, sc, sample, sample_info_size > sc->cenc.per_sample_iv_size);
+ if (ret < 0)
+ goto finish;
+ }
+ if (pb->eof_reached) {
+ av_log(c->fc, AV_LOG_ERROR, "Hit EOF while reading auxiliary info\n");
+ ret = AVERROR_INVALIDDATA;
+ } else {
+ encryption_index->nb_encrypted_samples = sample_count;
+ }
+
+finish:
+ avio_seek(pb, prev_pos, SEEK_SET);
+ if (ret < 0) {
+ for (; i > 0; i--) {
+ av_encryption_info_free(encryption_index->encrypted_samples[i - 1]);
+ }
+ av_freep(&encryption_index->encrypted_samples);
+ }
+ return ret;
+}
+
+/**
+ * Tries to read the given number of bytes from the stream and puts it in a
+ * newly allocated buffer. This reads in small chunks to avoid allocating large
+ * memory if the file contains an invalid/malicious size value.
+ */
+static int mov_try_read_block(AVIOContext *pb, size_t size, uint8_t **data)
+{
+ const unsigned int block_size = 1024 * 1024;
+ uint8_t *buffer = NULL;
+ unsigned int alloc_size = 0, offset = 0;
+ while (offset < size) {
+ unsigned int new_size =
+ alloc_size >= INT_MAX - block_size ? INT_MAX : alloc_size + block_size;
+ uint8_t *new_buffer = av_fast_realloc(buffer, &alloc_size, new_size);
+ unsigned int to_read = FFMIN(size, alloc_size) - offset;
+ if (!new_buffer) {
+ av_free(buffer);
+ return AVERROR(ENOMEM);
+ }
+ buffer = new_buffer;
+
+ if (avio_read(pb, buffer + offset, to_read) != to_read) {
+ av_free(buffer);
+ return AVERROR_INVALIDDATA;
+ }
+ offset += to_read;
+ }
+
+ *data = buffer;
+ return 0;
+}
+
+static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+ MOVEncryptionIndex *encryption_index;
+ MOVStreamContext *sc;
+ int ret;
+ unsigned int sample_count;
+
+ ret = get_current_encryption_info(c, &encryption_index, &sc);
+ if (ret != 1)
+ return ret;
+
+ if (encryption_index->nb_encrypted_samples) {
+ // This can happen if we have both saio/saiz and senc atoms.
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saiz\n");
+ return 0;
+ }
+
+ if (encryption_index->auxiliary_info_sample_count) {
+ av_log(c->fc, AV_LOG_ERROR, "Duplicate saiz atom\n");
+ return AVERROR_INVALIDDATA;
+ }
+
+ avio_r8(pb); /* version */
+ if (avio_rb24(pb) & 0x01) { /* flags */
+ if (avio_rb32(pb) != sc->cenc.default_encrypted_sample->scheme) {
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type\n");
+ return 0;
+ }
+ if (avio_rb32(pb) != 0) {
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring saiz box with non-zero aux_info_type_parameter\n");
+ return 0;
+ }
+ }
+
+ encryption_index->auxiliary_info_default_size = avio_r8(pb);
+ sample_count = avio_rb32(pb);
+ encryption_index->auxiliary_info_sample_count = sample_count;
+
+ if (encryption_index->auxiliary_info_default_size == 0) {
+ ret = mov_try_read_block(pb, sample_count, &encryption_index->auxiliary_info_sizes);
+ if (ret < 0) {
+ av_log(c->fc, AV_LOG_ERROR, "Failed to read the auxiliary info\n");
+ return ret;
+ }
+ }
+
+ if (encryption_index->auxiliary_offsets_count) {
+ return mov_parse_auxiliary_info(c, sc, pb, encryption_index);
+ }
+
+ return 0;
+}
+
+static int mov_read_saio(MOVContext *c, AVIOContext *pb, MOVAtom atom)
+{
+ uint64_t *auxiliary_offsets;
+ MOVEncryptionIndex *encryption_index;
+ MOVStreamContext *sc;
+ int i, ret;
+ unsigned int version, entry_count, alloc_size = 0;
+
+ ret = get_current_encryption_info(c, &encryption_index, &sc);
+ if (ret != 1)
+ return ret;
+
+ if (encryption_index->nb_encrypted_samples) {
+ // This can happen if we have both saio/saiz and senc atoms.
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring duplicate encryption info in saio\n");
+ return 0;
+ }
+
+ if (encryption_index->auxiliary_offsets_count) {
+ av_log(c->fc, AV_LOG_ERROR, "Duplicate saio atom\n");
+ return AVERROR_INVALIDDATA;
+ }
+
+ version = avio_r8(pb); /* version */
+ if (avio_rb24(pb) & 0x01) { /* flags */
+ if (avio_rb32(pb) != sc->cenc.default_encrypted_sample->scheme) {
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type\n");
+ return 0;
+ }
+ if (avio_rb32(pb) != 0) {
+ av_log(c->fc, AV_LOG_DEBUG, "Ignoring saio box with non-zero aux_info_type_parameter\n");
+ return 0;
+ }
+ }
+
+ entry_count = avio_rb32(pb);
+ if (entry_count >= INT_MAX / sizeof(*auxiliary_offsets))
+ return AVERROR(ENOMEM);
+
+ for (i = 0; i < entry_count && !pb->eof_reached; i++) {
+ unsigned int min_offsets = FFMIN(FFMAX(i, 1024), entry_count);
+ auxiliary_offsets = av_fast_realloc(
+ encryption_index->auxiliary_offsets, &alloc_size,
+ min_offsets * sizeof(*auxiliary_offsets));
+ if (!auxiliary_offsets) {
+ av_freep(&encryption_index->auxiliary_offsets);
+ return AVERROR(ENOMEM);
+ }
+ encryption_index->auxiliary_offsets = auxiliary_offsets;
+
+ if (version == 0) {
+ encryption_index->auxiliary_offsets[i] = avio_rb32(pb);
+ } else {
+ encryption_index->auxiliary_offsets[i] = avio_rb64(pb);
+ }
+ if (c->frag_index.current >= 0) {
+ encryption_index->auxiliary_offsets[i] += c->fragment.base_data_offset;
+ }
+ }
+
+ if (pb->eof_reached) {
+ av_log(c->fc, AV_LOG_ERROR, "Hit EOF while reading saio\n");
+ av_freep(&encryption_index->auxiliary_offsets);
+ return AVERROR_INVALIDDATA;
+ }
+
+ encryption_index->auxiliary_offsets_count = entry_count;
+
+ if (encryption_index->auxiliary_info_sample_count) {
+ return mov_parse_auxiliary_info(c, sc, pb, encryption_index);
+ }
+
+ return 0;
+}
+
static int mov_read_schm(MOVContext *c, AVIOContext *pb, MOVAtom atom)
{
AVStream *st;
@@ -6078,6 +6300,17 @@ static int cenc_filter(MOVContext *mov, MOVStreamContext *sc, AVPacket *pkt, int
}
if (encryption_index) {
+ if (encryption_index->auxiliary_info_sample_count &&
+ !encryption_index->nb_encrypted_samples) {
+ av_log(mov->fc, AV_LOG_ERROR, "saiz atom found without saio\n");
+ return AVERROR_INVALIDDATA;
+ }
+ if (encryption_index->auxiliary_offsets_count &&
+ !encryption_index->nb_encrypted_samples) {
+ av_log(mov->fc, AV_LOG_ERROR, "saio atom found without saiz\n");
+ return AVERROR_INVALIDDATA;
+ }
+
if (!encryption_index->nb_encrypted_samples) {
// Full-sample encryption with default settings.
encrypted_sample = sc->cenc.default_encrypted_sample;
@@ -6222,6 +6455,8 @@ static const MOVParseTableEntry mov_default_parse_table[] = {
{ MKTAG('s','i','n','f'), mov_read_default },
{ MKTAG('f','r','m','a'), mov_read_frma },
{ MKTAG('s','e','n','c'), mov_read_senc },
+{ MKTAG('s','a','i','z'), mov_read_saiz },
+{ MKTAG('s','a','i','o'), mov_read_saio },
{ MKTAG('s','c','h','m'), mov_read_schm },
{ MKTAG('s','c','h','i'), mov_read_default },
{ MKTAG('t','e','n','c'), mov_read_tenc },
@@ -6617,6 +6852,8 @@ static void mov_free_encryption_index(MOVEncryptionIndex **index) {
av_encryption_info_free((*index)->encrypted_samples[i]);
}
av_freep(&(*index)->encrypted_samples);
+ av_freep(&(*index)->auxiliary_info_sizes);
+ av_freep(&(*index)->auxiliary_offsets);
av_freep(index);
}
--
2.16.0.rc1.238.g530d649a79-goog
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
