System-wide sandboxing framework - sandbox-app-launcher

Patrick · December 16, 2020, 9:55am

#!/bin/bash

set -x
set -e
set -o pipefail

app_user="test"

sudoers_file_content="## test"

mkdir -p /etc/sandbox-app-launcher

## Non-ideal location for temporary file but required for following atomic rename.
## Should not cross potential file system barriers such as /tmp on a different partition than /etc.
echo "$sudoers_file_content" | tee "/etc/sandbox-app-launcher/sandbox-app-launcher_${app_user}" >/dev/null

## Using '>/dev/null' to hide success messages on stdout.
## Would still show error messages if any on stderr.
SUDO_EDITOR='/bin/false' VISUAL='/bin/false' EDITOR='/bin/false' visudo --strict --check --file "/etc/sandbox-app-launcher/sandbox-app-launcher_${app_user}" >/dev/null

## Atomic rename.
mv "/etc/sandbox-app-launcher/sandbox-app-launcher_${app_user}" "/etc/sudoers.d/sandbox-app-launcher_${app_user}"

chaosmonk · December 16, 2020, 8:11pm

But even if we verified using visudo that the file is valid, writing into /etc/sudoers.d needs to be atomic.

Does visudo not write to /etc/sudoers.d/ atomically? I thought the point of visudo was to write to /etc/sudoers.d/ safely by first writing to a temporary file, similarly to what your example code does, plus checks to prevent multiple users from editing sudoers simultaneously.

Patrick · December 17, 2020, 8:51am

I don’t think visudo is designed for use in scripts. Can’t find many examples of using it that way.

Above answer doesn’t consider bash pipefail. I.e.

set -o pipefail

I.e.

echo 'foobar ALL=(ALL:ALL) ALL'  | visudo ... something ...

doesn’t return non-zero on the shell unless we set set -o pipefail. Will edit my above draft to add set -o pipefail.

visudo supports various environment variables for editor setting. It might be best if we set these to /bin/false? Just in case? I am not sure in our scenario there is any chance for these environment variables to be set.

SUDO_EDITOR='/bin/false' VISUAL='/bin/false' EDITOR='/bin/false' visudo

(Will edit above pseudo code to add.)

Related quote from man page:

If set, visudo will use the value of the SUDO_EDITOR, VISUAL or EDITOR environment variables before falling back on the default editor list. Note that this may create a security hole as it allows the user to run any arbitrary command as root without logging. A safer alternative is to place a colon-separated list of editors in the editor variable. visudo will then only use SUDO_EDITOR, VISUAL or EDITOR if they match a value specified in editor. If the env_reset flag is enabled, the SUDO_EDITOR, VISUAL and/or EDITOR environment variables must be present in the env_keep list for the env_editor flag to function when visudo is invoked via sudo. The default value is on, which can be set at compile time via the --with-env-editor configure option.

Above link suggests to use tee as editor for sudo.

#!/bin/bash

set -x
set -e

app_user="test"

sudoers_file_content="## test"

mkdir -p /etc/sandbox-app-launcher

## Should probably set after "set -e" for better style. Setting it here to
## illustrate importance.
set -o pipefail

## visudo does not check exit code of the editor.

echo 'foobar ALL=(ALL:ALL) ALL' | SUDO_EDITOR='tee' VISUAL='/bin/false' EDITOR='/bin/false' visudo --strict --file /tmp/x >/dev/null

But if you replace tee with nonexistent then sudo doesn’t notice that the editor actually failed (non-zero exit code) and doesn’t exit with an error message.

In light of that, I think my above pseudo code (System-wide sandboxing framework - sandbox-app-launcher - #291 by Patrick) is best suited.

Patrick · December 20, 2020, 10:12am

Are we partially re-inventing flatpak which is internally using bubblewrap?
I guess we have a nicer decoupled design.

Also interesting, perhaps worth not re-inventing, regarding /shared:

sandbox-app-launcher (bubblewrap based) will probably be incompatible with applications from flatpak since these have their own bubblewrap based mandatory sandbox?

FrozenCat · December 21, 2020, 4:54am

Have you found a way to connect wayland clients to wayland even if they are on different mount namespaces?

madaidan · December 21, 2020, 7:35pm

Flatpak suffers from numerous fundamental flaws in its sandboxing design.

Likely.

Not yet.

madaidan · December 23, 2020, 10:30pm

Patrick · December 24, 2020, 11:57am

Good. Merged.

Quotes these variables otherwise spaces in variable names (even though we shouldn’t have any) would break that loop.

This line looks unnecessarily complex:

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

Starting a line with : is very uncommon style.
If we need || true would be nice to have it outside of the variable assignment.
:= - Set $FOO to val if unset (or null) - why do we need that there?

shellcheck reports:

In usr/bin/sandbox-app-launcher line 41:
: ${app_path:="$(type -P “${app_name}” || true)"}
^-- SC2223: This default assignment may cause DoS due to globbing. Quote it.

Could we simplify please?

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

Objections?

Patrick · December 24, 2020, 12:26pm

Could you please add /etc/X11/Xsession.d/20software_rendering_in_vms (from vm-config-dist) to xsession_d_file_list_executable if that makes sense?

madaidan · December 24, 2020, 7:04pm

It was intentionally made like that as to support manually setting app_path. It allows a user to execute:

app_path="/path/to/example" sandbox-app-launcher setup example

Why would it be necessary?

Patrick · December 24, 2020, 7:21pm

See file /etc/X11/Xsession.d/20software_rendering_in_vms.

monero-gui in sandbox
any other Qt based application being slow inside VMs because of this

madaidan · December 24, 2020, 8:35pm

Also, is there any need for the full file path in the array?

We could have:

xsession_d_file_list_executable=(
  "20torbrowser"
  "20uwt"
  ...
)

for file_name in "/etc/X11/Xsession.d/${xsession_d_file_list_executable[@]}" ; do

Makes it simpler.

Patrick · December 25, 2020, 11:41am

madaidan via Whonix Forum:

Add 20software_rendering_in_vms by madaidan · Pull Request #51 · Kicksecure/sandbox-app-launcher · GitHub

Merged.

xsession_d_file_list_executable=(
  "20torbrowser"
  "20uwt"
  ...
)

for file_name in "/etc/X11/Xsession.d/${xsession_d_file_list_executable[@]}" ; do

I don’t think this code would work. Tested. Would expand to:

/etc/X11/Xsession.d/20torbrowser

20uwt

String /etc/X11/Xsession.d/ could be inside the for loop. That would
work.

But I don’t see much advantage in that. Trading a bit shorter contents
inside the variable assignment at the cost of higher complexity inside
the for loop. Using the full file names such as
/etc/X11/Xsession.d/20uwt makes it easier to grep (the whole Whonix)
source code. As currently implemented, code is easier to understand.
Currently one can copy/paste one string, the file name, and then use cat
or open in an editor to view the file. By making it
/etc/X11/Xsession.d/ + variable it gets harder. Then one has to
understand this a bit better, copy/paste /etc/X11/Xsession.d/ (or
manually write) + the variable name (such as 20uwt).

Patrick · December 28, 2020, 12:24pm

Would it be possible to combine sandbox-app-launcher with constrained system resources program starter wrapper? Either by one script calling the other. Which one would run which? Or by adding similar code to sandbox-app-launcher?

sandbox-app-launcher currently attempts to improve security by isolating applications in a sandbox. It however does not yet have any defenses against denial of service (DOS) by exhausting system resources. Either as part of an actual DOS attack (compromised application doing DOS). Or as an attempt to prevent a buggy application from slowing down or even freezing the system.

HulaHoop · December 30, 2020, 3:16pm

what general purpose system utilities can be integrated for DoS resistance? Are the defaults sane?

Patrick · December 31, 2020, 3:01pm

There doesn’t seem to be much available against local source DoS (equals misbehaving buggy applications). See constrained system resources program starter wrapper.

Are the defaults sane?

I don’t think there are any default restrictions in Debian at all.

Patrick · January 5, 2021, 6:18am

madaidan · January 12, 2021, 12:19am

We should probably add similar code to sandbox-app-launcher.

We do use AppArmor to enforce a maximum of 200 processes as a basic protection but we should definitely invest in more.

sandbox-app-launcher/sandbox-app-launcher at master · Kicksecure/sandbox-app-launcher · GitHub

More on rlimit rules: apparmor.d(5) — apparmor — Debian buster — Debian Manpages

No, the dreaded :(){ :|:& };: can still be used to trivially wreck a system.

Patrick · January 12, 2021, 7:04am

sudo sandbox-app-launcher setup url_to_unixtime

sandbox-app-launcher run url_to_unixtime

ERROR: The sandbox for this program has not been set up yet. Please execute:

sudo sandbox-app-launcher setup url_to_unixtime

bash -x sandbox-app-launcher run url_to_unixtime

...
+ '[' -d /home/sandbox-app-launcher-appdata/url_to_unixtime ']'
+ echo 'ERROR: The sandbox for this program has not been set up yet. Please execute:

sudo sandbox-app-launcher setup url_to_unixtime'
ERROR: The sandbox for this program has not been set up yet. Please execute:

sudo sandbox-app-launcher setup url_to_unixtime

ls /home/sandbox-app-launcher-appdata/url_to_unixtime

ls: cannot access ‘/home/sandbox-app-launcher-appdata/url_to_unixtime’: Permission denied

That test…

if [ -d “${app_homedir}” ]; then

Cannot work because user user has no permission to check that. How can we solve that?

Patrick · January 12, 2021, 7:32am

Found a solution. And fixed some other things too.

new issue:

sudo --set-home --user=sal-url_to_unixtime bash -c ’
bwrap --ro-bind /bin /bin --ro-bind /usr/bin /usr/bin --ro-bind /lib /lib --ro-bind-try /lib32 /lib32 --ro-bind-try /lib64 /lib64 --ro-bind /usr/lib /usr/lib --ro-bind-try /usr/local/lib /usr/local/lib --ro-bind /usr/share /usr/share --ro-bind-try /usr/local/share /usr/local/share --ro-bind /usr/include /usr/include --ro-bind /etc /etc --ro-bind-data 10 /etc/passwd --ro-bind-data 11 /etc/group --ro-bind /usr/share/sandbox-app-launcher/machine-id /etc/machine-id --ro-bind /var/lib /var/lib --tmpfs /var/lib/dbus --ro-bind /usr/share/sandbox-app-launcher/machine-id /var/lib/dbus/machine-id --ro-bind /sys/devices /sys/devices --ro-bind /sys/class /sys/class --ro-bind /sys/bus /sys/bus --ro-bind /sys/fs/cgroup /sys/fs/cgroup --bind /home/sandbox-app-launcher-appdata/url_to_unixtime /home/sandbox-app-launcher-appdata/url_to_unixtime --proc /proc --tmpfs /tmp --ro-bind-try /tmp/.X11-unix /tmp/.X11-unix --tmpfs /var/tmp --tmpfs /var/cache --ro-bind /var/cache/sandbox-app-launcher-autogenerated/wrappers/url_to_unixtime /var/cache/sandbox-app-launcher-autogenerated/wrappers/url_to_unixtime --ro-bind /usr/bin/url_to_unixtime /usr/bin/url_to_unixtime --tmpfs /run --symlink /run /var/run --dev /dev --chdir /home/sandbox-app-launcher-appdata/url_to_unixtime --setenv HOME /home/sandbox-app-launcher-appdata/url_to_unixtime --setenv USER sal-url_to_unixtime --setenv LOGNAME sal-url_to_unixtime --setenv XAUTHORITY /home/sandbox-app-launcher-appdata/url_to_unixtime/.Xauthority --setenv SHELL /sbin/nologin --setenv started_by_sandbox_app_launcher true --unsetenv SUDO_USER --unsetenv SUDO_UID --unsetenv SUDO_GID --unsetenv SUDO_COMMAND --unsetenv OLDPWD --unsetenv MAIL --unshare-pid --unshare-cgroup --unshare-uts --hostname host --new-session --cap-drop all --seccomp 12 10< <(getent passwd root sal-url_to_unixtime nobody) 11< <(getent group root sal-url_to_unixtime nobody) 12< /var/cache/sandbox-app-launcher-autogenerated/seccomp-filter.bpf /var/cache/sandbox-app-launcher-autogenerated/wrappers/url_to_unixtime ’
bwrap: Can’t create file at /etc/machine-id: Read-only file system

File /etc/machine-id does not exist on my test system for some reason. I guess during setup we need to create an empty one if none exists?

Or better change

--ro-bind ${main_app_dir}/machine-id /etc/machine-id

to

--ro-bind-try ${main_app_dir}/machine-id /etc/machine-id

?