System-wide sandboxing framework - sandbox-app-launcher

[madaidan]

Unset bwrap_args and sanitize app_name

[madaidan]

@Patrick

What if app_name contains spaces?

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

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

[Patrick]

@madaidan

Aren’t the quotes around $(...) enough? Wouldn’t quoting those variables cancel those quotes?

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/*

@madaidan

Is there any reason why using bash -e as a shebang isn’t used at all in Whonix instead of set -e ?

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.

[Patrick]

. "/etc/sandbox-app-launcher/${app_name}.conf"

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

[Patrick]

  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.

[Patrick]

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?

[HulaHoop]

@madaidan

I’m not sure whether it should require root or not.

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

[Patrick]

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?

[madaidan]

@Patrick

shellcheck says it’s bad.

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

And it’s true.

What about single quotes?

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

@Patrick

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?

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.

@HulaHoop

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

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

@Patrick

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?

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

[madaidan]

@Patrick

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?

How is:

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

?

[madaidan]

@Patrick

Better a trap ERR (see Whonix source code error_handler in many places).

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?

[Patrick]

madaidan via Whonix Forum:

@Patrick

shellcheck says it’s bad.

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

And it’s true.

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?

@Patrick

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?

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.

[Patrick]

@madaidan

How is:

app_name="${1}"
args="${@:2}"

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.

[Patrick]

@madaidan

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?

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

[madaidan]

Improvements

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

[Patrick]

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.

[madaidan]

@Patrick

Please avoid subshell.

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.

@Patrick

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

Would this be alright?

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

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

@Patrick

On one hand exec is discouraged for security

Why? I can’t find anything about that.

@Patrick

Doesn’t like full path to app.

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

@Patrick

Chokes in my Qubes Debian based VM.

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

@Patrick

Testing sandboxing a binary that will try to break out but be prevented from it.

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

snap - seccomp BBlacklist for TIOCSTI can be Circumvented

[madaidan]

Don't fail if some directories don't exist

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.

[madaidan]

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

[madaidan]

Nvm, I finally figured it out.

Fix W^X

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