[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 22:26
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] The "bad" Patch
> 

Hello Mark,


(I've re-ordered some parts to unclutter the conversation)


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

That's great because this gets to the essence of what we're talking
about and here I beg to differ:

My point is that exactly this way of judgement you are describing 
is itself a sign of incompetence instead.

This isn't meant to be a rhetoric or flaming statement. It follows from
the simple and confirmed reasoning that hardly anybody would have 
recognized or objected when I would have included the internal implementation
of system() instead. This invalidates the former in total.

But "incompetence" was your word. I wouldn't call it like that because 
I do not want to insult anybody and I do not deem anybody here to be.
Rather would I call it "lazy judgement", as that's what's happened:

One said: "I've seen system()"
And a choir started singing: "Bad, bad, revert, revert" - without 
looking at the code, without own judgement and consideration.

(except yours and possibly others who didn't sing)


> In some cases they may in fact be capable and benign

Thanks for confirming this explicitly.


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

This code is what I had posted for review. I have sufficient experience 
with FFmpeg submissions to know that it is just stupid to work days
on something where there's a chance to get rejected anyway, so it
was done to be fairly safe but without specific effort on hardening.


> You did not do this, and indeed your
> implementation was easily exploited.

Oh, how do you come to that idea? I have conceded that it's better 
not to rely on any environment variable - but what you've shown is
not an exploit:

$ export TMPDIR="'; rm -rf / ;'\\\\"
$ ./ffmpeg -sg -i /dev/null -f null -

as then you could do

$ rm -rf /

right away.


A system (more precisely: user session) where an attacker has 
the ability to set arbitrary env variables to arbitrary values
must already be considered as compromised. 

Setting variables like..

LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, PYTHONPATH
PATH, SHELL, EDITOR, PAGER, SSH_ASKPASS, GIT_SSH
LOCPATH, TERMINFO, ICONV_PATH, NODE_OPTIONS

..is more than enough for an attacker to get anywhere they want
and without need to go through an FFmpeg feature

Regarding realistic exploitability of this, we are talking about very 
specific niche cases like for example:

- an attacker cannot set arbitrary env variables, but TMPDIR only
- and they can control the FFmpeg command line that is executed

When these things coincide, then it could be exploited.
There are surely other variants, but in total still a very small attack
surface (I'm excluding cases with arbitrary env manipulation, because 
FFmpeg/-sg is not needed for anything then).

So, to conclude on this: "easily exploitable" => no way
but still a valid point as I had already conceded.

---

> My point here is really that I don't believe the security implications of the
> patch were considered at all in the initial submission

Like said above, I haven't spent extra time on this as I wasn't sure where
it might go. The primary consideration for the initial submission 
was: How to execute?

For Windows it was clear, but for Linux I had looked around how others are 
doing it. Where system was not used, there was always a lot of code involved,
which made me afraid for two reasons:

1. I wasn't sure about the range of platform support - i.e. where to draw
   the lines between one or another implementation
2. I could already visualize the potential objections from others against
   so much process management code.

Eventually, I looked at glibc's system() code, first the latest code
using clone or clone3 which I hadn't seen much usages of in the wild, and 
then I went back to an earlier version (which matches my Ubuntu VM), and I
realized that this is pretty much the same code that I'm seeing at other 
places where shell invocations are done.
This seemed perfect to me: same procedure like others use, but with a single 
line of code.

And that's how I knew that using system() is okay.

I'm not a frequent Linux platform developer, so I didn't know that people
would freak out about it, but I did my homework, which led to the weird 
situation that I could claim that system() isn't worse than what everybody 
else is doing while many others were acting like it would be even something 
that allows privileged execution.

Such things can happen, but it should not happen in the way it did, 
combined with so much disrespect and everything that followed.

In the latter regard - again - you have been a shiny exception,
and highly appreciate that!


PS: This is a perfect finish, so I'll respond to the details in a 
separate mail.

Thank you,
sw