[FFmpeg-devel] The "bad" Patch

[Mark Thompson]

On 31/05/2025 19:28, softworkz . wrote:
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of Mark
>> Thompson
>> Sent: Samstag, 31. Mai 2025 18:21
>> To: ffmpeg-devel at ffmpeg.org
>> Subject: Re: [FFmpeg-devel] The "bad" Patch
>>
>> 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)
> 
> 
> Hi Mark,
> 
> thanks a lot for your reply, much appreciated.
> 
> When you have read the last two (final) messages of this conversation, it
> should have become clear that what I'm up to has never been whether the 
> patch would get merged/reverted or not. It is about the way how it happened.
> Without anybody looking closer (except yours at least), the patch was 
> brandmarked as "bad" >> sw is pushing bad patches >> and everything that
> followed.
> 
> 
> Everything you wrote (below) is totally valid and needs careful consideration.
> Such considerations and discussion have just never happened. When the outcome 
> would have been that it cannot be done safely enough, that would have been 
> totally fine as well, I never had the idea that this must get into FFmpeg 
> by all means. 
> 
> 
>> 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.
> 
> No, that hasn't been my assumption. 
> 
> 
>> In reality, ffmpeg is often used on multi-user systems and called in strange
>> ways from network services where many inputs are not trusted.
> 
> None of such systems have xdg-open

xdg-open is in most default install of Linux distributions (indeed, that's why you use it), so I don't think it is a reasonable assumption that it would not be there.

>> 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).
> 
> All valid, for sure.
> 
> 
>> 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.
> 
> That's three bullets, but a single thing and the one from which I had 
> already said this was the one flaw it had. It's fixed in the updated version:
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/d44acee0a6f1021669f57641b1f1875d8dbe51c5.1748445849.git.ffmpegagent@gmail.com/
> Series: https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=14631
> 
> The reason why I've fixed it, is again not about getting it merged but
> about the question: Now that this part is fixed, what is still bad
> about using system()?
> 
> That's the single question I'm asking, because the rejection was 
> about use of system(), not about the temp folder routine (you were the
> only one who's even seen it, even though the file wasn't really
> big).

Doing this sort of thing in a secure and robust way is well-known to be a difficult problem.

It does not seem unreasonable for people to treat the appearance of a vulnerable call to system() like seeing self-rolled cryptography: the submitter is almost certainly either incompetent or malicious, and can be safely ignored.

In some cases they may in fact be capable and benign, but it is up to the submitter to show when doing that that they understand all of the issues and have properly dealt all of them.  You did not do this, and indeed your implementation was easily exploited.

>> 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 file name is built from the local time with milliseconds. Pretty hard to
>   hit

No, trivial to hit given that creating a file and watching whether it gets touched (inotify) are very low cost operations.

> - In v2, a temp-directory specific to the current user is created. Other users
>   have no access

Your new method does not work because the attacker could have created the temporary directory (world-writeable) before the ffmpeg process does.

> - The file is an html file which will be launched by a browser from file:///
>   url, means it is treated with extra safety and isolation. There's hardly anything
>   you could achieve these days from a local html page

Is there some general citation for this?

I would naively expect browsers to assign greater trust to local rather than remote files and possibly allow some additional capabilities to scripts running in them, but I admit I have no familiarity with this area so I may be completely wrong.

>> * 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).
> 
> Yes, that's fixed, no more env variables are used at all. This was the 
> one flaw it actually had.

Some further thoughts on your new patch which you will undoubtably have already considered:

* What happens if the system argument string exceeds the allowed command argument length?

* What happens if /bin/sh is not bash?

* What happens if the attacker successfully contrives a transient out-of-memory condition during any of the calls to av_bprintf()?  (As they can do on a shared machine.)

(The Windows implementation is not changed and does not look robust, I assume you have not revised it.)

>> 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.
> 
> Many other CLI tools are launching browsers, so that's not really rocket 
> science like you're trying to allude to.

I agree.  Rocketry seems to be generally reliable and successful when compared to computer security, where people forever find new vulnerabilities in supposedly secure and audited programs.

I would hope that other CLI tools doing this have carefully documented the circumstances in which they do so to ensure that they don't get used in cases where it might cause problems.

>                                          I still agree to the degree that
> there's always a "you-never-know factor". At that point, it's much more effective
> and secure to bring additional factors into the game. These could be like:
> 
> 
> 1. Requiring a one-time setup
> 
> For example, on first use of the feature, it says "please run once with sudo to
> enable the feature". When done, it writes some file somewhere with the uid for
> which it should be allowed and appropriate permissions.
> 
> 2. Requiring user input 
> 
> Like a key press to launch, after which it is enabled for a while to auto-launch
> 
> 3. Checking console env for whether it's an interactive session 
> 
> Not sure to which extent that's possible and safe
> 
> 4. Check uid
> 
> Don't do it when 0
> 
> 5. Check existence of xdg-open and/or browser
> 
> possibly even content of xdg-open
> 
> 
> ------------
> 
> Of course I haven't done any of this, because I had expected that there 
> would be a review/discussion to determine a direction. 
> 
> Or the outcome would have been a "No, we don't want this". Also fine.

I think this outcome is the clear answer at this point.

My point here is really that I don't believe the security implications of the patch were considered at all in the initial submission, and are still not being treated properly in the revised version.  While some responses may not have been ideally formulated they were directionally correct.

Thanks,

- Mark