[FFmpeg-devel] CBS

[Michael Niedermayer]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240807/469c2b57/attachment.sig>