[FFmpeg-devel] [FFmpeg-cvslog] h264_metadata: Avoid integer overflow in bitrate

[Mark Thompson]

On 26/09/18 22:50, Carl Eugen Hoyos wrote:
> 2018-09-25 0:16 GMT+02:00, Mark Thompson <git at videolan.org>:
>> ffmpeg | branch: master | Mark Thompson <sw at jkqxz.net> | Mon Sep 24 22:45:50
>> 2018 +0100| [321294adb788b5e143fcec776cdf1daf79ed921c] | committer: Mark
>> Thompson
>>
>> h264_metadata: Avoid integer overflow in bitrate
>>
>> Fixes CID #1439664.
>>
>>> http://git.videolan.org/gitweb.cgi/ffmpeg.git/?a=commit;h=321294adb788b5e143fcec776cdf1daf79ed921c
>> ---
>>
>>  libavcodec/h264_metadata_bsf.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
>> index 991fcfa537..bf37528234 100644
>> --- a/libavcodec/h264_metadata_bsf.c
>> +++ b/libavcodec/h264_metadata_bsf.c
>> @@ -226,10 +226,10 @@ static int h264_metadata_update_sps(AVBSFContext *bsf,
>>
>>              if (sps->vui.nal_hrd_parameters_present_flag) {
>>                  bit_rate =
>> (sps->vui.nal_hrd_parameters.bit_rate_value_minus1[0] + 1) *
>> -                     (1 << (sps->vui.nal_hrd_parameters.bit_rate_scale +
>> 6));
> 
> How can this overflow?

bit_rate_value_minus1 can be up to UINT32_MAX - 1 and bit_rate_scale can be up to 15, so the product can get to just under 2^53.  While that isn't valid for any level, the fields come from untrusted user input.

>> +                    (INT64_C(1) <<
>> (sps->vui.nal_hrd_parameters.bit_rate_scale + 6));
>>              } else if (sps->vui.vcl_hrd_parameters_present_flag) {
>>                  bit_rate =
>> (sps->vui.vcl_hrd_parameters.bit_rate_value_minus1[0] + 1) *
>> -                     (1 << (sps->vui.vcl_hrd_parameters.bit_rate_scale +
>> 6));
> 
> Same here: Isn't the maximum (15+6) ?

While the first shift can't overflow, the whole expression can as noted above.  Coverity was complaining specifically about the shift rather than the whole expression, so it seemed sensible to put the int64_t cast here.

>> +                    (INT64_C(1) <<
>> (sps->vui.vcl_hrd_parameters.bit_rate_scale + 6));
>>                  // Adjust for VCL vs. NAL limits.
>>                  bit_rate = bit_rate * 6 / 5;
>>              } else {

Thanks,

- Mark