[FFmpeg-devel] [PATCH 2/4] avcodec/hevc_ps: Fix integer overflow with num_tile_rows

[Michael Niedermayer]

On Thu, Jun 20, 2019 at 01:04:45AM -0300, James Almer wrote:
> On 6/19/2019 3:59 PM, James Almer wrote:
> > On 6/19/2019 3:13 PM, Michael Niedermayer wrote:
> >> On Wed, Jun 19, 2019 at 12:54:25PM -0300, James Almer wrote:
> >>> On 6/19/2019 6:22 AM, Michael Niedermayer wrote:
> >>>> On Mon, Jun 17, 2019 at 07:55:45PM -0300, James Almer wrote:
> >>>>> On 6/17/2019 6:54 PM, Michael Niedermayer wrote:
> >>>>>> On Sun, Jun 16, 2019 at 11:10:43PM -0300, James Almer wrote:
> >>>>>>> On 6/13/2019 3:32 PM, Michael Niedermayer wrote:
> >>>>>>>> Fixes: signed integer overflow: -2147483648 - 1 cannot be represented in type 'int'
> >>>>>>>> Fixes: 14880/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_HEVC_fuzzer-5130977304641536
> >>>>>>>>
> >>>>>>>> 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 | 2 +-
> >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/libavcodec/hevc_ps.c b/libavcodec/hevc_ps.c
> >>>>>>>> index 80df417e4f..0ed6682bb4 100644
> >>>>>>>> --- a/libavcodec/hevc_ps.c
> >>>>>>>> +++ b/libavcodec/hevc_ps.c
> >>>>>>>> @@ -1596,7 +1596,7 @@ int ff_hevc_decode_nal_pps(GetBitContext *gb, AVCodecContext *avctx,
> >>>>>>>>          if (pps->num_tile_rows <= 0 ||
> >>>>>>>>              pps->num_tile_rows >= sps->height) {
> >>>>>>>>              av_log(avctx, AV_LOG_ERROR, "num_tile_rows_minus1 out of range: %d\n",
> >>>>>>>> -                   pps->num_tile_rows - 1);
> >>>>>>>> +                   pps->num_tile_rows - 1U);
> >>>>>>>
> >>>>>>> The proper fix for this is making pps->num_tile_rows/cols unsigned. 
> >>>> [...]
> >>>>>>
> >>>>>> is this here ok if num_tile_rows is 0 ?
> >>>>>> for (i = 0; i < pps->num_tile_rows - 1; i++) { (example line from ffmpeg git)
> >>>>>>
> >>>>>> i would guess nearly everyone wold say yes without having seen the
> >>>>>> discussion about the type. but of course if this is unsigned its not
> >>>>>> going to be safe with it being 0. 
> >>>>>
> >>>>> pps->num_tile_rows is set to a value returned by "get_ue_golomb_long() +
> >>>>> 1", which will always be in the 1..UINT32_MAX range. It can't be 0, as
> >>>>> it would be a bug. Int is definitely not the right type for it.
> >>>>
> >>>> i dont think num_tile_rows of more than 2^31 (that is the negative when signed range)
> >>>> makes much sense but sure i can change it to unsigned if preferred.
> >>>
> >>> Of course it doesn't. In pretty much any sample it will be at least 1
> >>> and at most 22, which is the max value allowed by hevc level 6.2 in
> >>> table A.6. Only if the stream reports an undefined level it could go up
> >>> to, if i'm reading this right, sps->ctb_height and not sps->height as
> >>> the decoder is currently checking. This means you can even replace
> >>> get_ue_golomb_long() for a get_ue_golomb(). That would also fix this.
> >>>
> >>> In any case, the bitstream value is unsigned, so the struct field should
> >>> be unsigned as well. Having it be signed and assigning it a value using
> >>> a function that potentially returns huge unsigned values introduced this
> >>> issue to being with, so instead of working around it using type
> >>> promotion, just make it follow the spec.
> >>
> >> what would be your feeling/oppinon about making the field uint16_t ?
> >> this would make it unsigned in the struct but avoid the problems with
> >> unsigned int ?
> > 
> > That's fine. Could even make it uint8_t, like cbs_h265.h does. I'd also
> > change it to use get_ue_golomb() while at it. And please do it for both
> > rows and columns. Can be in separate patches if you want the rows one to
> > explicitly address the fuzzing testcase.
> > 

> > Can you also confirm my suspicion that the checks should be comparing
> > the values with sps->ctb_height/weight and not with sps->height/weight?
> 
> Ok, so yes, it is. Should be changed in a separate patch as well.

will post it together with the other updated patch

thanks

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20190625/4fb664ac/attachment.sig>