System-wide sandboxing framework - sandbox-app-launcher

madaidan · November 15, 2020, 12:55am

Actually, this doesn’t seem to work either. Permissions modified within the namespace seem to be modified outside it too.

FrozenCat · November 15, 2020, 12:36pm

Perhaps, you want to create a proxy UNIX domain socket?

madaidan · November 15, 2020, 5:43pm

I don’t know if that would work. Can you test?

Unrelated:

FrozenCat · November 16, 2020, 7:29am

I can’t test because I don’t know how to test it.

Let’s assume that I created a proxy UNIX domain socket as wayland user with a proxy program. I would need to find a way to transfer the socket to tmpfs on a new mount namespace and change the owner of the socket.

To do that, the proxy program needs to have access to wayland’s mount namespace and a new wayland client’s mount namespace.

HulaHoop · November 16, 2020, 3:00pm

Xwayland does AFAICT from this bug fix:

https://cgit.freedesktop.org/xorg/xserver/commit/?id=c4534a3

Also an interesting related thread:

chaosmonk · December 16, 2020, 8:50am

What about something like this?

echo "${SUDO_USER} ALL=NOPASSWD: /usr/bin/sudo -H -u ${app_user}" |  EDITOR=tee visudo /etc/sudoers.d/sandbox-app-launcher_${app_user}

Patrick · December 16, 2020, 9:34am

visudo is a good idea but insufficient. We can write to a temporary file. Then use visudo as a sanity check. But even if we verified using visudo that the file is valid, writing into /etc/sudoers.d needs to be atomic. I am worried about file system inconsistency. For example, power loss during writing the file. Either the file creation is complete and successful or partial and not really part of the file system.

I.e. can

echo "0123456789" > testfile

result in only ending up

01245

on the hard drive if power is lost in the middle of the operation?

Patrick · December 16, 2020, 9:44am

References on atomic writes to disk:

https://btrfs.wiki.kernel.org/index.php?title=FAQ&oldid=11521#What_are_the_crash_guarantees_of_overwrite-by-rename.3F

(We currently use ext4 as file system. Not btrfs. Just to show why this might be an issue.)

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.