bbin install should not overwrite other executables in bin - potential security issue

[ieugen]

bbin install should fail when a binary with the same name exists at the path.
It should IMO allow user to force installation with a flag.

This could be a security issue, especially with package owners allowed to set the binary name.
Some people might chose to override things like ls or other stuff.

Current status:

• bbin installs binary commands as they are
• If a binary is present it will be overwritten

~/.local/bin$ ls -lah neil
-rwxr-xr-x 1 ieugen ieugen 1,2K sep 27 11:05 neil

BABASHKA_BBIN_DIR=~/.local bbin install io.github.babashka/bbin --latest-sha --as neil
{:coords
 {:git/url "https://github.com/babashka/bbin",
  :git/sha "6fde1b1dbfaef3063eb1eba4899a730bf703c792"},
 :lib io.github.babashka/bbin}

~/.local/bin$ ls -lah neil
-rwxr-xr-x 1 ieugen ieugen 1,1K sep 27 11:26 neil

cat neil 
#!/usr/bin/env bb

; :bbin/start
;
; {:coords
;  {:git/url "https://github.com/babashka/bbin",
;   :git/sha "6fde1b1dbfaef3063eb1eba4899a730bf703c792"},
;  :lib io.github.babashka/bbin}
;
; :bbin/end

(require '[babashka.process :as process]
         '[babashka.fs :as fs]
         '[clojure.string :as str])

(def script-root "/home/ieugen/.gitlibs/libs/io.github.babashka/bbin/6fde1b1dbfaef3063eb1eba4899a730bf703c792")
(def script-lib 'io.github.babashka/bbin)

Might be related to

• Consider keeping metadata and scripts separate #34
• Add ability to pick which script is installed if repo provides multiple #18
• Use Freedesktop specification for default paths #35

[borkdude]

I think it's a feature that the older version gets overwritten with the newer version.

[borkdude]

We could however prevent overwriting by introducing a flag: --force or so

[rads]

@ieugen: If you're concerned about security, you need to make sure you only install scripts from a) authors you trust or b) code that you've verified yourself. If you're installing from an untrusted source, there's nothing that bbin can do to help you since we don't have any sandboxing feature in Babashka. Blocking script names during bbin install isn't enough on its own since the script can do whatever it wants with the filesystem when it's running.

[rads]

I also recommend putting local bin directories as a suffix in the PATH rather than a prefix. This is how the README does it:

export PATH="$PATH:$HOME/.local/bin"

This reduces the risk of overriding something like sudo if you did happen to install a malicious script.

That said, this isn’t a complete solution since a malicious script could start a subshell with a new PATH that looks like your current shell.

[ieugen]

@borkdude :

I think it's a feature that the older version gets overwritten with the newer version.

I can overwrite ANY binary this way so IMO it needs some consideration or at least a notice.

@rads :

export PATH="$PATH:$HOME/.local/bin"

This is normally set by the distribution.

Blocking script names during bbin install isn't enough on its own since the script can do whatever it wants with the filesystem when it's running.

True. We can maybe do one or more of the following:

• print the changes that are being done what apps are installed and where: e.g. Installed neil as ~/.local/bin/neil
• ask for permission, esp in case where it will overwrite an existing binary. This could be done on first install only if we consider the next ones to be upgrades and overwriting is expected.
• If the script is linked in /local/bin we can check it's contents hash match what we expect to have, before we attempt changes ?!

[jeroenvandijk]

I agree with @ieugen. I think leaving all the responsibility to the end user of bbin is simply not fair. This user has to understand the following:

• What a Babashka script can do (file writing, http requests etc)
• How bbin will use the coordinates to create a binary
• What potential other binaries this new binary is overwriting, literally or by PATH precedence.
• What this other binary is responsible for.

I believe something like bbin should exist for ease of distribution so moving the responsibility of all these safety concerns removes the ease for a serious user actually. Ease would only exist for the naive user, until things go wrong and no one will trust bbin again. Sounds a bit dramatic, but this is how trust works I believe.

So one extra vote for having bbin do a maximum effort to protect the end-user, as this IMO will really contribute to the distribution.

I would want this to go even further than only protecting existing binaries, I would also like so see:

• Telling the user what the script potentially can do. Does it write to non-temporary dirs (analyzing the filesystem code would do a great deal here)
• Does the script do external requests
• etc.

Maybe a permission system would be interesting. "Script asks permission for X, Y, Z. Please confirm"

[rads]

@jeroenvandijk: In general I'm in favor of making bbin's behavior explicit so people know what they're installing. For example, bbin install prints the coordinates so you can check them before running your script. There is also a (currently undocumented) --dry-run option if you want to see what bbin is going to do without any side-effects.

To keep things simple, I think of bbin as a way to write wrappers for bb. So if we want the ability to restrict what a script is doing, it should be supported by Babashka and then bbin can build on top of that. bbin is redundant if you just need to pull in some Babashka code in an automated script (such as CI). In that case you should use a deps.edn file in source control or bb -Sdeps so you're always pinning to an exact version.

I think it's too far for bbin to try to limit what's going on inside the scripts once they're installed. This would be a huge cost in terms of maintenance and complexity and it goes beyond what other competitors are doing. npm has the same problems, allowing you to run arbitrary Bash scripts with whatever binary name you choose:

#!/usr/bin/env bash
echo "Hello"

{
  "name": "rads-ls",
  "bin": {"ls": "./ls"}
}

$ npm install -g ./rads-ls
$ which ls
/Users/rads/.nvm/versions/node/v16.16.0/bin/ls
$ ls
Hello

[rads]

I'm going to move this to a discussion since I don't currently plan to take any action on this issue, but I don't want to outright close it either.