run_program() {
...
run_if_root chmod 1777 "${shared_dir}"
That’s a mistake. If run_program() it shouldn’t be running as root under any circumstances. Actually should error out if running as root inside run_program()?
run_if_root() makes sense when run from setup().
setup() should be renamed to setup_or_check() or so. Then all commands from within setup_or_check() could be prepended with run_if_root().
run_program() being run as root isn’t a major issue; privileges are dropped in the sandbox anyway.
If the user is not root and the permissions are broken, run_if_root will exit with an error message, informing the user to run the setup. If the user is root, it will immediately fix the permissions seamlessly. This seems like the best approach to me.
Is this not what you described in System-wide sandboxing framework - sandbox-app-launcher - #325 by Patrick ?
We could maybe output a message if run_program() is run as root, explaining that root is unnecessary although actually aborting the program doesn’t seem beneficial since privileges are dropped either way.
setup() must always be run as root.
I don’t think the term official release is well defined yet at Whonix. The word release can mean many things.
Package sandbox-app-launcher is available from official Whonix repository. That’s a development milestone as well as useful for further development/testing.
The launcher is currently a work-in-progress and is not yet ready for actual use.
Once it’s ready that notice will be removed from its wiki page and there will be a call for testers Whonix news blog post.
Currently, we give total access to all binaries and libraries (/bin, /lib, etc.) but most of them aren’t needed. I’m experimenting with a way to dynamically determine the exact binaries and libraries required by a program to mount them into the sandbox. My current approach is:
objdump is used to determine the libraries directly required by the provided executable. ldconfig is used to translate the basic library names (e.g. libc.so) into their actual file path (e.g. /usr/lib/libc.so.6). The script then recurses over each of those libraries to determine the libraries that are indirectly required. This process is repeated until all libraries have been discovered.
The issue with this however, is that an executable may call other executables that pull in their own libraries. These libraries will not be detected by this approach so the package manager then recursively checks all the dependencies required by the package which owns the executable. It repeats the above but for every library and executable in every dependency discovered (with the file list e.g. Debian -- File list of package firefox-esr/buster/amd64).
It does not use ldd for security reasons. See the security section of the man page: ldd(1) - Linux manual page
From my initial testing this seems to be pretty thorough. It can add significant overhead to starting applications so I added a way to cache the list of required libraries.
On my system, there are 143617 files in /usr/bin and /usr/lib. When running Chromium with the script, there are only 1274 files available in the sandbox. That’s 0.887%.
The only issue I’ve found so far is that Krita requires too many arguments than bubblewrap can handle:
bwrap: Exceeded maximum number of arguments 9000
We may be able to resolve this and figure out a way to slim it down. Other than that, most applications seem to work fine. The script is a bit complex and messy however.
I’ve only tested this on Arch Linux with pacman but porting it to Debian/apt should be easy. Only few commands are package manager dependent.
Use it like:
findlibs /usr/bin/chromium
What do you think? Would it be worth it? Libraries aren’t particularly sensitive and they’re all read-only anyway but this could be good practice, following principle of least privilege.
That’s a cool idea. It might break an exploit chain, disable some exploit payloads, make exploitation harder. Non-perfect as per Are non-perfect Defenses that defeat off-the-shelf Viruses a worthwhile Development Goal? or could it be in some cases really strong?
Similar to:
I am wondering why nobody earlier got that idea? Or did someone?
A bit similar to this:
Design:
With sandbox-app-launcher we have to worry about too things:
And sandbox-app-launcher might be used to start two different types of binaries:
objdump, ldconfig, pacman (not on Debian), sandbox-app-launcher (not sure the attack surface of itself), its dependencies sudo, bash, bubblewrapTherefore it might make sense to run the parser (and its dependencies) itself through either sandbox-app-launcher, bubblewrap and/or some mandatory access control?
At the end of the script, check that variable all_paths contains only white listed characters? (To avoid crafted, malicious contents of being re-used in command line options later by sandbox-app-launcher calling sudo, bwrap.)
printf, /usr/bin/printf by coreutils: What’s the security record of that? Any way that printf can lead to unwanted code execution? Really required to use printf instead of bash’s built-in echo?
Merged.
This is to have as few levels of indentation/logic as possible. Closing an if/then statement whenever possible to keep complexity, mental load low.
I’ll look into improvement of some checks now as discussions earlier.
I am experiencing one strange issue.
sudo sandbox-app-launcher setup ristretto
INFO: Done, setup complete, OK.
sudo sandbox-app-launcher remove ristretto
userdel: sal-ristretto mail spool (/var/mail/sal-ristretto) not found
INFO: Done, removal complete, OK.
sal does the right things in bash xtrace as far as I can see…
adduser --home /home/sandbox-app-launcher-appdata/ristretto --no-create-home --> disabled-login --gecos '' sal-ristretto
Ok.
userdel --remove --force sal-ristretto
userdel: sal-ristretto mail spool (/var/mail/sal-ristretto) not found
Any idea?
cat /etc/passwd | grep ristretto
sal-ristretto:x:1001:1001:,:/home/sandbox-app-launcher-appdata/ristretto:/bin/bash
Should we change from default login shell /bin/bash to /bin/false?
To do so we would add to adduser:
--shell /bin/false
Will stop making changes for now.
Should remove also do the following?
## Don't leave any left-over processes such as the D-Bus daemon.
killall -9 -u "${app_user}"
Though, is going to be slightly more complicated as killall is asynchronous. Sends only a signal to process. Doesn’t block/wait until really killed the process.
Need to fix some bugs that I just introduced. Working on it now.
Well, that’s fixed and was an interesting exercise. However, I think the wrapper creation inside sal is quite ugly code wise.
Do we really need a dynamically generated wrapper script?
The only dynamic parts are really just A) set -x, bash xtrace for debugging or not which is not super important and could be implemented to only do that if some variable is set and B)
"/path/to/app" "${@}"
i.e. path to binary and command line options. All three variables (debug, app_path and args) could be set in sal. Then we could have a “static” script in a different file. sal would set appropriate variables and the script would serve the same purpose. No more need to have 1 script per application or dynamic wrapper script generation.
What do you think?
I’m not sure if it would be very strong. The impacts need more research.
It could make our dynamic native code execution / W^X restrictions stronger. Currently those don’t cover interpreters (i.e. shell, python, etc. scripts; this should probably be documented in the wiki) but with these restrictions, if the app doesn’t actually need them, those interpreters won’t be available within the sandbox for an attacker to possibly abuse. For example, with Chromium, bash and python aren’t accessible in the sandbox.
Another possible advantage is that it could limit the amount of code reuse gadgets available to an attacker in the sandbox to complement W^X. However, I highly doubt this will be significant and it will most likely be negligible since all necessary gadgets would probably be in libc, etc. already. There has been research on the ineffectiveness of removing gadgets within a singular binary ([1902.10880] Is Less Really More? Why Reducing Code Reuse Gadget Counts via Software Debloating Doesn't Necessarily Indicate Improved Security) and when considering a complete sandboxed environment with a lot of accessible binaries/libraries, it’d be even less effective.
grsecurity’s RBAC did.
Maybe. Vulnerabilities have been discovered in objdump before: CVE - Search Results
We could just use echo.
That’s a harmless error. userdel --remove attempts to delete the mail spool, as well as the home folder but since we don’t setup a mail spool, there’s nothing to delete.
Yes.
That would be great.
Done.
Currently we don’t have proper whitespace handling.
bash -c "
bwrap \
--ro-bind /bin /bin \
...
--ro-bind ${main_app_dir}/machine-id /etc/machine-id \
...
10< <(getent passwd root ${app_user} nobody) \
11< <(getent group root ${app_user} nobody) \
12< ${seccomp_filter} \
${bwrap_args} \
${wrapper_script} ${@}"
User names or folder names with white spaces wouldn’t work, but we don’t have these and check that there are no such characters.
Arguments however ${@} would be broken. Such as
sandbox-app-launcher start vlc "my video"
would break. That would result in VLC trying to open a file my and a file video.
It could be made to work similar to: proper whitespace handling · Kicksecure/apparmor-profile-everything@d3eccd4 · GitHub
But since the brwap command is very lengthy, that would be rather ugly.
I guess we couldn’t even use
command+=("--ro-bind /bin /bin")
command+=("--ro-bind /usr/bin /usr/bin")
...
Because that would be interpreted as:
‘–ro-bind /bin /bin’
And not as intended as:
–ro-bind /bin /bin
However, the following would probably work
command+=("--ro-bind")
command+=("/bin")
command+=("/bin")
...
bash -c "${command[@]}"
but make code look much worse. (60 lines of bwrap command would become I guestimate 180 lines.)
The issue comes from bash subshell opening with another double quote bash -c ":
sudo \
...
bash -c "
bwrap \
...
--seccomp 12 \
10< <(getent passwd root ${app_user} nobody) \
11< <(getent group root ${app_user} nobody) \
12< ${seccomp_filter} \
${bwrap_args} \
${wrapper_script} ${@}"
Do you think it would be a good idea to move that into its own /usr/share/sandbox-app-launcher/bwrap-wrapper script? Then I believe whitespace handling could be easily fixed.
(No need for command+=("--ro-bind") or "${command[@]}".)
I think it’s better to just lay a few files in the root mount namespace shared by multiple applications and guard the files with apparmor so that only applications that need the shared files can access them.
How would that fix it? We’d still need the subshell for the file descriptors.
Yes, but that is OK…?
We’d have proper whitespace handling… The pseudo code:
sandbox-app-launcher:
sudo \
--set-home \
--user="${app_user}" \
sandbox_app_launcher_debug="$sandbox_app_launcher_debug" \
app_path="$app_path" \
/usr/share/sandbox-app-launcher/bwrap-wrapper
bwrap-wrapper:
bwrap \
...
--ro-bind "${main_app_dir}/machine-id /etc/machine-id" \
--ro-bind "${wrapper_script}" "${wrapper_script}" \
10< <(getent passwd root "${app_user}" nobody) \
11< <(getent group root "${app_user}" nobody) \
12< "${seccomp_filter}" \
"${bwrap_args}" \
"${wrapper_script}" "${@}"
Using "${@}" (or "$@") would fix whitespace handling.
If that sounds good, I am eager to implement this.
Ah, I understand now. That would be good to implement.
It’s implemented. We have now proper whitespace support. Please have a look.
Not too beautiful code. Some imperfections.
bwrap_args still does not have proper whitespace support but that’s not user facing since path to shared_dir is hardcoded.sandbox-app-launcher to brwap-wrapper since we cannot preserve environment (sudo -E).sandbox-app-launcher start vlc "abc cde" "file 2"
This is actually good:
[0000738ee3f89d00] filesystem stream error: cannot open file /home/sandbox-app-launcher-appdata/vlc/abc cde (No such file or directory)
[0000738ee3f8f480] filesystem stream error: cannot open file /home/sandbox-app-launcher-appdata/vlc/file 2 (No such file or directory)
Your input can’t be opened:
VLC is unable to open the MRL ‘file:///home/sandbox-app-launcher-appdata/vlc/abc%20cde’. Check the log for details.
Your input can’t be opened:
VLC is unable to open the MRL ‘file:///home/sandbox-app-launcher-appdata/vlc/file%202’. Check the log for details.
It shows proper whitespace hanlding. Previously VLC (example, any application) would have attempted to open 4 files (abc, def, file, 2) instead of 2 files (“abc def”, “file 2”).
Looks good to me.