[FFmpeg-devel] [PATCH] Checked get_bits.h functions to prevent overread

[Alexander Strasser]

Hi,

Laurent Aimar wrote:
>  After trying some fuzzing on libavcodec, it seems that a lot of decoders
> does not check (or not enough) for buffer overread which can lead for some
> to a segfault.
> 
>  I attached a patch that make get_bits.h function checked for overread by
> default but let safe decoders disabling the checks at compilation time by
> defining UNCHECK_BITSTREAM_READER before including get_bits.h.
>  If such patch would be including, I would gladly provide a patch
> adding the #define UNCHECK_BITSTREAM_READER to the decoder that are 'safe'.

  I like the idea of making things secure by default. So if we go
for a solution like this which allows decoders to be flagged safe,
we should also come up with a procedure that must be passed before
flagging decoders as safe to define UNCHECK_BITSTREAM_READER.

  Maybe something like: It must have passed a defined fuzzing test
(must be easy/clear for developers how to do that test) and must
pass an audit by at least one developer that is not the author of
the decoder.

[...]
> diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h
> index d2ae345..e3fedb9 100644
> --- a/libavcodec/get_bits.h
> +++ b/libavcodec/get_bits.h
> +#ifndef UNCHECK_BITSTREAM_READER
> +#   warn "Checked bistream reader unimplemented"
> +#endif

  After correcting the typo pointed out by Ronald, this sentence
still doesn't make sense to me. Given the double negation in the
condition it seems not clear or to even conveys the opposite of
the intended meaning. But maybe it is just too late at night and
you can easily show me wrong.

  Alexander