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?
#!/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}"
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.
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.
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.
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?
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.
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).
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.