Message ID | 4c67b844-070a-fbbc-5f99-4bf7c8e510d7@googlemail.com |
---|---|
State | New |
Headers | show |
On Wed, Oct 26, 2016 at 01:59:59AM +0200, Andreas Cadhalpun wrote: > On 25.10.2016 14:56, Hendrik Leppkes wrote: > > On Tue, Oct 25, 2016 at 2:39 PM, wm4 <nfxjfg@googlemail.com> wrote: > >> On Tue, 25 Oct 2016 09:47:29 +0200 > >> Hendrik Leppkes <h.leppkes@gmail.com> wrote: > >> > >>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun > >>> <andreas.cadhalpun@googlemail.com> wrote: > >>>> This should reduce the impact of a demuxer (or API user) setting bogus > >>>> codec parameters. > >>>> > >>>> > >>> > >>> This seems rather noisy > > I've added a macro to make it less noisy. > > >>> and doesn't really solve anything, does it? > > As you analyze below, it was fixing only a part of the problem. > > >>> Decoders still need to validate values instead of blindly trusting > >>> them, and this just hides some problems in these decoders, > > Yes, the problem hiding is bad, which is why I added av_assert2's > so that developers can easily check if the validation fails. > > >>> instead of > >>> fixing them there. API users of avcodec would not fill > >>> AVCodecParameters, they would fill a codec context directly. > >> > >> You could also argue that the demuxer shouldn't return invalid > >> parameters either. > > > > It should not, but this patch does not address this. > > Indeed, the demuxers should be fixed in addition to this patch. > > > There is various combinations of component usage that are possible, > > and in fact are used in the real world: > > > > avformat -> avcodec > > other demuxer -> avcodec > > avformat -> other decoder > > > > This patch only addresses the first case, and only if you actually use > > this function (which I for example do not, since I have an abstraction > > layer in between, so I never have AVCodecParameters and AVCodecContext > > in the same function). > > So in short, it just doesn't fix much, and you can still get invalid > > output from avformat, and potentially still undefined behavior in > > avcodec if its fed those values through other means. > > For the third case, the demuxers have to be fixed. Having the asserts > in the central code makes it much easier to find these problems. > > >> How about this: always convert the params to a temporary codecpar, and > >> provide a function to determine the validity of a codecpar. This way > >> the check could be done in multiple places without duplicating the code > >> needed for it. > > > > That still sounds odd, although slightly better. At the very least it > > should be a dedicated function that checks the values in a key place, > > say you want to check params that are fed to a decoder, then call this > > check in avcodec_open, because thats something everyone has to call to > > use avcodec. > > I tried to implement the suggestions of both of you, see attached patch. > > Note that the added asserts are triggered by *many* of my fuzzed samples. > I'm happy to write patches for these cases, if we achieve agreement > that the central check alone is insufficient. > Another noteworthy point is that this patch makes it easy to trigger > the av_assert0 in iff_read_packet. I think that assert should be replaced > with an error return. > > Best regards, > Andreas > > utils.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 80 insertions(+), 2 deletions(-) > 40a8bafecb6d289a4220a27ac411fbcac3204952 0001-avcodec-validate-codec-parameters.patch > From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 > From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> > Date: Tue, 25 Oct 2016 01:45:27 +0200 > Subject: [PATCH] avcodec: validate codec parameters > > This should reduce the impact of a broken demuxer (or API user) setting bogus > codec parameters. > > The av_assert2 calls should ease detecting broken demuxers. have you tried a fuzzer ? these assertions fail on fuzzed files Assertion 0 failed at libavcodec/utils.c:4157 Aborted Assertion !((unsigned)par->color_primaries > AVCOL_PRI_NB) failed at libavcodec/utils.c:4161 [...]
On 27.10.2016 22:14, Michael Niedermayer wrote: > On Wed, Oct 26, 2016 at 01:59:59AM +0200, Andreas Cadhalpun wrote: >> Note that the added asserts are triggered by *many* of my fuzzed samples. >> I'm happy to write patches for these cases, if we achieve agreement >> that the central check alone is insufficient. Have you seen this comment? >> utils.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 80 insertions(+), 2 deletions(-) >> 40a8bafecb6d289a4220a27ac411fbcac3204952 0001-avcodec-validate-codec-parameters.patch >> From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 >> From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> >> Date: Tue, 25 Oct 2016 01:45:27 +0200 >> Subject: [PATCH] avcodec: validate codec parameters >> >> This should reduce the impact of a broken demuxer (or API user) setting bogus >> codec parameters. >> >> The av_assert2 calls should ease detecting broken demuxers. > > have you tried a fuzzer ? > these assertions fail on fuzzed files > > Assertion 0 failed at libavcodec/utils.c:4157 > Aborted > Assertion !((unsigned)par->color_primaries > AVCOL_PRI_NB) failed at libavcodec/utils.c:4161 As noted above, I'm well aware of that. This just shows how many demuxers currently set bogus values... Best regards, Andreas
From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001 From: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> Date: Tue, 25 Oct 2016 01:45:27 +0200 Subject: [PATCH] avcodec: validate codec parameters This should reduce the impact of a broken demuxer (or API user) setting bogus codec parameters. The av_assert2 calls should ease detecting broken demuxers. Suggested-by: wm4 Signed-off-by: Andreas Cadhalpun <Andreas.Cadhalpun@googlemail.com> --- libavcodec/utils.c | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/libavcodec/utils.c b/libavcodec/utils.c index 87de15f..9773b0b 100644 --- a/libavcodec/utils.c +++ b/libavcodec/utils.c @@ -1243,6 +1243,7 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code int ret = 0; AVDictionary *tmp = NULL; const AVPixFmtDescriptor *pixdesc; + AVCodecParameters *par = NULL; if (avcodec_is_open(avctx)) return 0; @@ -1259,8 +1260,14 @@ int attribute_align_arg avcodec_open2(AVCodecContext *avctx, const AVCodec *code if (!codec) codec = avctx->codec; - if (avctx->extradata_size < 0 || avctx->extradata_size >= FF_MAX_EXTRADATA_SIZE) - return AVERROR(EINVAL); + par = avcodec_parameters_alloc(); + if (!par) + return AVERROR(ENOMEM); + /* check if the codec parameters in the context are valid */ + ret = avcodec_parameters_from_context(par, avctx); + avcodec_parameters_free(&par); + if (ret < 0) + return ret; if (options) av_dict_copy(&tmp, *options, 0); @@ -4124,6 +4131,66 @@ static void codec_parameters_reset(AVCodecParameters *par) par->level = FF_LEVEL_UNKNOWN; } +#define CHECK_PARAMETER(parameter, name, check) { \ + av_assert2(!(check)); \ + if (check) { \ + av_log(avctx, AV_LOG_ERROR, "Invalid " name " %d\n", par->parameter); \ + return AVERROR(EINVAL); \ + } \ +} + +static int codec_parameters_check(const AVCodecContext *codec, const AVCodecParameters *par) +{ + AVCodecContext *avctx = (AVCodecContext *)codec; + if (par->bit_rate < 0) { + av_log(avctx, AV_LOG_ERROR, "Invalid bit rate %"PRId64"\n", par->bit_rate); + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(bits_per_coded_sample, "bits per coded sample", par->bits_per_coded_sample < 0) + CHECK_PARAMETER(bits_per_raw_sample, "bits per raw sample", par->bits_per_raw_sample < 0) + CHECK_PARAMETER(extradata_size, "extradata size", par->extradata_size < 0 || par->extradata_size >= FF_MAX_EXTRADATA_SIZE) + + switch (par->codec_type) { + case AVMEDIA_TYPE_VIDEO: + if ( (par->width || par->height) && av_image_check_size(par->width, par->height, 0, avctx) < 0) { + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(color_range, "color range", (unsigned)par->color_range > AVCOL_RANGE_NB) + CHECK_PARAMETER(color_primaries, "color primaries", (unsigned)par->color_primaries > AVCOL_PRI_NB) + CHECK_PARAMETER(color_trc, "color transfer characteristics ", (unsigned)par->color_trc > AVCOL_PRI_NB) + CHECK_PARAMETER(color_space, "color space", (unsigned)par->color_space > AVCOL_SPC_NB) + CHECK_PARAMETER(chroma_location, "chroma location", (unsigned)par->chroma_location > AVCHROMA_LOC_NB) + if (par->sample_aspect_ratio.num < 0 || par->sample_aspect_ratio.den < 0) { + av_log(avctx, AV_LOG_ERROR, "Invalid sample aspect ratio %d/%d\n", + par->sample_aspect_ratio.num, par->sample_aspect_ratio.den); + av_assert2(0); + return AVERROR(EINVAL); + } + CHECK_PARAMETER(video_delay, "video delay", par->video_delay < 0) + break; + case AVMEDIA_TYPE_AUDIO: + CHECK_PARAMETER(format, "sample format", par->format < -1 || par->format > AV_SAMPLE_FMT_NB) + CHECK_PARAMETER(channels, "number of channels", par->channels < 0) + CHECK_PARAMETER(sample_rate, "sample rate", par->sample_rate < 0) + CHECK_PARAMETER(block_align, "block alignment", par->block_align < 0) + CHECK_PARAMETER(frame_size, "frame size", par->frame_size < 0) + CHECK_PARAMETER(initial_padding, "initial padding", par->initial_padding < 0) + CHECK_PARAMETER(trailing_padding, "trailing padding", par->trailing_padding < 0) + CHECK_PARAMETER(seek_preroll, "seek preroll", par->seek_preroll < 0) + break; + case AVMEDIA_TYPE_SUBTITLE: + if ((par->width || par->height) && av_image_check_size(par->width, par->height, 0, avctx) < 0) { + av_assert2(0); + return AVERROR(EINVAL); + } + break; + } + return 0; +} + + AVCodecParameters *avcodec_parameters_alloc(void) { AVCodecParameters *par = av_mallocz(sizeof(*par)); @@ -4166,6 +4233,7 @@ int avcodec_parameters_copy(AVCodecParameters *dst, const AVCodecParameters *src int avcodec_parameters_from_context(AVCodecParameters *par, const AVCodecContext *codec) { + int ret; codec_parameters_reset(par); par->codec_type = codec->codec_type; @@ -4217,12 +4285,22 @@ int avcodec_parameters_from_context(AVCodecParameters *par, par->extradata_size = codec->extradata_size; } + ret = codec_parameters_check(codec, par); + if (ret < 0) { + codec_parameters_reset(par); + return ret; + } + return 0; } int avcodec_parameters_to_context(AVCodecContext *codec, const AVCodecParameters *par) { + int ret = codec_parameters_check(codec, par); + if (ret < 0) + return ret; + codec->codec_type = par->codec_type; codec->codec_id = par->codec_id; codec->codec_tag = par->codec_tag; -- 2.9.3