diff mbox

[FFmpeg-devel] ffmdec: sanitize codec parameters

Message ID 8cef9df2-9cc2-663f-bdbe-21484c1d1823@googlemail.com
State Superseded
Headers show

Commit Message

Andreas Cadhalpun Nov. 17, 2016, 6:35 p.m. UTC 
On 17.11.2016 02:26, Michael Niedermayer wrote:
> On Thu, Nov 17, 2016 at 01:08:31AM +0100, Andreas Cadhalpun wrote:
>> +    SANITIZE_PARAMETER(pix_fmt,                "pixel format",                    codec->pix_fmt < AV_PIX_FMT_NONE || codec->pix_fmt > AV_PIX_FMT_NB,             AV_PIX_FMT_NONE)
>> +    SANITIZE_PARAMETER(bits_per_coded_sample,  "bits per coded sample",           codec->bits_per_coded_sample < 0,                                               0)
>> +    SANITIZE_PARAMETER(bits_per_raw_sample,    "bits per raw sample",             codec->bits_per_raw_sample < 0,                                                 0)
>> +    SANITIZE_PARAMETER(extradata_size,         "extradata size",                  codec->extradata_size < 0 || codec->extradata_size >= FF_MAX_EXTRADATA_SIZE,    0)
>> +    SANITIZE_PARAMETER(color_range,            "color range",                     (unsigned)codec->color_range > AVCOL_RANGE_NB,                                  AVCOL_RANGE_UNSPECIFIED)
>> +    SANITIZE_PARAMETER(color_primaries,        "color primaries",                 (unsigned)codec->color_primaries > AVCOL_PRI_NB,                                AVCOL_PRI_UNSPECIFIED)
>> +    SANITIZE_PARAMETER(color_trc,              "color transfer characteristics ", (unsigned)codec->color_trc > AVCOL_TRC_NB,                                      AVCOL_TRC_UNSPECIFIED)
>> +    SANITIZE_PARAMETER(colorspace,             "color space",                     (unsigned)codec->colorspace > AVCOL_SPC_NB,                                     AVCOL_SPC_UNSPECIFIED)
>> +    SANITIZE_PARAMETER(chroma_sample_location, "chroma location",                 (unsigned)codec->chroma_sample_location > AVCHROMA_LOC_NB,                      AVCHROMA_LOC_UNSPECIFIED)
>> +    SANITIZE_PARAMETER(has_b_frames,           "video delay",                     codec->has_b_frames < 0,                                                        0)
>> +    SANITIZE_PARAMETER(sample_fmt,             "sample format",                   codec->sample_fmt < AV_SAMPLE_FMT_NONE || codec->sample_fmt > AV_SAMPLE_FMT_NB, AV_SAMPLE_FMT_NONE)
> 
> This breaks API/ABI

You mean this uses private API/ABI.

> for example AVCOL_SPC_NB is not part of the public API of libavutil

But it's already used in e.g. libavcodec/options_table.h -- which reminds
me that this is a much better place to sanitize options.
I'll send a separate patch doing that. Attached is an updated version
of this patch.

> one should be able to use av_color_space_name() to detect invalid color
> spaces

Indeed, that would have worked, too.

Best regards,
Andreas

Comments

Michael Niedermayer Nov. 18, 2016, 1:40 a.m. UTC | #1 
On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote:
> On 17.11.2016 02:26, Michael Niedermayer wrote:
> > On Thu, Nov 17, 2016 at 01:08:31AM +0100, Andreas Cadhalpun wrote:
> >> +    SANITIZE_PARAMETER(pix_fmt,                "pixel format",                    codec->pix_fmt < AV_PIX_FMT_NONE || codec->pix_fmt > AV_PIX_FMT_NB,             AV_PIX_FMT_NONE)
> >> +    SANITIZE_PARAMETER(bits_per_coded_sample,  "bits per coded sample",           codec->bits_per_coded_sample < 0,                                               0)
> >> +    SANITIZE_PARAMETER(bits_per_raw_sample,    "bits per raw sample",             codec->bits_per_raw_sample < 0,                                                 0)
> >> +    SANITIZE_PARAMETER(extradata_size,         "extradata size",                  codec->extradata_size < 0 || codec->extradata_size >= FF_MAX_EXTRADATA_SIZE,    0)
> >> +    SANITIZE_PARAMETER(color_range,            "color range",                     (unsigned)codec->color_range > AVCOL_RANGE_NB,                                  AVCOL_RANGE_UNSPECIFIED)
> >> +    SANITIZE_PARAMETER(color_primaries,        "color primaries",                 (unsigned)codec->color_primaries > AVCOL_PRI_NB,                                AVCOL_PRI_UNSPECIFIED)
> >> +    SANITIZE_PARAMETER(color_trc,              "color transfer characteristics ", (unsigned)codec->color_trc > AVCOL_TRC_NB,                                      AVCOL_TRC_UNSPECIFIED)
> >> +    SANITIZE_PARAMETER(colorspace,             "color space",                     (unsigned)codec->colorspace > AVCOL_SPC_NB,                                     AVCOL_SPC_UNSPECIFIED)
> >> +    SANITIZE_PARAMETER(chroma_sample_location, "chroma location",                 (unsigned)codec->chroma_sample_location > AVCHROMA_LOC_NB,                      AVCHROMA_LOC_UNSPECIFIED)
> >> +    SANITIZE_PARAMETER(has_b_frames,           "video delay",                     codec->has_b_frames < 0,                                                        0)
> >> +    SANITIZE_PARAMETER(sample_fmt,             "sample format",                   codec->sample_fmt < AV_SAMPLE_FMT_NONE || codec->sample_fmt > AV_SAMPLE_FMT_NB, AV_SAMPLE_FMT_NONE)
> > 
> > This breaks API/ABI
> 
> You mean this uses private API/ABI.
> 
> > for example AVCOL_SPC_NB is not part of the public API of libavutil
> 
> But it's already used in e.g. libavcodec/options_table.h -- which reminds
> me that this is a much better place to sanitize options.
> I'll send a separate patch doing that. Attached is an updated version
> of this patch.
> 
> > one should be able to use av_color_space_name() to detect invalid color
> > spaces
> 
> Indeed, that would have worked, too.
> 
> Best regards,
> Andreas
> 

>  ffmdec.c |  105 +++++++++++++++++++++++++++++++++++++--------------------------
>  1 file changed, 62 insertions(+), 43 deletions(-)
> 72925e00379c208f03b9fc3fcd82d0615fc5904d  0001-ffmdec-sanitize-codec-parameters.patch
> From 9bc15dfba44533c9e0f2cc54eec519b359f7f0c5 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: sanitize 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 | 105 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 62 insertions(+), 43 deletions(-)
> 
> diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c
> index 6b09be2..d5e10b0 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,13 @@ static int ffm_append_recommended_configuration(AVStream *st, char **conf)
>      return 0;
>  }
>  
> +#define SANITIZE_PARAMETER(parameter, name, check, default) {                     \
> +    if (check) {                                                                  \
> +        av_log(codec, AV_LOG_WARNING, "Invalid " name " %d\n", codec->parameter); \
> +        codec->parameter = default;                                               \
> +    }                                                                             \
> +}
> +
>  static int ffm2_read_header(AVFormatContext *s)
>  {
>      FFMContext *ffm = s->priv_data;
> @@ -346,23 +355,29 @@ static int ffm2_read_header(AVFormatContext *s)
>              }
>              codec->codec_type = avio_r8(pb);
>              if (codec->codec_type != codec_desc->type) {
> -                av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n",
> +                av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n",
>                         codec_desc->type, codec->codec_type);
> -                codec->codec_id = AV_CODEC_ID_NONE;
> -                codec->codec_type = AVMEDIA_TYPE_UNKNOWN;
> -                goto fail;
> +                codec->codec_type = codec_desc->type;
>              }
>              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);
> +                codec->bit_rate = 0;
> +            }
>              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);
> -                codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
> -                if (!codec->extradata)
> -                    return AVERROR(ENOMEM);
> -                codec->extradata_size = size;
> -                avio_read(pb, codec->extradata, size);
> +                if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
> +                    av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size);

i think this and possibly others should be a hard failure
or why would a invalid extradata_size be ok ?

[...]
Andreas Cadhalpun Nov. 18, 2016, 9:35 p.m. UTC | #2 
On 18.11.2016 02:40, Michael Niedermayer wrote:
> On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote:
>> +                if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
>> +                    av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size);
> 
> i think this and possibly others should be a hard failure
> or why would a invalid extradata_size be ok ?

The decoder might still be able to work.
What would be the advantage of a hard failure?

Best regards,
Andreas
Michael Niedermayer Nov. 19, 2016, 1:14 a.m. UTC | #3 
On Fri, Nov 18, 2016 at 10:35:29PM +0100, Andreas Cadhalpun wrote:
> On 18.11.2016 02:40, Michael Niedermayer wrote:
> > On Thu, Nov 17, 2016 at 07:35:01PM +0100, Andreas Cadhalpun wrote:
> >> +                if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
> >> +                    av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size);
> > 
> > i think this and possibly others should be a hard failure
> > or why would a invalid extradata_size be ok ?
> 
> The decoder might still be able to work.
> What would be the advantage of a hard failure?

the advantage of stoping clearly invalid data early
The decoder runs on a seperate machine with ffm possibly. The file
just gets demuxed and sent over the network. The warning hinting to
the issue is on the sending side. The decoder failure at the receiver
i didnt try but if iam not mixing things up that would be confusing
neither side would fully understand whats going on without the other

Does this corrupted but working data occur in practice?


[...]
diff mbox

Patch

From 9bc15dfba44533c9e0f2cc54eec519b359f7f0c5 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: sanitize 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 | 105 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 62 insertions(+), 43 deletions(-)

diff --git a/libavformat/ffmdec.c b/libavformat/ffmdec.c
index 6b09be2..d5e10b0 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,13 @@  static int ffm_append_recommended_configuration(AVStream *st, char **conf)
     return 0;
 }
 
+#define SANITIZE_PARAMETER(parameter, name, check, default) {                     \
+    if (check) {                                                                  \
+        av_log(codec, AV_LOG_WARNING, "Invalid " name " %d\n", codec->parameter); \
+        codec->parameter = default;                                               \
+    }                                                                             \
+}
+
 static int ffm2_read_header(AVFormatContext *s)
 {
     FFMContext *ffm = s->priv_data;
@@ -346,23 +355,29 @@  static int ffm2_read_header(AVFormatContext *s)
             }
             codec->codec_type = avio_r8(pb);
             if (codec->codec_type != codec_desc->type) {
-                av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n",
+                av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n",
                        codec_desc->type, codec->codec_type);
-                codec->codec_id = AV_CODEC_ID_NONE;
-                codec->codec_type = AVMEDIA_TYPE_UNKNOWN;
-                goto fail;
+                codec->codec_type = codec_desc->type;
             }
             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);
+                codec->bit_rate = 0;
+            }
             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);
-                codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
-                if (!codec->extradata)
-                    return AVERROR(ENOMEM);
-                codec->extradata_size = size;
-                avio_read(pb, codec->extradata, size);
+                if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
+                    av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size);
+                } else {
+                    codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
+                    if (!codec->extradata)
+                        return AVERROR(ENOMEM);
+                    codec->extradata_size = size;
+                    avio_read(pb, codec->extradata, size);
+                }
             }
             break;
         case MKBETAG('S', 'T', 'V', 'I'):
@@ -372,21 +387,21 @@  static int ffm2_read_header(AVFormatContext *s)
             }
             codec->time_base.num = avio_rb32(pb);
             codec->time_base.den = avio_rb32(pb);
-            if (codec->time_base.num <= 0 || codec->time_base.den <= 0) {
-                av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n",
+            if (codec->time_base.num < 0 || codec->time_base.den <= 0) {
+                av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n",
                        codec->time_base.num, codec->time_base.den);
-                ret = AVERROR_INVALIDDATA;
-                goto fail;
+                codec->time_base.num = 0;
+                codec->time_base.den = 1;
             }
             codec->width = avio_rb16(pb);
             codec->height = avio_rb16(pb);
+            if (av_image_check_size(codec->width, codec->height, 0, s) < 0) {
+                codec->width = 0;
+                codec->height = 0;
+            }
             codec->gop_size = avio_rb16(pb);
             codec->pix_fmt = avio_rb32(pb);
-            if (!av_pix_fmt_desc_get(codec->pix_fmt)) {
-                av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt);
-                codec->pix_fmt = AV_PIX_FMT_NONE;
-                goto fail;
-            }
+            SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE)
             codec->qmin = avio_r8(pb);
             codec->qmax = avio_r8(pb);
             codec->max_qdiff = avio_r8(pb);
@@ -432,13 +447,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;
-            }
+            SANITIZE_PARAMETER(sample_rate, "sample rate",        codec->sample_rate < 0, 0)
             codec->channels = avio_rl16(pb);
+            SANITIZE_PARAMETER(channels,    "number of channels", codec->channels < 0, 0)
             codec->frame_size = avio_rl16(pb);
+            SANITIZE_PARAMETER(frame_size,  "frame size",         codec->frame_size < 0, 0)
             break;
         case MKBETAG('C', 'P', 'R', 'V'):
             if (f_cprv++) {
@@ -563,13 +576,15 @@  static int ffm_read_header(AVFormatContext *s)
         }
         codec->codec_type = avio_r8(pb); /* codec_type */
         if (codec->codec_type != codec_desc->type) {
-            av_log(s, AV_LOG_ERROR, "Codec type mismatch: expected %d, found %d\n",
+            av_log(s, AV_LOG_WARNING, "Codec type mismatch: expected %d, found %d\n",
                    codec_desc->type, codec->codec_type);
-            codec->codec_id = AV_CODEC_ID_NONE;
-            codec->codec_type = AVMEDIA_TYPE_UNKNOWN;
-            goto fail;
+            codec->codec_type = codec_desc->type;
         }
         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);
+            codec->bit_rate = 0;
+        }
         codec->flags = avio_rb32(pb);
         codec->flags2 = avio_rb32(pb);
         codec->debug = avio_rb32(pb);
@@ -579,19 +594,20 @@  static int ffm_read_header(AVFormatContext *s)
             codec->time_base.num = avio_rb32(pb);
             codec->time_base.den = avio_rb32(pb);
             if (codec->time_base.num <= 0 || codec->time_base.den <= 0) {
-                av_log(s, AV_LOG_ERROR, "Invalid time base %d/%d\n",
+                av_log(s, AV_LOG_WARNING, "Invalid time base %d/%d\n",
                        codec->time_base.num, codec->time_base.den);
-                goto fail;
+                codec->time_base.num = 0;
+                codec->time_base.den = 1;
             }
             codec->width = avio_rb16(pb);
             codec->height = avio_rb16(pb);
+            if (av_image_check_size(codec->width, codec->height, 0, s) < 0) {
+                codec->width = 0;
+                codec->height = 0;
+            }
             codec->gop_size = avio_rb16(pb);
             codec->pix_fmt = avio_rb32(pb);
-            if (!av_pix_fmt_desc_get(codec->pix_fmt)) {
-                av_log(s, AV_LOG_ERROR, "Invalid pix fmt id: %d\n", codec->pix_fmt);
-                codec->pix_fmt = AV_PIX_FMT_NONE;
-                goto fail;
-            }
+            SANITIZE_PARAMETER(pix_fmt, "pixel format", codec->pix_fmt != AV_PIX_FMT_NONE && !av_pix_fmt_desc_get(codec->pix_fmt), AV_PIX_FMT_NONE)
             codec->qmin = avio_r8(pb);
             codec->qmax = avio_r8(pb);
             codec->max_qdiff = avio_r8(pb);
@@ -633,23 +649,26 @@  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;
-            }
+            SANITIZE_PARAMETER(sample_rate, "sample rate",        codec->sample_rate < 0, 0)
             codec->channels = avio_rl16(pb);
+            SANITIZE_PARAMETER(channels,    "number of channels", codec->channels < 0, 0)
             codec->frame_size = avio_rl16(pb);
+            SANITIZE_PARAMETER(frame_size,  "frame size",         codec->frame_size < 0, 0)
             break;
         default:
             goto fail;
         }
         if (codec->flags & AV_CODEC_FLAG_GLOBAL_HEADER) {
             int size = avio_rb32(pb);
-            codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
-            if (!codec->extradata)
-                return AVERROR(ENOMEM);
-            codec->extradata_size = size;
-            avio_read(pb, codec->extradata, size);
+            if (size < 0 || size >= FF_MAX_EXTRADATA_SIZE) {
+                av_log(s, AV_LOG_WARNING, "Invalid extradata size %d\n", size);
+            } else {
+                codec->extradata = av_mallocz(size + AV_INPUT_BUFFER_PADDING_SIZE);
+                if (!codec->extradata)
+                    return AVERROR(ENOMEM);
+                codec->extradata_size = size;
+                avio_read(pb, codec->extradata, size);
+            }
         }
 
         avcodec_parameters_from_context(st->codecpar, codec);
-- 
2.10.2