Remove --allow-run .. rather people should use --allow-all

[ry]

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.

[hayd]

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

[nayeemrmn]

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

[kevinkassimo]

Hmm but this means we are gaining extra privilege besides all --allow-* flags. I think this is likely to confuse people...

[axetroy]

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

[rsp]

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

1. all subprocesses (no security)
2. Deno subprocesses (security guarantees)

then I see either:

• adding a new flag for No.2 like --allow-deno (this may not be descriptive enough)
• changing the current flag for No.1 to --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.

[axetroy]

@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

[rsp]

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

[brandonkal]

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.

[nayeemrmn]

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

[axetroy]

@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

[brandonkal]

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.

[hayd]

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?

[brandonkal]

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

• Changing allow-all to allow-unsafe makes it more clear that no permission checks are performed.
• Changing allow-run to allow-exec makes it harder to confuse with deno run and that Deno's permission sandbox cannot restrict the allowed subprocesses.
• I've also changed args to cmd in the renamed Deno.run function. But that it is a separate proposal.
• A whitelist is required on allow-exec
• Adds a prominent warning and simple demonstration of how "all bets are off"

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.

[ahungry]

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.

[kitsonk]

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.

[brandonkal]

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.

[danopia]

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

Prior Art: sudoers file

## Allows members of the users group to mount and unmount the cdrom as root
%users  ALL=/sbin/mount /mnt/cdrom, /sbin/umount /mnt/cdrom

## Allows members of the users group to shutdown this system
%users  localhost=/sbin/shutdown -h now

## Allows users to export SMART statistics from any connected hard drive
%users  ALL=/usr/sbin/smartctl -a -- *

## Allows admin users to start or stop WireGuard tunnels
%wheel ALL=/bin/systemctl start wg-quick@*, /bin/systemctl stop wg-quick@*

## Groups of commands to allow
Cmnd_Alias DELEGATING = /usr/sbin/visudo, /bin/chown, /bin/chmod, /bin/chgrp 
Cmnd_Alias PROCESSES = /bin/nice, /bin/kill, /usr/bin/kill, /usr/bin/killall

This file is a list of commands that allows non-root users to run commands as root (among other arrangements). The granted commands could be a very specific whole command, or a specific binary to run with whatever, or just a straight wildcard to do anything. This amount of wildcard expressiveness is probably overkill for --allow-run but it would be pretty sweet!

[stale]

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.

[danopia]

Apparently --allow-run scoping made it into Deno 1.9 via #9925 & #9833

[bartlomieju]

Requiring --allow-all just to invoke a child process is a huge mistake.

This has never happened. You can still use --allow-run flag, we actually made it more useful and provide stronger guarantees in Deno 2: https://deno.com/blog/v2.0-release-candidate#--allow-run-flag-changes

[justinmchase]

Sorry @bartlomieju, I think this issue was maybe the wrong place to comment so I filed a more thorough issue because it did just happen to me:
#26839

Then, anytime a subprocess is spawned with LD_* or DYLD_* environmental variables, a full --allow-run permission will be required.

Sorry, but this is seems like a major security regression to me.