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

[Paul B Mahol]

On 5/11/18, Derek Buitenhuis <derek.buitenhuis at gmail.com> wrote:
>> 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.
>
> [...]

If you think you are fixing security issue you are very wrong.