[FFmpeg-devel] [PATCH 1/2] avcodec: Add AV_CODEC_FLAG2_FAST_UNSAFE, move unsafe uses of FAST to it

[James Almer]

On 5/28/2020 1:20 PM, Michael Niedermayer wrote:
> TODO: Bump
> 
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  doc/APIchanges             | 3 +++
>  doc/codecs.texi            | 2 ++
>  libavcodec/avcodec.h       | 6 ++++++
>  libavcodec/h264dec.c       | 2 +-
>  libavcodec/options_table.h | 1 +
>  tools/target_dec_fuzzer.c  | 2 +-
>  6 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index fb5534b5f5..3e20a44379 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -15,6 +15,9 @@ libavutil:     2017-10-21
>  
>  API changes, most recent first:
>  
> +2020-xx-xx - xxxxxxxxxx - lavc 58.xx.100 - avcodec.h
> +  Add AV_CODEC_FLAG2_FAST_UNSAFE
> +
>  2020-xx-xx - xxxxxxxxxx - lavc 58.88.100 - avcodec.h codec.h
>    Move AVCodec-related public API to new header codec.h.
>  
> diff --git a/doc/codecs.texi b/doc/codecs.texi
> index c092aadc0e..46790b66b3 100644
> --- a/doc/codecs.texi
> +++ b/doc/codecs.texi
> @@ -787,6 +787,8 @@ Possible values:
>  @table @samp
>  @item fast
>  Allow non spec compliant speedup tricks.
> + at item fast_unsafe
> +Allow speedup tricks which can lead to out of array reads and crashes on damaged or crafted files.

This will raise more than a couple eyebrows. Having an option to enable
what people will consider security issues is not a good idea at all. For
starters, it acknowledges lavc is not secure and has known issues that
are purposely not being fixed. And on top of it, this can't be outright
disabled/removed at compile time, so something could still call
ffmpeg/lavc with it enabled.

The issues should be fixed, or the relevant "fast" codepath in the
decoder removed for being buggy.

>  @item noout
>  Skip bitstream encoding.
>  @item ignorecrop
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 01099bc8cd..479f219b43 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -346,6 +346,12 @@ typedef struct RcOverride{
>   * Allow non spec compliant speedup tricks.
>   */
>  #define AV_CODEC_FLAG2_FAST           (1 <<  0)
> +
> +/**
> + * Allow speedups tricks which can read out of array on non compliant streams.
> + */
> +#define AV_CODEC_FLAG2_FAST_UNSAFE    (1 <<  1)
> +
>  /**
>   * Skip bitstream encoding.
>   */
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index e463fde2a5..b764caa942 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -603,7 +603,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size)
>      }
>  
>      ret = ff_h2645_packet_split(&h->pkt, buf, buf_size, avctx, h->is_avc, h->nal_length_size,
> -                                avctx->codec_id, avctx->flags2 & AV_CODEC_FLAG2_FAST, 0);
> +                                avctx->codec_id, avctx->flags2 & AV_CODEC_FLAG2_FAST_UNSAFE, 0);
>      if (ret < 0) {
>          av_log(avctx, AV_LOG_ERROR,
>                 "Error splitting the input into NAL units.\n");
> diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
> index 8ba137f51e..4e26b844f6 100644
> --- a/libavcodec/options_table.h
> +++ b/libavcodec/options_table.h
> @@ -71,6 +71,7 @@ static const AVOption avcodec_options[] = {
>  {"drop_changed", "Drop frames whose parameters differ from first decoded frame", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG_DROPCHANGED }, INT_MIN, INT_MAX, A|V|D, "flags"},
>  {"flags2", NULL, OFFSET(flags2), AV_OPT_TYPE_FLAGS, {.i64 = DEFAULT}, 0, UINT_MAX, V|A|E|D|S, "flags2"},
>  {"fast", "allow non-spec-compliant speedup tricks", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_FAST }, INT_MIN, INT_MAX, V|E, "flags2"},
> +{"fast_unsafe", "allow speedup tricks which can read out of arrays", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_FAST_UNSAFE }, INT_MIN, INT_MAX, V|E, "flags2"},
>  {"noout", "skip bitstream encoding", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_NO_OUTPUT }, INT_MIN, INT_MAX, V|E, "flags2"},
>  {"ignorecrop", "ignore cropping information from sps", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_IGNORE_CROP }, INT_MIN, INT_MAX, V|D, "flags2"},
>  {"local_header", "place global headers at every keyframe instead of in extradata", 0, AV_OPT_TYPE_CONST, {.i64 = AV_CODEC_FLAG2_LOCAL_HEADER }, INT_MIN, INT_MAX, V|E, "flags2"},
> diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c
> index d01deaf8d5..414cdab593 100644
> --- a/tools/target_dec_fuzzer.c
> +++ b/tools/target_dec_fuzzer.c
> @@ -208,7 +208,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
>              if (flags & 8)
>                  ctx->err_recognition |= AV_EF_EXPLODE;
>          }
> -        if ((flags & 0x10) && c->id != AV_CODEC_ID_H264)
> +        if (flags & 0x10)
>              ctx->flags2 |= AV_CODEC_FLAG2_FAST;
>  
>          if (flags & 0x40)
>