#!/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.
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 theSUDO_EDITOR
,VISUAL
orEDITOR
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 useSUDO_EDITOR
,VISUAL
orEDITOR
if they match a value specified in editor. If the env_reset flag is enabled, theSUDO_EDITOR
,VISUAL
and/orEDITOR
environment variables must be present in the env_keep list for the env_editor flag to function whenvisudo
is invoked viasudo
. 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.
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?
Have you found a way to connect wayland clients to wayland even if they are on different mount namespaces?
Flatpak suffers from numerous fundamental flaws in its sandboxing design.
Likely.
Not yet.
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?
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?
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?
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
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.
madaidan via Whonix Forum:
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
).
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.
what general purpose system utilities can be integrated for DoS resistance? Are the defaults sane?
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.
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.
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?
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
?