[FFmpeg-devel] [PATCH 3/3] avformat: set the default whitelist to disable hls

Nicolas George george at nsup.org
Mon Jun 5 18:33:29 EEST 2017

Le septidi 17 prairial, an CCXXV, Michael Niedermayer a écrit :
> thats "ad hominem"

I am sorry, I did not realize it was, please forgive me and allow me to

The pattern is: someone is made aware of a minor security exploit in
parts of the code not their direct responsibility. Nonetheless, that
someone rushes and designs and implements a fix that only blocks that
particular exploit and its close cousins without fixing the actual
underlying security issue, and that has a lot of drawbacks on top of
that. I think I am allowed to call that a "bad fix".

We had two instances quoted in this discussion. I think there were
others (possibly not security related) but I cannot pinpoint them.

In both cases, the someone was you. This is not ad-hominem, just a fact,
and part of the pattern.

Maybe you have a tendency to behave that way, but that is not for me to
say, and it would really be ad-hominem. Let us not talk about it.

Maybe your special place in the project pushes you to doing that. I hope
you agree this is not ad-hominem. Maybe ad-positionem.

Maybe people notice it more when you are doing it.

Maybe it was just a coincidence.

Maybe a little bit of each.

Anyway, let us agree to all try and avoid that pattern and not dwell on

> You dont need to convince me that the extension check or changes
> within just hls are not a complete solution. Iam quite well aware
> of this. This is intended to stop an existing exploit and variants of
> it in practice and do so quickly.

It depends on the severity of the threat. This one seems quite minor and
far-fetched, and thus I think we could take our time to fix it properly.
We all have noticed that temporary quick-and-dirty fixes usually stay
here a long time unless whoever implemented them is actively working on
a real fix.

Now, as I said, I have no special knowledge making me a good candidate
for working on a fix. I know how Perl's taint check works, but it will
not suit our needs. I know vaguely how Firefox avoids cross-site
information leak, but not in enough detail to serve as a base for a
design. I know nothing about Windows security zones.

Still, after having spent a few hours in a train without proper network
access, and with someone actually interested in listening, I could give
it some thought.

I will start by explaining why protocol_whitelist is bad.

First, it is a string. Enough with strings used as data structures! That
is inefficient, inconvenient and a maintenance nightmare since extending
or fixing the syntax will likely be considered an API break by some.

Second, the issue never was about protocols. ~/tmp/playlist.m3u8 should
not be allowed to access ../.firefox/cookies.txt (Firefox protects us
from that by adding a salt in the path, but not all sensitive files are
protected that way, this is only an illustrative example), and that
applies to local files, to SMB shares or to SFTP clients;
http://www.example.com/playlist.m3u8 should not be allowed to access
http://localhost:631/, yet they are both HTTP.

The issue is about subsets of the URL space. Files from one URL should
be allowed to access data from URLs in the same relevant subset (same
subdirectory or same web server maybe?), but not outside.

Of course, the protocol is part of the URL. But not all protocols are
like that: file://, http://, ssh:// are part of the URL, while concat:,
crypto:, subfile: are not and should be transparent for the mechanism. I
will call the formers "URL protocols" and the laters "utility

So let us get rid of that protocol_whitelist and replace it with an
actual data structure, designed to encode a subset of the URL space. Let
us call it AVSecurityClearance, at least for the rest of this mail.

Now, there is something done right with protocol_whitelist: it is, as it
should be, a property of muxers, demuxers and protocols (and anything
else that would be allowed to access external data) and must be
inherited when a component opens another component (demuxer -> protocol,
or demuxer -> slave demuxer, or protocol -> slave protocol, etc.).
AVSecurityClearance should work the same way.

Now, how should we implement that data structure? I do not know!

Maybe we can restrict it to subsets that are a sub-hierarchy: "anything
below http://www.example.com/music/".

But I suspect AVSecurityClearance will need to be polymorphic: the
structure of data is not necessarily the same for all protocols.

If AVSecurityClearance is polymorphic, then implementing union and
intersection operators is quite easy.

A few considerations about hierarchy. Some protocols have a host part.
The syntax of DNS domains is hierarchical from right to left, unlike the
syntax for file paths. It must be handled separately. The port part
needs a special treatment too: which ones are closer to
http://example.com/: http://www.example.com/ or
http://example.com:8080/? Or maybe they are to be considered all

Also, URLs with numerical IP addresses require a special treatment; we
do not want to implement a whois client, so we will have to treat them
as completely separate.

Another design question: when a master opens a slave and the
AVSecurityClearance is inherited, should it be copied, like
protocol_whitelist, or shared? If it is shared, later restrictions
caused by data access in one component are immediately propagated to all
the components. I suspect it is the right choice. But then,
AVSecurityClearance must be refcounted.

This is all I have for now. And I will not have time to work on this
anytime soon. But somebody needs to.

Of course, whoever works on it can come up with their own ideas. But I
suspect any working solution will work in the ways I just outlined.


  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170605/5d355592/attachment.sig>

More information about the ffmpeg-devel mailing list