[FFmpeg-devel] CBS

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

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
-------------- 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/20240806/48f131bf/attachment.sig>