[FFmpeg-devel] [PATCH 2/3] avformat: reject FFmpeg-style merged side data in raw packets

Michael Niedermayer michael at niedermayer.cc
Thu Mar 9 14:12:17 EET 2017

On Thu, Mar 09, 2017 at 12:16:09PM +0100, Nicolas George wrote:
> Le nonidi 19 ventôse, an CCXXV, Michael Niedermayer a écrit :
> > This is very basic really but lets elaborate
> > for each side data type T
> > possiblity A
> > nothing uses side data type T
> > 
> > possiblity B
> > something uses side data type T
> > 
> > Its the same with a codec, either a codec is used in some case or
> > its used in no case.
> > 
> > If something is used in no case then it has been eliminated as you
> > describe.
> > If somehing is still used in a case it has not been eliminated
> > 
> > If as you describe side data has been eliminated then you could
> > remove side data as a whole from the source code.
> > 
> > If you cannot remove side data or a specific side data type from
> > the source code then it has not been eliminated
> > 
> > your change removes one way for an attacker to set side data but
> > by the fact that you dont remove any of the side data types its
> > clear you are aware of that every is still in use in some code path.
> > 
> > a attacker may need to use a specific container format to set a
> > specific side data type or may depend on a specific demuxer lib or
> > application that allows him to set a side data type.
> > 
> > now if you remove every way to set side data for an attacker then
> > you can remove that side data type as a whole from the code.
> > Of course that removes whatever the side data is for.
> > 
> > Let me provide a specific example
> > If a container suports changing extradata mid stream it will either
> > be support or not.
> > if any demuxer supports it then you have not eliminated the possiblity
> > for an attacker
> > 
> > I hope writing a elaborate reply will not lead to this discussion
> > to shift onto some unrelated detail
> You are rehashing a lot of obvious facts, but you do not address the
> important questions.
> Side data is useful. It is a badly designed API, because it adds a lot
> of complexity for a hypothetical benefit but fails to reap that benefit.
> As such, it should be kept but enhanced, either by removing the
> complexity or by actually reaping the benefit.

> But it is not the topic of this discussion.

> MERGED side data is a completely brainded design that should never have
> been written.

Iam not sure its on topic, but it gave FFmpeg an advantage
over our competition at the time when this code was added.

> I am purposefully not looking at the archive to find out who is
> responsible for this mess. Now is not the time to point fingers but to
> fix the code.
> Now, please answer this very specific question:

> If someone were to REMOVE ALL AND EVERY use of
> av_packet_merge_side_data() and av_packet_split_side_data(), what would
> be the actual bad consequences?

* All applications using libavformat and libavcodec would need to
preserve the side data between the layers, they currently do not
have to. Thats also a API/ABI change requiring a major bump

* Any testcases that used this to inject side data will no longer
  function as regression tests

* It reduces (but does not eliminate) the ways by which a fuzzer can
  inject side data.

> But before you start with fuzzing or anything similar, let me stop you:
> fuzzing exposes bugs that can be triggered by crafted inputs.

> If fuzzing
> can not trigger it, that means the bug does not exist, period.

This may be formally correct if you have infinite time but we dont.

A specific example would be a raw h264 file used as input to the fuzzer
with the split code the fuzzer can inject side data by adding a
8 byte value, fuzzers actually manage to do this

without split code the fuzzer can still inject side data but it needs
to build the right container around the h264 frames, like a matroska
file that supports the specific type of side data.
This is much harder for a fuzzer to do
So the fuzzer then could only do that with cases that are mkv files
to begin with which reduces the attack surface probed by the fuzzer


Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/20170309/91aeddd1/attachment.sig>

More information about the ffmpeg-devel mailing list