[FFmpeg-devel] [PATCH] avcodec: validate codec parameters in avcodec_parameters_to_context

[Michael Niedermayer]

On Wed, Oct 26, 2016 at 01:59:59AM +0200, Andreas Cadhalpun wrote:
> On 25.10.2016 14:56, Hendrik Leppkes wrote:
> > On Tue, Oct 25, 2016 at 2:39 PM, wm4 <nfxjfg at googlemail.com> wrote:
> >> On Tue, 25 Oct 2016 09:47:29 +0200
> >> Hendrik Leppkes <h.leppkes at gmail.com> wrote:
> >>
> >>> On Tue, Oct 25, 2016 at 1:50 AM, Andreas Cadhalpun
> >>> <andreas.cadhalpun at googlemail.com> wrote:
> >>>> This should reduce the impact of a demuxer (or API user) setting bogus
> >>>> codec parameters.
> >>>>
> >>>>
> >>>
> >>> This seems rather noisy
> 
> I've added a macro to make it less noisy.
> 
> >>> and doesn't really solve anything, does it?
> 
> As you analyze below, it was fixing only a part of the problem.
> 
> >>> Decoders still need to validate values instead of blindly trusting
> >>> them, and this just hides some problems in these decoders,
> 
> Yes, the problem hiding is bad, which is why I added av_assert2's
> so that developers can easily check if the validation fails.
> 
> >>> instead of
> >>> fixing them there. API users of avcodec would not fill
> >>> AVCodecParameters, they would fill a codec context directly.
> >>
> >> You could also argue that the demuxer shouldn't return invalid
> >> parameters either.
> > 
> > It should not, but this patch does not address this.
> 
> Indeed, the demuxers should be fixed in addition to this patch.
> 
> > There is various combinations of component usage that are possible,
> > and in fact are used in the real world:
> > 
> > avformat -> avcodec
> > other demuxer -> avcodec
> > avformat -> other decoder
> > 
> > This patch only addresses the first case, and only if you actually use
> > this function (which I for example do not, since I have an abstraction
> > layer in between, so I never have AVCodecParameters and AVCodecContext
> > in the same function).
> > So in short, it just doesn't fix much, and you can still get invalid
> > output from avformat, and potentially still undefined behavior in
> > avcodec if its fed those values through other means.
> 
> For the third case, the demuxers have to be fixed. Having the asserts
> in the central code makes it much easier to find these problems.
> 
> >> How about this: always convert the params to a temporary codecpar, and
> >> provide a function to determine the validity of a codecpar. This way
> >> the check could be done in multiple places without duplicating the code
> >> needed for it.
> > 
> > That still sounds odd, although slightly better. At the very least it
> > should be a dedicated function that checks the values in a key place,
> > say you want to check params that are fed to a decoder, then call this
> > check in avcodec_open, because thats something everyone has to call to
> > use avcodec.
> 
> I tried to implement the suggestions of both of you, see attached patch.
> 
> Note that the added asserts are triggered by *many* of my fuzzed samples.
> I'm happy to write patches for these cases, if we achieve agreement
> that the central check alone is insufficient.
> Another noteworthy point is that this patch makes it easy to trigger
> the av_assert0 in iff_read_packet. I think that assert should be replaced
> with an error return.
> 
> Best regards,
> Andreas
> 

>  utils.c |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 2 deletions(-)
> 40a8bafecb6d289a4220a27ac411fbcac3204952  0001-avcodec-validate-codec-parameters.patch
> From f371be7a027da3958e221b4dc88ad558c1489107 Mon Sep 17 00:00:00 2001
> From: Andreas Cadhalpun <Andreas.Cadhalpun at googlemail.com>
> Date: Tue, 25 Oct 2016 01:45:27 +0200
> Subject: [PATCH] avcodec: validate codec parameters
> 
> This should reduce the impact of a broken demuxer (or API user) setting bogus
> codec parameters.
> 
> The av_assert2 calls should ease detecting broken demuxers.

have you tried a fuzzer ?
these assertions fail on fuzzed files

Assertion 0 failed at libavcodec/utils.c:4157
Aborted
Assertion !((unsigned)par->color_primaries > AVCOL_PRI_NB) failed at libavcodec/utils.c:4161

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161027/e9608fde/attachment.sig>