[FFmpeg-devel] [PATCH 3/3] avcodec/cfhd: More strictly check tag order and multiplicity

[Paul B Mahol]

On Wed, Dec 23, 2020 at 3:59 PM Michael Niedermayer <michael at niedermayer.cc>
wrote:

> On Wed, Dec 23, 2020 at 10:52:03AM +0100, Paul B Mahol wrote:
> > On Tue, Dec 22, 2020 at 10:27 PM Michael Niedermayer
> <michael at niedermayer.cc>
> > wrote:
> >
> > > On Sun, Dec 20, 2020 at 10:18:40PM +0100, Paul B Mahol wrote:
> > > > Unacceptable, please share privately sample that allows to reproduce
> > > this.
> > >
> > > shared the ones which reproduce.
> > >
> > > Please explain why this patch is unacceptable to you.
> > >
> > > the CFHD decoder decodes header elements in the order in which they are
> > > stored. The problem is that many have interdependancies yet there are
> > > no checks for these. And where there are checks theres no protection
> > > against changing dependancies after they have been used.
> > > Basically CFHD allows an attacker to do absolutely anything
> > >
> > > To pick a random example:
> > > the code reading the SubbandNumber adjusts the level and then
> > > checks its range based on transform_type. Yet transform_type
> > > may be not set yet or may be subsequently changed.
> > > That is issue 27872
> > >
> > > One surely can try to add specific checks for all this but i doubt that
> > > will
> > > result in secure code anytime soon. Its IMO better to fundamentally
> > > fix this and not allow anything to occur in any multiplicity and order.
> > > My posted patch is one way of many possible alternatives to move in
> that
> > > direction
> > >
> > >
> > Well, you forgot that when you check for order of tags, you basically
> still
> > allow
> > crash to happen, just this time crashing code path needs to follow
> correct
> > parsing order.
>
> I dont see how that would be possible
>
> with the correct order of tags
> transform_type is set and checked and can after that only be 0 or 2
> the crash required it to be -1. As transform_type is marked as a mandatory
> element in the table and -1 is not a possible value after it i dont see
> how that could work.
> But maybe iam missing something
>

Transform type is always set, so that one can always be checked that it is
set to correct value.
But above code is too complex for my alternative fix.