[FFmpeg-devel] [RFC][PATCH] configure: Disable unsafe demuxers by default

[Derek Buitenhuis]

> please correct me if iam wrong, theres quite a bit iam guessing here
> IIUC the problem is that in your usecase
> 1. ffmpeg has access to sensitive files
> 2. one of these files can be opened by an attacker with ffmpeg
> 2b. This involves the file being probed as a supported format

It is "probed" as file extension mostly. .txt is one of these
extensions, which 99.999999999999999%
of the time, is not a multimedia file, for example.

> 3. The file is then demuxed, decoded, encoded, muxed
> 4. the result is given back to the attacker
>
> This patchset removes one of the demuxers involved in the attack

Not removed. Disabled. As discussed in other replies, making it manual
also seems
fine to me. Rendering arbitrary text files as images (as in, rendering
the text using
a built in font, to an image with that text in it) isn't exactly a
great and secure default
behavior, especially for the world's most commont text file extension...

> The first problem of this patchset is that it does not provide any
> evidence of how the other demuxers probe functions can trigger on
> random text files.

ffmpeg -i something.txt a.png

But really, it is not necssarily an attack on its own (it could be),
but it makes any
other attack vastly easier to exploit. For example, that HLS stuff
avformat had fixed
last year could have pointed at various .txt files. I don't really
understand why the
concept of "rendering arbitrary text files as images" is not obviously bad?

> for example idf_probe() requires, the first 12 bytes of the file  to
> match exactly and some of these are not text. So a attack which depends
> on the probing of sensitive text data to succeed should not work with it

it is not specifci to probing. It's choosing e.g. the ansi decoder based off the
.txt file extension, for example. Or the bintext decoder based off the
.bin extension.

> The second problem i see is that the patch disables the demuxers, while
> disabling the probe functions affected should have the same effect
> security wise but is a smaller step in respect to disabling.

As noted in my reply to Marton and Gyan, I'm fine with this approach,
as the implications
are the same.

> The third problem i see is that really once you read sensitive data
> and pass something back to the user you already lost.

[...]

> A text demuxer and decoder makes it easier and it makes it much easier
> to demonstrate the attack in a flashy way. But having the probe code
> access the sensitive data even if none succeeds already makes quite
> a bit of internal state (caches, branch prediction, deallocated memory, ...)
> be contaminated with sensitive information. Its hard to ensure that
> none of this can be recovered by the attacker.

Ah the classic "well it can technically be beaten, you may as well make it
as easy as possible" argument that people use to argue against things
like ASLR... :|

The point is to minimize the risk where possible, with good defaults for *all*
users. It's not a great thing to just point at users and go "well you
should have
done a better job containerizing/sandboxing/etc." IMO.

> I think first ffmpeg should not be able to access sensitive data.
> Then none of our probe functions should succeed on the average
> sensitive text file. If one does, we should look into making it not
> detect that.

The succeed because libavformat loves probing based on file extenions.

> Also it may seem counter-intuitive but adding a probe function which
> is designed to succeed on sensitive text files may be more reliable
> to stop their detection than to disable probe functions.

This is just adding more heuristics to probing, though

> The probe system is basically doing differential probing. That is
> the one of 2 that has the higher score wins.
> So it may be more effective to add a function that that returns a
> high score intentionally for a null demuxer than to try to stop
> every function that returns more than 0

Wouldn't this break a lot of currently "working" detection of
bad/broken files that
users seem to rely on? (I don't think they should rely on it, but
that's just my opinion.)

> if such probe code is added, it also could maybe warn the admin about a
> potential misconfiguration that allows probing to reach to sensitive
> data.

[...]

- Derek