[FFmpeg-devel] CBS

[Andreas Rheinhardt]

Michael Niedermayer:
> On Tue, Aug 06, 2024 at 08:41:16PM +0200, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 8/6/2024 2:54 PM, Andreas Rheinhardt wrote:
>>>> Michael Niedermayer:
>>>>> On Tue, Aug 06, 2024 at 07:05:38PM +0200, Michael Niedermayer wrote:
>>>>>> Hi
>>>>>>
>>>>>> Did CBS win the obfuscated C contest yet?
>>>>>>
>>>>>> I was just looking at a msan issue and then looked at this:
>>>>>>
>>>>>> CHECK(FUNC_SEI(message_list)(ctx, rw, &current->message_list, 1));
>>>>>>
>>>>>>
>>>>>> #define CHECK(call) do { \
>>>>>>          err = (call); \
>>>>>>          if (err < 0) \
>>>>>>              return err; \
>>>>>>      } while (0)
>>>>>>
>>>>>> #define FUNC_NAME2(rw, codec, name) cbs_ ## codec ## _ ## rw ## _ ##
>>>>>> name
>>>>>> #define FUNC_NAME1(rw, codec, name) FUNC_NAME2(rw, codec, name)
>>>>>> #define FUNC_H264(name) FUNC_NAME1(READWRITE, h264, name)
>>>>>> #define FUNC_H265(name) FUNC_NAME1(READWRITE, h265, name)
>>>>>> #define FUNC_H266(name) FUNC_NAME1(READWRITE, h266, name)
>>>>>> #define FUNC_SEI(name)  FUNC_NAME1(READWRITE, sei,  name)
>>>>>>
>>>>>> #define SEI_FUNC(name, args) \
>>>>>> static int FUNC(name) args;  \
>>>>>> static int FUNC(name ## _internal)(CodedBitstreamContext *ctx, \
>>>>>>                                     RWContext *rw, void *cur,   \
>>>>>>                                     SEIMessageState *state)     \
>>>>>> { \
>>>>>>      return FUNC(name)(ctx, rw, cur, state); \
>>>>>> } \
>>>>>> static int FUNC(name) args
>>>>>>
>>>>>>
>>>>>> anyway, can we remove all preprocessor use from cbs ?
>>>>
>>>> I don't think that this is really obfuscated.
>>>>
>>>>>
>>>>> the issue iam looking at is due to
>>>>>
>>>>> SEI_FUNC(sei_pic_timing, (CodedBitstreamContext *ctx, RWContext *rw,
>>>>> H264RawSEIPicTiming *current, SEIMessageState *sei))
>>>>>
>>>>> having different active SPS on writing than reading, so the write code
>>>>> has nal_hrd_parameters_present_flag set while the read had that 0
>>>>> so uninitialized data is written
>>>>>
>>>>> I cannot find any match for "cbs" in MAINTAINERS, also there are no
>>>>> copyright
>>>>> with names in the cbs code.
>>>>
>>>> 1. I just sent a patch that "fixes" this.
>>>> 2. But actually, there is a deeper bug here: We would need to defer
>>>> parsing certain SEI message units to a second pass when the currently
>>>> active SPS is known. This can happen with spec-compliant input (and even
>>>
>>> Is this a scenario where a slice that referenced an SPS was parsed, then
>>> a SEI message, then another slice that references another SPS, and the
>>> SEI expects the latter to be active despite it being coded before the
>>> slice?
>>
>> Your example is not spec-compliant  (at least not if the input is
>> supposed to only contain a single access unit). It can happen if there
>> is an SEI, then new parameter sets and then a slice activating these new
>> parameter sets. Or it can happen if there are multiple parameter sets
>> and an SEI followed by a slice activating a different SPS. The current
>> code is also wrong when parsing the first packet: Because in this case
>> no SPS has been activated yet, film_grain and sei_pic_timing SEI parsing
>> code contains a hack to make this work in the common case of a single SPS.
> 
> Having an attacker be able to supply SPS, SPS activation and SPS use in
> an atacker choosen order. While these elements are not self contained but
> refer to each other is just hard to do without being exploitable.
> 
> One way to fix this is to make the sei self contained and not refer to the SPS
> at all. Just refer to SPS for parsing but then when storing just store the fields
> that where parsed not depending on the right SPS being (still) there.

This overlooks that one already needs the correct SPS for parsing.

- Andreas