[FFmpeg-devel] Overflow check for frame_size in v4l.c

[Stefano Sabatini]

On date Saturday 2008-12-27 17:16:05 +0100, Michael Niedermayer encoded:
> On Sat, Dec 27, 2008 at 03:12:59PM +0100, Stefano Sabatini wrote:
> > On date Saturday 2008-12-27 14:36:37 +0100, Michael Niedermayer encoded:
[...]
> > > > > I think the check is insufficient and more not less checking is needed
> > > > > 
> > > > >  frame_size = s->video_win.width * s->video_win.height * video_formats[j].depth / 8;
> > > > > 
> > > > > will not work with 32767*32767*...
> > > > 
> > > > OK, 32767 = 2^15 -1.
> > > > 
> > > > We may then check for 16383 = 2^14 -1 (check the patch below), or
> > > > maybe some function like these ones may help:
> > > 
> > > avcodec_check_dimensions()
> > 
> > I don't think it is a good idea to use that function here, its domain
> 
> its exactly what the function is designed to check.

/**
 * Checks if the given dimension of a picture is valid, meaning that all
 * bytes of the picture can be addressed with a signed int.
 *
 * @param[in] w Width of the picture.
 * @param[in] h Height of the picture.
 * @return Zero if valid, a negative value if invalid.
 */
int avcodec_check_dimensions(void *av_log_ctx, unsigned int w, unsigned int h);

The definition is promising, yet I'm missing the part which deals with
the pixel depth.

Then reading the code my confusion increased, as I can't figure out
the meaning of the code (what I -- naively -- expected was something like:
w * h < (INT_MAX -1 ) / MAX_DEPTH) (I guess those 128s are for
alignment reasons?):

int avcodec_check_dimensions(void *av_log_ctx, unsigned int w, unsigned int h){
    if((int)w>0 && (int)h>0 && (w+128)*(uint64_t)(h+128) < INT_MAX/4)
        return 0;

    av_log(av_log_ctx, AV_LOG_ERROR, "picture size invalid (%ux%u)\n", w, h);
    return -1;
}

And I agree, if this function may be used here then it's the way to
go.

> > is very specific and here we have to check the result of a
> > multiplication with *three* operands.
> > 
> > I propose three possible choices:
> > 
> 
> > 1) Implement a stricter check.
> > 
> >    Since we have
> >    frame_size =  s->video_win.width * s->video_win.height * video_formats[j].depth / 8;
> > 
> >    and video_formats[j].depth / 8 is at maximum 3, then we have:
> > 
> >    X * X * 3 < 2 ^ 31 -1;
> > 
> >    that is:
> >    X * X < (2^31 - 1) / 3
> > 
> >    max_X = tail (sqrt ((2^31 - 1) / 3)) = 26754
> > 
> >    so we could check for width/heigth <= 26754
> > 
> >    Or if we want to be more prudent, we can replace 3 with 8, then we have
> >    max_X = 16383.
> > 
> 
> this would reject cases that could never cause a problem, though iam not
> against it, its better than what we have currently.

Since 4 seems a good candidate as the max pixel depth in bytes, I think
that some comment and the value 23170 would be a good tradeoff.
 
> > 2) Introduce some generic function in libavutil such as av_safe_mul32()
> >    as proposed in the previous post and use it.
> 
> if you want to check for an overflow, check for it:
> 
> if(a*(uint64_t)b > INT_MAX)
>     error
> something(a*b);
> 
> 
> using this silly av_safe_mul32() will only make the code more messy:
> 
> int32_t tmp;
> if(av_safe_mul32(&tmp, a, b) < 0)
>     error;
> something(tmp);

Yes, though for more than two operands it still could be useful.

> > 
> > 3) Leave the check as it is, if we're lucky no one will ever have any
> >    problem with the non-strict-enough check, since valid values for
> >    height and width are unlikely to generate an overflow.
> 
> I would prefer if we dont deal with security holes that way.

I agree.

Regards.
-- 
FFmpeg = Fast Fierce Magical Powerful Everlasting Guru