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