diff mbox series

[FFmpeg-devel,v2,2/3] avformat/apngdec: Fix size/overflow checks

Message ID 20200112215651.1258-2-andreas.rheinhardt@gmail.com
State New
Headers show
Series [FFmpeg-devel,v2,1/3] avformat/apngdec: Return error for incomplete header
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Andreas Rheinhardt Jan. 12, 2020, 9:56 p.m. UTC
apng data consists of parts containing a small header (including a
four-byte size field) and a data part; the size field does not account
for everything and is actually twelve bytes short of the actual size. In
order to make sure that the size fits into an int, the size field is
checked for being > INT_MAX; yet this does not account for the + 12 and
upon conversion to int (which happens when calling append_extradata()),
the size parameter can still wrap around.

Furthermore, append_extradata() appends the new data to the already
existing extradata and therefore needs to make sure that the combined
size of new and old data as well as padding fits into an int. The check
used for this is "if (old_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE -
new_size)" and this does not work if new_size is > INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE.

Finally, the size field of a fcTL part has to be 26. This was enforced
when reading packets, yet not when reading the header.

All of these issues have been fixed.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/apngdec.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)
diff mbox series

Patch

diff --git a/libavformat/apngdec.c b/libavformat/apngdec.c
index 36657877d2..e9c317efbf 100644
--- a/libavformat/apngdec.c
+++ b/libavformat/apngdec.c
@@ -121,13 +121,13 @@  end:
     return AVPROBE_SCORE_MAX;
 }
 
-static int append_extradata(AVCodecParameters *par, AVIOContext *pb, int len)
+static int append_extradata(AVCodecParameters *par, AVIOContext *pb, uint64_t len)
 {
     int previous_size = par->extradata_size;
     int new_size, ret;
     uint8_t *new_extradata;
 
-    if (previous_size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE - len)
+    if (previous_size + len > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
         return AVERROR_INVALIDDATA;
 
     new_size = previous_size + len;
@@ -208,16 +208,11 @@  static int apng_read_header(AVFormatContext *s)
             goto fail;
 
         len = avio_rb32(pb);
-        if (len > 0x7fffffff) {
-            ret = AVERROR_INVALIDDATA;
-            goto fail;
-        }
-
         tag = avio_rl32(pb);
         switch (tag) {
         case MKTAG('a', 'c', 'T', 'L'):
             if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
-                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
+                (ret = append_extradata(st->codecpar, pb, len + 12ULL)) < 0)
                 goto fail;
             acTL_found = 1;
             ctx->num_frames = AV_RB32(st->codecpar->extradata + ret + 8);
@@ -226,7 +221,7 @@  static int apng_read_header(AVFormatContext *s)
                                     ctx->num_frames, ctx->num_play);
             break;
         case MKTAG('f', 'c', 'T', 'L'):
-            if (!acTL_found) {
+            if (!acTL_found || len != 26) {
                ret = AVERROR_INVALIDDATA;
                goto fail;
             }
@@ -235,7 +230,7 @@  static int apng_read_header(AVFormatContext *s)
             return 0;
         default:
             if ((ret = avio_seek(pb, -8, SEEK_CUR)) < 0 ||
-                (ret = append_extradata(st->codecpar, pb, len + 12)) < 0)
+                (ret = append_extradata(st->codecpar, pb, len + 12ULL)) < 0)
                 goto fail;
         }
     }