[FFmpeg-devel] The "bad" Patch

[Mark Thompson]

On 31/05/2025 12:44, softworkz . wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of softworkz .
>> Sent: Donnerstag, 29. Mai 2025 04:59
>> To: FFmpeg development discussions and patches <ffmpeg-devel at ffmpeg.org>
>> Subject: Re: [FFmpeg-devel] The "bad" Patch
> 
> 
> Two and a half days have passed and nobody has answered any of the questions
> from my previous post. This speaks for itself.
> 
> What really happened, is that some had seen my use of system() and 
> since this is commonly known as a "dangerous" API when not used carefully,
> they did lazy judgement without due diligence and without detailed 
> assessment.
> The judgement was based on commonplace knowledge instead: 
> system() => is-bad => patch is bad
> 
> 
> Looking at the specific case and way of usage (and how system() works
> internally) would have revealed that this doesn't apply here. 
> 
> => The patch was NOT bad at all
> => All review comments were addressed when it was pushed
> 
> (the second part is evident anyway)

The actions being taken in your patch (including but not limited to calling system()) were dangerous and the patch failed to prevent exploitation of the new features.

I think the point you are missing boils down to: what is the threat model of ffmpeg?

Your patch is built on the assumption that ffmpeg will only ever be run on single-user systems where many things can be trusted, but this is not the case.

In reality, ffmpeg is often used on multi-user systems and called in strange ways from network services where many inputs are not trusted.

In particular:
* ffmpeg cannot trust anything which does not come from a known-trusted source (in practice this means it can trust little beyond its own binary).
* ffmpeg must not assume that it is the only user on the system (anything which can be touched by another user at runtime may be modified).
* ffmpeg should not in general trust the environment (it may be launched from a service which has allowed nasty things to be injected into the environment), though this may be allowed in specific well-known cases.
* ffmpeg should not excessively trust its command-line arguments, as the caller may not have properly vetted them (in practice some nasty cases have to be allowed for the program to work at all).

The main thing that it must prevent is any sort of code execution in the context of the current user, whether that be binary code in the current process or by launching another process running code which came from an untrusted source.

Your patch was dangerous in at least the following ways:
* It interacts with temporary directories in unsafe ways.
* It trusts values coming from the environment.
* It runs a shell with arguments which could have come from an untrusted source.

The last one of those is completely unacceptable and I believe it lead people to reject the patch out of hand without considering anything else.

Some example exploits:
* Another user on a shared system observes the user of the common temporary directory.  They create a file with the predictable name before your code does, then manipulate its contents.  The command to open it then runs code under the attacker's control as the victim user.
* The attacker manipulates the environment to inject their own code into the call to system(), running shell commands as the victim user (I gave a specific example of this in a previous email).

This list is not exhaustive at all, so please do not treat it as a set of specific things to fix - there are very likely other paths for the attacker to achieve code execution in the context of the current user that I am not sufficiently clever to think of as the patch is not written in a secure and robust way.

More generally, I am unsure whether anyone here believes themselves competent to review a patch which securely and robustly performs these sorts of very dangerous actions (I do not believe myself to be), so in practice ffmpeg developers will prefer to never include any code which performs such actions.

Thanks,

- Mark