[FFmpeg-devel] [PATCH 1/2] avcodec/hevc_ps: Check log2_sao_offset_scale_*

[Michael Niedermayer]

On Wed, Jan 24, 2018 at 11:42:44PM -0300, James Almer wrote:
> On 1/24/2018 11:03 PM, Michael Niedermayer wrote:
> > On Wed, Jan 24, 2018 at 12:47:18AM -0300, James Almer wrote:
> >> On 1/24/2018 12:34 AM, Michael Niedermayer wrote:
> >>> Fixes: 4868/clusterfuzz-testcase-minimized-6236542906400768
> >>> Fixes: runtime error: shift exponent 126 is too large for 32-bit type 'int'
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >>> ---
> >>>  libavcodec/hevc_ps.c | 11 +++++++++++
> >>>  1 file changed, 11 insertions(+)
> >>>
> >>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>> index 4787312cfa..746c96b17e 100644
> >>> --- a/libavcodec/hevc_ps.c
> >>> +++ b/libavcodec/hevc_ps.c
> >>> @@ -1324,6 +1324,17 @@ static int pps_range_extensions(GetBitContext *gb, AVCodecContext *avctx,
> >>>      pps->log2_sao_offset_scale_luma = get_ue_golomb_long(gb);
> >>>      pps->log2_sao_offset_scale_chroma = get_ue_golomb_long(gb);
> >>>  
> >>> +    if (   pps->log2_sao_offset_scale_luma   > FFMAX(sps->bit_depth        - 10, 0)
> >>> +        || pps->log2_sao_offset_scale_chroma > FFMAX(sps->bit_depth_chroma - 10, 0)
> >>> +    ) {
> >>> +        av_log(avctx, AV_LOG_ERROR,
> >>> +                "log2 sao offset scales (%d %d) are invalid\n",
> >>> +               pps->log2_sao_offset_scale_luma,
> >>> +               pps->log2_sao_offset_scale_chroma
> >>> +              );
> >>> +        return AVERROR_INVALIDDATA;
> >>
> >> Wouldn't it be better to just port the h264 and hevc decoder to use the
> >> cbs API at this point? It correctly does a range check for every
> >> sps/vps/pps/slice value already.
> >>
> >> Otherwise you'll be adding a lot of range checks as oss-fuzz finds an
> >> ubsan testcase for them.
> > 
> > cbs is not available in the releases
> > we need to fix issues in the releases
> > 
> > so i dont think cbs can help here
> 
> For release branches yes, no way around it, patches like this are
> needed.

Yes and fixes to releases are bascially always backports from master
and thats what this patchset does, fix it in master in a form that can
be hopefully backported with minimal issues.


> But for future releases it will prevent this kind of fix to be
> added as fuzzers find issues. Eventually, every supported release will
> be one using cbs where range checks are already implemented, so the
> quickest it's done the better.

when all release use cbs then we will still find bugs and will have to
fix them. Both in cbs and outside.
In case of hevc that should be fewer bugs as the hevc bit stream reading
seems to have been writen more sloppy than in other decoders.
Other decoders quite possibly will have more bugs in their cbs code than
before as cbs is alot of not very much tested code, while the decoders
have been tested and fuzzed quite extensivly


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180125/54f75c5e/attachment.sig>