Fre let's you execute arbitrary shell commands

[uwagura]

Describe the bug
Since fre uses shell=True when running subprocesses, you can pass in arbitrary shell commands to fre, which is a security risk

fre-cli/fre/pp/status.py

Lines 14 to 15 in d1457e2

	cmd = f"cylc workflow-state {name}"
	subprocess.run(cmd, shell=True, check=True, timeout=30)

To Reproduce
fre/canopy will execute the shell commands within the following command

fre pp status -e ";cat ~/.ssh/id_rsa;" -p ";touch unwanted_file.txt;" -t ";echo $USER;"

Expected behavior
fre should ignore shell commands passed as arguments into the function

Additional context
In practice, this probably isn't a big issue for fre since fre users won't have the ability to manipulate other peoples data and probably won't pass anything that can be interpreted as a shell command to fre. Still, it may be worth removing the shell=True argument from fre just for best practice reasons. It looks like the recommended way to do this is to change the above command to:

subprocess.run(['cylc','workflow-state',name], check=True, timeout=50)

And likewise, to do the same everywhere where subprocess.run is called with the shell=True argument

[ilaflott]

wow this is the most validating thing i have ever seen

[ceblanton]

Thank you so much @uwagura for raising this issue. I 100% agree with you, and feel that that ALL external shell calls be immediately outlawed and existing usage should be removed.

All of the cylc and isodatetime and other similar usages could be rephrased as native python using the cylc, isodatetime, and other python interfaces. The existing usage is really first-draft, and not proper I agree. Thank you again for bringing this up.

Can we outlaw external shell calls through pylint?

[cwhitlock-NOAA]

I think you could outlaw external shell calls by lintering, but I'd argue it's got some valid use cases - the git calls in the checkout script, for example. Then again, if we can get an install of gitpython ( https://github.com/gitpython-developers/GitPython), that one point goes moot. Still, you SHOULD be able to linter for calls to subprocess with shell=TRUE and exclude shell=FALSE.

…

On Fri, Oct 25, 2024 at 12:26 PM Chris Blanton ***@***.***> wrote: Thank you so much @uwagura <https://github.com/uwagura> for raising this issue. I 100% agree with you, and feel that that ALL external shell calls be immediately outlawed and existing usage should be removed. All of the cylc and isodatetime and other similar usages could be rephrased as native python using the cylc, isodatetime, and other python interfaces. The existing usage is really first-draft, and not proper I agree. Thank you again for bring this up. Can we outlaw external shell calls through pylint? — Reply to this email directly, view it on GitHub <#219 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AC362WVDRHEFBOHCDMITNYTZ5JWJJAVCNFSM6AAAAABQTQMFQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMZYGI3DAMBTGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- Carolyn Whitlock

[ceblanton]

Let's take the existing shell usages case-by-case, and give exceptions for ones that don't have python interfaces. I agree with you there could be some essential needs for shell calls in those cases.

[ceblanton]

Bronx fre-commands has a super-rich use of Linux shell utilities, so there's nothing wrong with shell calls, but I would say it's more proper to use native python interfaces if possible, and also the security argument.

What about potential use of fre-cli on Windows (shudder) as motivation for going shell-free, even if we don't have to. Shell calls can't work on Windows, I would guess.

[ilaflott]

Windows has powershell, so the same problem exists there

I had to figure this out for ppan_handler in fre-workflows

This kind of problem is exactly the use case for shlex

here's how cylc uses shlex to sanitize job submission commands.

we could, like cylc, wrap all subprocess.run/POpen calls with a function that sanitizes the input command

[ilaflott]

about 13 instances of subprocess.run calls- not too embedded in the code base to sanitize at all.

only two POpens thankfully