[FFmpeg-devel,1/8] lavc/cbs_h265: Disallow nonsensically large HVCC NAL arrays

Fixes CID 1419833.
---
 libavcodec/cbs_h2645.c | 5 +++++
 1 file changed, 5 insertions(+)

On 9/29/2019 1:45 PM, Mark Thompson wrote:
> Fixes CID 1419833.
> ---
>  libavcodec/cbs_h2645.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
> index 2dc261f7a5..185c458f61 100644
> --- a/libavcodec/cbs_h2645.c
> +++ b/libavcodec/cbs_h2645.c
> @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>          nb_arrays = bytestream2_get_byte(&gbc);
>          for (i = 0; i < nb_arrays; i++) {
>              nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f;
> +
>              nb_nals = bytestream2_get_be16(&gbc);
> +            if (nb_nals > 64) {

Why not check for the actual limit of each ps type instead? This code
will still try to parse the file if it reports more than 16 sps, for
example, despite it being invalid.

Maybe also check for nb_nals == 0.

> +                // Too many NALs of this type - the header must be invalid.
> +                return AVERROR_INVALIDDATA;
> +            }
>  
>              start = bytestream2_tell(&gbc);
>              for (j = 0; j < nb_nals; j++) {
>

On 29/09/2019 20:54, James Almer wrote:
> On 9/29/2019 1:45 PM, Mark Thompson wrote:
>> Fixes CID 1419833.
>> ---
>>  libavcodec/cbs_h2645.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>> index 2dc261f7a5..185c458f61 100644
>> --- a/libavcodec/cbs_h2645.c
>> +++ b/libavcodec/cbs_h2645.c
>> @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>>          nb_arrays = bytestream2_get_byte(&gbc);
>>          for (i = 0; i < nb_arrays; i++) {
>>              nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f;
>> +
>>              nb_nals = bytestream2_get_be16(&gbc);
>> +            if (nb_nals > 64) {
> 
> Why not check for the actual limit of each ps type instead?

Mainly because that would require a big switch statement to determine the limit - since it's an early check for invalid streams which will fail later anyway I didn't want to clutter the code too much.

>                                                             This code
> will still try to parse the file if it reports more than 16 sps, for
> example, despite it being invalid.
> 
> Maybe also check for nb_nals == 0.

Is that actually invalid rather than just pointless?  I don't have a specification for this, and hevc_parse.c also accepts it (not checking the count at all).

>> +                // Too many NALs of this type - the header must be invalid.
>> +                return AVERROR_INVALIDDATA;
>> +            }
>>  
>>              start = bytestream2_tell(&gbc);
>>              for (j = 0; j < nb_nals; j++) {
>>

Thanks,

- Mark

On 9/29/2019 5:01 PM, Mark Thompson wrote:
> On 29/09/2019 20:54, James Almer wrote:
>> On 9/29/2019 1:45 PM, Mark Thompson wrote:
>>> Fixes CID 1419833.
>>> ---
>>>  libavcodec/cbs_h2645.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c
>>> index 2dc261f7a5..185c458f61 100644
>>> --- a/libavcodec/cbs_h2645.c
>>> +++ b/libavcodec/cbs_h2645.c
>>> @@ -695,7 +695,12 @@ static int cbs_h2645_split_fragment(CodedBitstreamContext *ctx,
>>>          nb_arrays = bytestream2_get_byte(&gbc);
>>>          for (i = 0; i < nb_arrays; i++) {
>>>              nal_unit_type = bytestream2_get_byte(&gbc) & 0x3f;
>>> +
>>>              nb_nals = bytestream2_get_be16(&gbc);
>>> +            if (nb_nals > 64) {
>>
>> Why not check for the actual limit of each ps type instead?
> 
> Mainly because that would require a big switch statement to determine the limit - since it's an early check for invalid streams which will fail later anyway I didn't want to clutter the code too much.
> 
>>                                                             This code
>> will still try to parse the file if it reports more than 16 sps, for
>> example, despite it being invalid.
>>
>> Maybe also check for nb_nals == 0.
> 
> Is that actually invalid rather than just pointless?  I don't have a specification for this, and hevc_parse.c also accepts it (not checking the count at all).

Yeah, it would obviously be invalid data. The for loop below wouldn't
trigger in that case and both start and end will have the same value,
meaning ff_h2645_packet_split() will be called with a length of 0, which
apparently will not be considered an error and instead will just create
and empty H2645Packet.
If that's going to be an issue further in the fragment splitting
process, i don't know.

> 
>>> +                // Too many NALs of this type - the header must be invalid.
>>> +                return AVERROR_INVALIDDATA;
>>> +            }
>>>  
>>>              start = bytestream2_tell(&gbc);
>>>              for (j = 0; j < nb_nals; j++) {
>>>
> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>