System-wide sandboxing framework - sandbox-app-launcher

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().

1 Like

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.

1 Like

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.

1 Like

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.

Debian paste error

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.

2 Likes

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:

  • Automatically creating a mandatory access control profile based on parsing the binary (or source code).
  • Automatically creating an apparmor profile based on parsing the binary (or source code).

I am wondering why nobody earlier got that idea? Or did someone?

A bit similar to this:

Design:

  • For sure keep this in a separate file for better abstraction. Might even be useful for some other purpose. A good addition to https://gitlab.com/whonix/helper-scripts? (Not important if you prefer to keep all inside sandbox-app-launcher package.)
  • helper-scripts package shouldn’t change anything on the system. Should have most minimal risk of breaking unrelated packages that don’t depend on it. Should just be a vehicle to ship files. Therefore the parser could be added there.
  • The caching of parsed information, folder creation however belong into sandbox-app-launcher package since that is making more intrusive file system modifications.
  • Have a feature in sandbox-app-launcher to disable/enable this feature per application using the usual config snippets. (Default: enabled, if attainable/realistic development milestone.)

With sandbox-app-launcher we have to worry about too things:

  • local privilege escalation vulnerabilities
  • sandbox escape vulnerabilities

And sandbox-app-launcher might be used to start two different types of binaries:

  • those which “only” contain vulnerabilities and/or bugdoors
  • those containing specifically crafted code intended to exploit the parser and its dependencies (currently objdump, ldconfig, pacman (not on Debian), sandbox-app-launcher (not sure the attack surface of itself), its dependencies sudo, bash, bubblewrap

Therefore 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?

1 Like

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.

1 Like

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.

1 Like

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.

1 Like

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?

1 Like

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.

1 Like

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.

1 Like

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.

  • /run/user-id/sway-ipc-*
  • /run/user-id/wayland-*
  • shared dbus sessions.

How would that fix it? We’d still need the subshell for the file descriptors.

1 Like

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.

2 Likes

Ah, I understand now. That would be good to implement.

2 Likes

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.
  • Lots of variables have to be passed from sandbox-app-launcher to brwap-wrapper since we cannot preserve environment (sudo -E).
2 Likes
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”).

1 Like

Looks good to me.

1 Like