diff mbox

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

Message ID CAO7y9i9LbpEGefr7nW79VPW1SPM34jnBkG_1NryrJ2EdC9TmUg@mail.gmail.com
State Superseded
Headers show

Commit Message

Jacob Trimble Jan. 8, 2018, 10:34 p.m. UTC
On Fri, Jan 5, 2018 at 3:41 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-01-05 23:58 GMT+01:00 Jacob Trimble <modmaker-at-google.com@ffmpeg.org>:
>> On Fri, Jan 5, 2018 at 2:01 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>> 2018-01-05 22:29 GMT+01:00 Jacob Trimble <modmaker-at-google.com@ffmpeg.org>:
>>>> On Fri, Jan 5, 2018 at 12:41 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble <modmaker-at-google.com@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.

>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Comments

Carl Eugen Hoyos Jan. 9, 2018, 1:39 a.m. UTC | #1
2018-01-08 23:34 GMT+01:00 Jacob Trimble <modmaker-at-google.com@ffmpeg.org>:
> On Fri, Jan 5, 2018 at 3:41 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>> 2018-01-05 23:58 GMT+01:00 Jacob Trimble <modmaker-at-google.com@ffmpeg.org>:
>>> On Fri, Jan 5, 2018 at 2:01 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>> 2018-01-05 22:29 GMT+01:00 Jacob Trimble <modmaker-at-google.com@ffmpeg.org>:
>>>>> On Fri, Jan 5, 2018 at 12:41 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
>>>>>> 2018-01-05 20:49 GMT+01:00 Jacob Trimble <modmaker-at-google.com@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
diff mbox

Patch

From 524f81fd4b7ab31c576271f2747087bd16ca8734 Mon Sep 17 00:00:00 2001
From: Jacob Trimble <modmaker@google.com>
Date: Wed, 6 Dec 2017 16:17:54 -0800
Subject: [PATCH] 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 <modmaker@google.com>
---
 libavformat/isom.h |   6 ++
 libavformat/mov.c  | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 238 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 4aeb379216..71cc9a3af3 100644
--- a/libavformat/mov.c
+++ b/libavformat/mov.c
@@ -5863,6 +5863,223 @@  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;
+    }
+
+    prev_pos = avio_tell(pb);
+    if (avio_seek(pb, encryption_index->auxiliary_offsets[0], SEEK_SET) != encryption_index->auxiliary_offsets[i]) {
+        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)
+{
+    uint8_t *buffer = NULL;
+    unsigned int alloc_size = 0, offset = 0;
+    while (offset < size) {
+        uint8_t *new_buffer = av_fast_realloc(buffer, &alloc_size, alloc_size + 1024 * 1024);
+        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);
+    encryption_index->auxiliary_offsets =
+        av_malloc_array(sizeof(*encryption_index->auxiliary_offsets), entry_count);
+    if (!encryption_index->auxiliary_offsets)
+        return AVERROR(ENOMEM);
+
+    for (i = 0; i < entry_count && !pb->eof_reached; i++) {
+      unsigned int min_offsets = FFMIN(FFMAX(i, 1024 * 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);
+      }
+
+      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 +6295,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 +6450,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 +6847,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.rc0.223.g4a4ac83678-goog