System-wide sandboxing framework - sandbox-app-launcher

ERROR: Could not find ‘test test’ in $PATH.

Is there any need for sanitizing app_name when we have the $PATH check?

shellcheck says it’s bad.

^-- SC2086: Double quote to prevent globbing and word splitting.

And it’s true.

#!/bin/bash

set -x

testvar='/sbin/*'
newtest="$(type -P ${testvar})"

This example globs all of /sbin/*

Kinda separate discussion. Not sure if strong reasons but yes.

  • When running bash -x scriptname then the shebang bash -e (errexit) is being disregarded.
  • Stylistic. Imo easier to read. Specifically when using multiple shell options.
1 Like
. "/etc/sandbox-app-launcher/${app_name}.conf"

Currently run before ${app_name} is sanitized. Could you fix that please?

1 Like
  if ! [ -e "${app_path}" ]; then
    echo "ERROR: Could not find '${app_name}' in \$PATH."
    exit 1
  fi

It’s not really checking if it’s in $PATH. It checks existence of any file system object (not only file, that’s -f?). Maybe -x?

But then also begging the fundamental question, why should sandbox-app-launcher be limited to binaries in $PATH? Why not any executable whether in $PATH or not in $PATH? The user can execute it anyhow. If with or without sandbox, then the latter is better, no? Maybe I am missing something here still.

1 Like

What about command line parameters to apps cat /path/to/file? Could you please compare with other wrappers (such as uwt) for proper getting name of binary and passing parameters?

1 Like

Wouldn’t root be better to protect the sandboxing app from modification by malicious programs in case the unprivileged user is compromised?

2 Likes

Modification of the sandbox app won’t be possible without root since it resides in /usr which isn’t writeable by non-root.

If starting sandbox-app-launcher should require sudo password or not is a different question. I don’t see what a malicious program could gain (compromised non-root, user account) by launching the sandbox-app-launcher?

2 Likes

What about single quotes?

app_path=“$(type -P ‘${app_name}’)”

We need to have the file path of the program (which is gotten via $PATH) as well as a name to create the symlink for the apparmor profile. We can’t just execute $app_name.

The apparmor profile confines everything in @{SYMLINK_DIR}. I don’t know any other way to choose only the sandboxed apps for apparmor confinement. There is aa-exec but this isn’t good as it will have to either confine the bwrap program too or be confined by bwrap which means we’ll have to weaken one of them.

apparmor-profile-everything will deny execution of anything outside of a few directories anyway.

Requiring sudo isn’t really a strong barrier: Strong Linux User Account Isolation

I think Hulahoop meant app data. A malicious user account can start the app in a sandbox to access the app’s home directory.

1 Like

How is:

app_name="${1}"
args="${@:2}"
...
bwrap \
...
${symlink_dir}/${app_name} ${args}

?

error_handler() {
  echo "
## sandbox-app-launcher BUG.
## BASH_COMMAND: ${BASH_COMMAND}
## Please report this BUG!
"
  exit 1
}

trap "error_handler" ERR

Would that be enough? Or is more needed?

madaidan via Whonix Forum:

What about single quotes?

app_path=“$(type -P ‘${app_name}’)”

Single quotes undo the effect of $ which then becomes a literal string
not symbol for variable.

apparmor-profile-everything will deny execution of anything outside of a few directories anyway.

Let’s think of sandbox-app-launcher as independent as possible?

I think Hulahoop meant app data. A malicious user account can start the app in a sandbox to access the app’s home directory.

Indeed. Not sure we can reasonably prevent that. The user capable to run
apps in a sandbox is kinda a “mini-admin” anyhow?

Maybe if/after this matured we could implement “no apps can be started
without without sandbox”? (At least superroot can overwrite.)

Or the “mini-admin” (whoever starts apps) itself is a sandbox? But even
then if that “mini-admin” is compromised, it doesn’t really matter if
sudo password is required or not as sudo isn’t a strong barrier.

1 Like

Shellsheckk complains. Also indeed shouldn’t assign an array to a string. That breaks things. No need for args. "$@" stays available throughout the runtime of the script.

This should handle all cases perfectly:

app_name="$1"
shift 1
"${symlink_dir}/${app_name}" "$@"

But for that we need to get rid of the bash -c double quoting issue.

1 Like

That is good. A previously set -e is useful if something goes wrong inside the trap or if run through dash instead of bash.

1 Like

What else is left other than the bash -c thing?

1 Like

bash -c thing. That should also fix white space issue. Once that is fixed also some bugs below are easier to fix.

Please avoid subshell.

Currently invalid path will result in silent exit 1. (Because of set -e and app_path="$(type -P "${app_name}")".)

Could investigate exec. I saw these other brwap scripts use it. On one hand exec is discouraged for security on the other hand might it be good to not have bash in memory?


Doesn’t like full path to app.

sudo sandbox-app-launcher /usr/bin/hello

useradd: cannot create directory /home/sandbox-app-launcher-appdata//usr/bin/hello


Chokes in my Qubes Debian based VM.

sudo sandbox-app-launcher hello

bwrap: Can’t find source path /usr/local/share: No such file or directory

bwrap: Can’t create file at /var/lib/dbus/machine-id: No such file or directory

(Fixed when manually editing the script but would be good if that could somehow be auto detected and/or avoided.)


Could use some manual or automated tests. Testing sandboxing a binary that will try to break out but be prevented from it. Also a well behaving app actually still functional.

1 Like

You mean

10< <(getent passwd root ${app_user} nobody) \
11< <(getent group root ${app_user} nobody) \

?

Why are these an issue? It’s far cleaner than creating temp files.

Would this be alright?

app_path="$(type -P "${app_name}" || true)"

Also, shouldn’t app_name be sanitized before setting app_path?

Why? I can’t find anything about that.

The file path should be gotten via $PATH and not supplied.

Those are easily fixed with --ro-bind-try.

This is a good idea. TIOCSTI is a common one that’s prevented by --new-session + seccomp.

1 Like

Unrelated, but should /var/log be accessible in the sandbox? Most important logs are root only anyhow due to filesystem permissions. /var/log is currently accessible but I’m not sure if we should remove it or make it a permission.

1 Like

Changing the seccomp filter to a whitelist definitely broke W^X somehow. I’ve no clue why it’s not working now.

1 Like

Nvm, I finally figured it out.

I’ve never been so happy to see my stuff breaking before.

1 Like