[FFmpeg-devel] [PATCH] avcodec/cbs_h2645: keep separate parameter set lists for reading and writing

[James Almer]

On 7/1/2020 6:40 AM, Michael Niedermayer wrote:
> On Sun, Jun 07, 2020 at 12:45:15PM -0300, James Almer wrote:
>> On 6/7/2020 12:20 PM, James Almer wrote:
>>> On 6/7/2020 11:45 AM, Michael Niedermayer wrote:
>>>> On Sun, Jun 07, 2020 at 10:27:37AM -0300, James Almer wrote:
>>>>> Similar logic as 4e2bef6a82. In scearios where an Access Unit is written right
>>>>> after reading it using the same CBS context (hevc_metadata), the list of parsed
>>>>> parameters sets used by the writer must not be the one that's the result of the
>>>>> reader having already parsed the current Access Unit in question.
>>>>>
>>>>> Fixes: out of array access
>>>>> Fixes: 23034/clusterfuzz-testcase-minimized-ffmpeg_BSF_HEVC_METADATA_fuzzer-5074645169733632.fuzz
>>>>>
>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>> ---
>>>>> An alternative is forcing the usage of separate CBS contexts for reading and
>>>>> writing.
>>>>>
>>>>>  libavcodec/cbs_h264.h  | 18 ++++++++---
>>>>>  libavcodec/cbs_h2645.c | 72 ++++++++++++++++++++++++++++++++----------
>>>>>  libavcodec/cbs_h265.h  | 26 +++++++++++----
>>>>>  3 files changed, 89 insertions(+), 27 deletions(-)
>>>>
>>>> i think the change is probably ok and it fixes the issue
>>>> what i feel uneasy about is if this is the sole way the security
>>>> issue is fixed.
>>>>
>>>> let me try to explain what i mean by a simpler example:
>>>> if you have a sprintf() that overwrites the buffer there are 3 ways
>>>> to fix that
>>>> A. You make the buffer big enough for what is written
>>>> B. You make the amount written only as large as the buffer
>>>> C. You check by using snprintf()
>>>>
>>>> Now like here A/B may represent a bugfix
>>>> the problem with A/B is that security rests on potentially complex code
>>>> So even when A/B is done, we also should do C
>>>>
>>>> This patch fixes the inconsistency on the write side be keeping more references
>>>> to the parameter sets.
>>>> For security one would have to proof that no crafted input to the reader
>>>> fed into any available useer of the reader could result in an inconsistency
>>>> to a writer following.
>>>> Are you sure thats the case now and with future users of the code ?
>>>> OTOH as dumb as a check in the writer may look, anyone can proof it fixes the
>>>> specific inconsistency.
>>>>
>>>> What i suggest specifically is to also include or apply the simple check
>>>> on top of this, or a equivalent / more generic check. So that security does not
>>>> rest on alot of spread out code
>>>>
>>>> Thanks
>>>
>>> Well, one possibility is that after this, the infer() warning could be
>>> replaced with an assert() instead. The CBS framework is not public, so
>>> crashing with an assert() would be acceptable as infer() failing in
>>> writing scenarios should be considered an internal bug (bitstream
>>> filters, or any CBS user for that matter, should ensure to not change
>>> fields in a way that would result in an invalid bitstream and thus
>>> failing infer() checks).
>>>
>>> The issue shouldn't be treated as "If inter_ref_pic_set_prediction_flag
>>> is 1 in this scenario, then we should stop to avoid out of array
>>> access", but as "We did something wrong because
>>> inter_ref_pic_set_prediction_flag was absolutely not meant to be 1 at
>>> this point". So using assert() after this patch sounds like a good
>>> solution and will help detect future bugs in the parsing code.
>>
>> It could also be a generic return AVERROR_INVALIDDATA as you suggested,
>> to be fair. All the standard writing helpers abort gracefully that way,
>> so infer() could do the same.
>>
>> Or the other helpers could be changed to assert().
> 
> whats the status of this ?
> has this issue been fixed in some other way i missed ?

I pushed the first two patches and backported them, so the issue should
be fixed by the first.

> will this get applied ?

I delayed applying this one waiting for more opinions, especially
Mark's, since it's kind of ugly.

> should i apply my not so pretty fix for 23034 ?
> what will be done about releases ? can this be backported ?

Already answered above, but maybe confirm it's fixed just to be sure.

> 
> PS: please just make sure 23034/ is mentioned in the commit message so
> whatever fixes it can be kept track of (i know its already mentioned
> this is more intended as a remainder for other alternative fixes)

Ah, guess i should have mentioned that ossfuzz issue in ef13fafe22 (and
e6ab99f324by extension) seeing i didn't push this one alongside it...

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>