[FFmpeg-devel] CBS

[FFmpeg-devel] CBS

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1283 bytes --]

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 ?

thx

--
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Dictatorship naturally arises out of democracy, and the most aggravated
form of tyranny and slavery out of the most extreme liberty. -- Plato

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1828 bytes --]

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 ?

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.

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Andreas Rheinhardt]

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
more so with spec-incompliant input, which is presumably what the fuzzer
produced).

- Andreas

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[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?

> more so with spec-incompliant input, which is presumably what the fuzzer
> produced).
> 
> - Andreas
> 
> _______________________________________________
> 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".
_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Andreas Rheinhardt]

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.

> 
>> more so with spec-incompliant input, which is presumably what the fuzzer
>> produced).
>>

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4129 bytes --]

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.

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Any man who breaks a law that conscience tells him is unjust and willingly 
accepts the penalty by staying in jail in order to arouse the conscience of 
the community on the injustice of the law is at that moment expressing the 
very highest respect for law. - Martin Luther King Jr

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [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

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 4885 bytes --]

On Tue, Aug 06, 2024 at 09:51:10PM +0200, Andreas Rheinhardt wrote:
> 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.

how is "Just refer to SPS for parsing" overlooking the SPS for parsing?

The problem is not the parsing, the problem is the writing doesnt match
the parsing. The simple fix i refered to is to store the parsed value and use it when
writing. Not to somehow hope that a network of invalid and attacker controlled
data structures, some of which might contain different values at different times
will look the same to teh writer than the parser.

Thats what i meant but you can fix it anyway you want.


Thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Homeopathy is like voting while filling the ballot out with transparent ink.
Sometimes the outcome one wanted occurs. Rarely its worse than filling out
a ballot properly.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".

Re: [FFmpeg-devel] CBS

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2879 bytes --]

On Tue, Aug 06, 2024 at 07:54:58PM +0200, 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.

Then you qualify as maintainer for this code.
Thanks!


> 
> > 
> > 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.

Thats what i came up with too (and it works). Given we both hit that,
please apply


> 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
> more so with spec-incompliant input, which is presumably what the fuzzer
> produced).

Thats a deeper analysis than what i did. I stoped at the "just clear it, as in 1."
and then was looking for someone who would implement the correct fix

Ill send you the input sample privately

Thanks alot!

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The educated differ from the uneducated as much as the living from the
dead. -- Aristotle 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".