Closed ry closed 3 years ago
I wonder if there could be a way to restrict the run, in the same way net/read etc. e.g. starts with python my_script.py
or specific-binary
, at least that way you'd be restricting to something more specific than anything ( rm -rf
).
how are most people using run
at the moment...?
+1 I've pointed this out before.
I guess the downside is losing any way of having more granular run permissions... but when would untrusted remote code be using your carefully written project-local scripts? I think they would normally want access to arbitrary common OS utilities which are often more powerful than they seem and just broadly not compatible with any sandboxing attempt. It's too easy to deceive upon.
Hmm but this means we are gaining extra privilege besides all --allow-*
flags. I think this is likely to confuse people...
Because the permissions of the child process cannot be restricted.
Running a child process means that it has all the permissions.
So I think this makes sense.
#!/usr/bin/env -S deno --allow-run
const { run, args, env } = Deno;
// get all permission
if (args.includes("--with-all-permission") == false) {
const command = ["deno", "--allow-all"]
.concat(args)
.concat(["--with-all-permission"]);
const ps = run({
args: command,
stdin: "inherit",
stderr: "inherit",
stdout: "inherit"
});
await ps.status();
} else {
// do the staff with all permission
console.log(env());
}
$ deno demo.ts --allow-run
@ry Yes, this is what I was talking about in #2128 in April - totally true that it makes no sense to have --allow-run
as it is now because it is effectively --allow-all
in disguise.
But I see a value in having --allow-run=/bin/ls
so my idea would be to make the parameter to --allow-run
mandatory.
By the way, as I explained in #2128 I think that there should be a separate permission to run child process of Deno programs with the same interpreter and the same (or with a subset of) privileges than the current process has. This will be the only way to run subprocesses safely without privilege escalation and I think running Deno subprocesses safely will be an important feature to have.
In #2128 I also was thinking about the flag names. If we have two flags for:
then I see either:
--allow-deno
(this may not be descriptive enough)--allow-exe
or --allow-exec
and have --allow-run
for No.2 (which sounds less scary than exe/exec and is in line with deno run
subcommand)My idea was either:
Keeping the existing --allow-run
:
--allow-run Allow running subprocesses (risk of privilege escalation)
--allow-deno Allow running Deno subprocesses (same permissions or less)
or changing the current --allow-run
to --allow-exe
or --allow-exec
to make the --allow-run
in line with deno run
:
--allow-exe Allow running subprocesses (risk of privilege escalation)
--allow-run Allow running Deno subprocesses (same permissions or less)
Edit: See also my comments in #2081 (1, 2) where I originally came up with the idea for more examples.
@rsp
How about --allow-run=/bin/bash
?
run({
args: ["/bin/bash", "-c", "do what ever you want"]
})
The same reason, once the child progress is turned on, there is no way to bind
@axetroy of course when you allow to spawn a shell then all bets are off, but this is at least explicit and obvious. If bare --allow-run
is disallowed, then people will not be likely to add --allow-run=/bin/bash
just for the hell of it. But if it is allowed, then people will be likely to use --allow-run
instead of --allow-run=/bin/ls --allow-run=/usr/local/bin/htop --allow-run=/usr/bin/gzip
because it's shorter and looks innocent. I think this is what @ry was concerned about, and rightly so.
I still see --allow-run
as useful outside of security. I don't see much benefit in removing it, though a note should be added to the flag in help that it can be used to effectively achieve the same result as --allow-all
security wise. Allow run should accept a whitelist.
Deno is useful for config generation because you can lock it down. For instance, you don't want a config program to be able to read environment variables so that the output is consistent. But you may wish to allow the program to run helm template
for instance. With a whitelist that is enforced by the runtime, you can reasonably be sure that other variables do not leak in.
@ry Now that --allow-plugin
exists and introduces the same problem, I think trying to achieve what this issue suggests will lead to complication and inconsistencies. As many have pointed out, neither would be redundant with whitelists. Close this?
@rsp
use --allow-run=/bin/ls --allow-run=/usr/local/bin/htop --allow-run=/usr/bin/gzip
seem good.
But there are libraries that need to be cross-platform, and each platform runs a different script
Once more libraries are referenced by the project
Then the --allow-run=xxxx
required for running will become very long.
For example a library I wrote deno_machine_id
Then to run this library, you need
deno run xxxx \\
--allow-run="${winDir}\\System32\\REG.exe" \\ # for Windows
--allow-run="ioreg" \\ # for macOS
--allow-run="/var/lib/dbus/machine-id" \\ # for Linux
--allow-run="/etc/machine-id" # for Linux
Imagine that, this is just a reference to a library
What if 10 libraries or more are referenced? This command will become very, very long and unmaintainable
This is a problem that i worry about
It could parse as a CSV list to keep the whitelist args approach shorter. I like the flexibility. If you must maintain a long list you could put the whitelist in code before calling the library.
xlink to PR: #4340 🙍
@axetroy I think in that case (if it is too overwhelming) they have the option to use --allow-all, but why mandate it?
A proposal:
Change the help text to look like this:
-U, --allow-unsafe
Disable all permission checks
--allow-env
Allow environment read and write access
--allow-hrtime
Allow high resolution time measurement
--allow-net=<allow-net>
Allow network access
--allow-plugin
Allow loading plugins
--allow-read=<allow-read>
Allow file system read access
--allow-exec=<allow-exec>
Allow executing specified subprocesses.
WARNING: Deno cannot restrict the subprocesses:
Deno.exec({ cmd: ['deno', 'run', '--allow-all', 'untrusted.ts'] })
A comma-separated whitelist is required.
--allow-write=<allow-write>
Allow file system write access
This does the following
deno run
and that Deno's permission sandbox cannot restrict the allowed subprocesses.If --allow-exec=*
is permitted and specified, always log the same warning before compile/run. Alternatively, --allow-exec=*
is disallowed and users would instead use --allow-unsafe
if they cannot be bothered to create a whitelist.
Beyond all that, it would be interesting if users could specify specifc subcommands in the whitelist. For example:
deno --allow-exec=[helm,template],[helm,repo,add] config-gen.ts`
That would allow a script to run helm template
and helm repo add
but not make modifications to a Kubernetes cluster such as helm install
. Deno would check the args of any Deno.exec() request and ensure they begin with one of the provided arrays.
Restricting the whitelist to specific subcommands also covers @rsp's use case of allowing a Deno subprocess with specific permissions.
As noted in my duplicate filed issue - this is very misleading (a false sense of security is worse than no security), so the allow-run flag is very dangerous.
I know it is a bit of a pain, but better UX to have --allow-run
log a message about security, say to use all, and exit. Make people aware of the consequences instead of just removing it.
I'm still very much against removing it. As explained many times above it is still a useful feature outside of security. Further, taking a binary whitelist approach as I propose above does make this a secure feature. Improve the help text messaging and consider a change to allow-unsafe and it is done.
This thread's a bit stale huh 😟
I'm also a big fan of the --allow-run
concept and believe executable scoping would make it a lot less of a footgun. This permission is excellent for particular complex tasks which are hard to accomplish within Deno and which are implemented in a well-known utility already anyway! It also reduces the amount of 'accidental' side effects your script can have as Brandon already said.
Some examples of how I'd use a scoped run permission:
--allow-run=kubectl
to allow a Deno program to access the user's preconfigured Kubernetes clusters. Replicating the full degree of Kubernetes connections is currently impossible in Deno actually, and would require nearly every permission anyway, including --allow-run
for auth helpers. So --allow-run=kubectl
gives the Deno script a lot less access and is still more productive and effective. I use this pattern extensively today!--allow-run=smartctl
for a script that lists connected hard drives & reads their health data.--allow-run=ping
to check if any host is up / the network is good, without being able to actually connect to any network servers.--allow-run=wg
to an unprivileged user is a good way to generate privkeys, and convert a privkey to a pubkey, if you were doing a VPN configuration script.--allow-run=buildah
to allow a script to make and push Docker images without root access (Buildah is a non-root image builder)This doesn't prevent a passlisted program from letting you read an arbitrary file, like you could have a Dockerfile that adds /etc/passwd
and then cat
s it. The attack would need to be a ton more specialized though; not an accidental copy-paste or a generic nefarious import.
I don't see reason to require an absolute path; --allow-run=wg
should be enough to write Deno.run({cmd: ["wg", "genkey"]})
. The program doesn't care where wg
comes from so it's a bit extra for the flag to require caring about that. it's not like the Deno script will be able to write out an alternative nefarious executable file and place it first in $PATH
without a few more permissions in place anyway.
A prior art on the usefulness of limited-scope subprocesses would be the sudoers file on Linux:
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.
Apparently --allow-run
scoping made it into Deno 1.9 via https://github.com/denoland/deno/issues/9925 & https://github.com/denoland/deno/pull/9833
When you execute a subprocess there's no guarantees of what it can do - all security bets are off.
--allow-run
masks this and is an extra command line flag. Therefore I suggest removing it entirely and requiring users to supply--allow-all
.