[FFmpeg-devel] The "bad" Patch

[softworkz .]

> -----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


> 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).


> 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
- In v2, a temp-directory specific to the current user is created. Other users
  have no access
- 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

> * 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.


> 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 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.


Just the actual outcome was very wrong.

Thanks a lot for writing back!

Best regards,
sw