diff mbox

[FFmpeg-devel] ffmdec: sanitize codec parameters

Message ID f77c111e-ecb6-e00c-7edf-9a9b45ad8ecd@googlemail.com
State Accepted
Headers show

Commit Message

Andreas Cadhalpun Nov. 19, 2016, 11:37 p.m. UTC
On 19.11.2016 22:36, Michael Niedermayer wrote:
> On Sat, Nov 19, 2016 at 05:30:59PM +0100, Andreas Cadhalpun wrote:
>> A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2]
>> convinced me that it should be avoided.
> 
> I think there are 2 different things here
> unavailable data, like a bitrate of 0
> and
> wrong data like a negative bitrate
> 
> We do not accept wrong values in general,

If you say so. I'm more interested in fixing the crash than continuing
this discussion, so I changed it to always error out. See attached patch.

> ffm files are AFAIK generally temporary (on disk fifo) files generated
> by the current muxer.

If that's the case, why does the demuxer still support FFM1 even though
the muxer can't create such files anymore?

Best regards,
Andreas

Comments

Michael Niedermayer Nov. 20, 2016, 2:41 a.m. UTC | #1
On Sun, Nov 20, 2016 at 12:37:05AM +0100, Andreas Cadhalpun wrote:
> On 19.11.2016 22:36, Michael Niedermayer wrote:
> > On Sat, Nov 19, 2016 at 05:30:59PM +0100, Andreas Cadhalpun wrote:
> >> A hard failure is unnecessary and Carl Eugen [1] and Hendrik [2]
> >> convinced me that it should be avoided.
> > 
> > I think there are 2 different things here
> > unavailable data, like a bitrate of 0
> > and
> > wrong data like a negative bitrate
> > 
> > We do not accept wrong values in general,
> 
> If you say so. I'm more interested in fixing the crash than continuing
> this discussion, so I changed it to always error out. See attached patch.
> 
> > ffm files are AFAIK generally temporary (on disk fifo) files generated
> > by the current muxer.
> 
> If that's the case, why does the demuxer still support FFM1 even though
> the muxer can't create such files anymore?

The muxer has created FFM1 files previously,
The muxer should not have created files with invalid values previously
though.
I dont know if no user would be affected if we drop FFM1 support

But with invalid values this is different, they should not have
been created and if they were created our muxer is or was buggy
we need to know about this so we can fix the muxer and stop more
invalid files from being created.
If we make our demuxer accept all invalid fields we would not find
out (no bug reports) about muxer bugs and would then not be able to
fix these

If there is a muxer which did create files with invalid values then
we should support these in the demuxer after fixing the muxer IMO


> 
> Best regards,
> Andreas
> 

>  ffmdec.c |   52 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 10 deletions(-)
> a565a670592ad6455e9f26e95edf9886b426dfb3  0001-ffmdec-validate-codec-parameters.patch
> From d3f33b108fe7035e1aa1bc7021dbb99aecd14821 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
> Date: Thu, 17 Nov 2016 00:04:57 +0100
> Subject: [PATCH] ffmdec: validate codec parameters
> 
> A negative extradata size for example gets passed to memcpy in
> avcodec_parameters_from_context causing a segmentation fault.

LGTM

thanks!

[...]
Andreas Cadhalpun Nov. 20, 2016, 7:33 p.m. UTC | #2
On 20.11.2016 03:41, Michael Niedermayer wrote:
> On Sun, Nov 20, 2016 at 12:37:05AM +0100, Andreas Cadhalpun wrote:
> 
>>  ffmdec.c |   52 ++++++++++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 42 insertions(+), 10 deletions(-)
>> a565a670592ad6455e9f26e95edf9886b426dfb3  0001-ffmdec-validate-codec-parameters.patch
>> From d3f33b108fe7035e1aa1bc7021dbb99aecd14821 Mon Sep 17 00:00:00 2001
>> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
>> Date: Thu, 17 Nov 2016 00:04:57 +0100
>> Subject: [PATCH] ffmdec: validate codec parameters
>>
>> A negative extradata size for example gets passed to memcpy in
>> avcodec_parameters_from_context causing a segmentation fault.
> 
> LGTM

Pushed.

Best regards,
Andreas
diff mbox

Patch

From d3f33b108fe7035e1aa1bc7021dbb99aecd14821 Mon Sep 17 00:00:00 2001
From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
Date: Thu, 17 Nov 2016 00:04:57 +0100
Subject: [PATCH] ffmdec: validate codec parameters

A negative extradata size for example gets passed to memcpy in
avcodec_parameters_from_context causing a segmentation fault.

Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com>
---
 libavformat/ffmdec.c | 52 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 10 deletions(-)

diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c
index 6b09be2..960e793 100644
--- a/libavformat/ffmdec.c
+++ b/libavformat/ffmdec.c
@@ -21,6 +21,7 @@ 
 
 #include <stdint.h>
 
+#include "libavutil/imgutils.h"
 #include "libavutil/internal.h"
 #include "libavutil/intreadwrite.h"
 #include "libavutil/intfloat.h"
@@ -28,6 +29,7 @@ 
 #include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
 #include "libavutil/pixdesc.h"
+#include "libavcodec/internal.h"
 #include "avformat.h"
 #include "internal.h"
 #include "ffm.h"
@@ -277,6 +279,14 @@  static int ffm_append_recommended_configuration(AVStream *st, char **conf)
     return 0;
 }
 
+#define VALIDATE_PARAMETER(parameter, name, check) {                              \
+    if (check) {                                                                  \
+        av_log(codec, AV_LOG_ERROR, "Invalid " name " %d\n", codec->parameter);   \
+        ret = AVERROR_INVALIDDATA;                                                \
+        goto fail;                                                                \
+    }                                                                             \
+}
+
 static int ffm2_read_header(AVFormatContext *s)
 {
     FFMContext *ffm = s->priv_data;
@@ -342,6 +352,7 @@  static int ffm2_read_header(AVFormatContext *s)
             if (!codec_desc) {
                 av_log(s, AV_LOG_ERROR, "Invalid codec id: %d\n", codec->codec_id);
                 codec->codec_id = AV_CODEC_ID_NONE;
+                ret = AVERROR_INVALIDDATA;
                 goto fail;
             }
             codec->codec_type = avio_r8(pb);
@@ -350,14 +361,25 @@  static int ffm2_read_header(AVFormatContext *s)
                        codec_desc->type, codec->codec_type);
                 codec->codec_id = AV_CODEC_ID_NONE;
                 codec->codec_type = AVMEDIA_TYPE_UNKNOWN;
+                ret = AVERROR_INVALIDDATA;
                 goto fail;
             }
             codec->bit_rate = avio_rb32(pb);
+            if (codec->bit_rate < 0) {
+                av_log(codec, AV_LOG_ERROR, "Invalid bit rate %"PRId64"\n", codec->bit_rate);
+                ret = AVERROR_INVALIDDATA;
+                goto fail;
+            }
             codec->flags = avio_rb32(pb);
             codec->flags2 = avio_rb32(pb);
             codec->debug = avio_rb32(pb);
             if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
                 int size = avio_rb32(pb);
+                if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
+                    av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size);
+                    ret = AVERROR_INVALIDDATA;
+                    goto fail;
+                }
                 codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
                 if (!codec->extradata)
                     return AVERROR(ENOMEM);
@@ -380,6 +402,9 @@  static int ffm2_read_header(AVFormatContext *s)
             }
             codec->width = avio_rb16(pb);
             codec->height = avio_rb16(pb);
+            ret = av_image_check_size(codec->width, codec->height, 0, s);
+            if (ret < 0)
+                goto fail;
             codec->gop_size = avio_rb16(pb);
             codec->pix_fmt = avio_rb32(pb);
             if (!av_pix_fmt_desc_get(codec->pix_fmt)) {
@@ -432,13 +457,11 @@  static int ffm2_read_header(AVFormatContext *s)
                 goto fail;
             }
             codec->sample_rate = avio_rb32(pb);
-            if (codec->sample_rate <= 0) {
-                av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate);
-                ret = AVERROR_INVALIDDATA;
-                goto fail;
-            }
+            VALIDATE_PARAMETER(sample_rate, "sample rate",        codec->sample_rate < 0)
             codec->channels = avio_rl16(pb);
+            VALIDATE_PARAMETER(channels,    "number of channels", codec->channels < 0)
             codec->frame_size = avio_rl16(pb);
+            VALIDATE_PARAMETER(frame_size,  "frame size",         codec->frame_size < 0)
             break;
         case MKBETAG('C', 'P', 'R', 'V'):
             if (f_cprv++) {
@@ -518,7 +541,7 @@  static int ffm_read_header(AVFormatContext *s)
     AVIOContext *pb = s->pb;
     AVCodecContext *codec;
     const AVCodecDescriptor *codec_desc;
-    int i, nb_streams;
+    int i, nb_streams, ret;
     uint32_t tag;
 
     /* header */
@@ -570,6 +593,10 @@  static int ffm_read_header(AVFormatContext *s)
             goto fail;
         }
         codec->bit_rate = avio_rb32(pb);
+        if (codec->bit_rate < 0) {
+            av_log(codec, AV_LOG_WARNING, "Invalid bit rate %"PRId64"\n", codec->bit_rate);
+            goto fail;
+        }
         codec->flags = avio_rb32(pb);
         codec->flags2 = avio_rb32(pb);
         codec->debug = avio_rb32(pb);
@@ -585,6 +612,8 @@  static int ffm_read_header(AVFormatContext *s)
             }
             codec->width = avio_rb16(pb);
             codec->height = avio_rb16(pb);
+            if (av_image_check_size(codec->width, codec->height, 0, s) < 0)
+                goto fail;
             codec->gop_size = avio_rb16(pb);
             codec->pix_fmt = avio_rb32(pb);
             if (!av_pix_fmt_desc_get(codec->pix_fmt)) {
@@ -633,18 +662,21 @@  static int ffm_read_header(AVFormatContext *s)
             break;
         case AVMEDIA_TYPE_AUDIO:
             codec->sample_rate = avio_rb32(pb);
-            if (codec->sample_rate <= 0) {
-                av_log(s, AV_LOG_ERROR, "Invalid sample rate %d\n", codec->sample_rate);
-                goto fail;
-            }
+            VALIDATE_PARAMETER(sample_rate, "sample rate",        codec->sample_rate < 0)
             codec->channels = avio_rl16(pb);
+            VALIDATE_PARAMETER(channels,    "number of channels", codec->channels < 0)
             codec->frame_size = avio_rl16(pb);
+            VALIDATE_PARAMETER(frame_size,  "frame size",         codec->frame_size < 0)
             break;
         default:
             goto fail;
         }
         if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
             int size = avio_rb32(pb);
+            if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
+                av_log(s, AV_LOG_ERROR, "Invalid extradata size %d\n", size);
+                goto fail;
+            }
             codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
             if (!codec->extradata)
                 return AVERROR(ENOMEM);
-- 
2.10.2