-
Notifications
You must be signed in to change notification settings - Fork 588
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Whitelist2 #4229
Whitelist2 #4229
Conversation
No way to target #3189 here too? |
There was not really a strong reason to deny /run. I'll add /run top level directory support. |
7c63d76
to
3ab6474
Compare
Ok, I rewrote the messed up commit history and added support for /run. Two nested whitelist top level directories (/run, /run/user/$UID and user home) and all the automatisms are a bit difficult to get right, but I think I'm now close to it. Something of note: firejail now runs without /run/firejail for a short time, after mounting the tmpfs on /run. The only affected (firejail) function is fs_tmpfs itself, which doesn't seem to need it currently. |
@smitsohu Another great piece of coding! Whenever you think this is ready for testing, feel free to ping. |
44f3981
to
4e66e55
Compare
@glitsj16 If you want you can give it a try. There are still two warnings from the gcc static analyzer, I need to look at them more closely but think they are false positives. |
4019e3c
to
62fd782
Compare
Let's try it out, thanks! |
besides some cosmetic tweaks, fixes --whitelist=/a/b where /a/b is a symbolic link to /a/c/d and c is the user home directory: create path as user and not as root. (going forward, a better and more comprehensive fix would be to prevent all mount point traversals in whitelist_mkpath, but it will take a bit of time to implement)
What is the reason for not allowing /proc? With a blacklist, there's the risk that some kernel modules expose some sensitive information in nonstandard /proc/ files, so I'd rather whitelist the files that I know the application needs, rather than risking to give access to unforeseen files. |
Yes, it would be desirable to include /proc and /sys. I excluded /proc mainly because Firejail itself still needs to access it after the whitelist has been set up, though one could argue that the same applies to blacklisting. Probably there is no really good reason to exclude /sys. The time I wrote this there was a bit too much respect/scare about subtle effects that missing files in /proc and /sys could have on other sandboxed setuid tools. |
The procfs is not an easy target for a sandboxing tools anyways, because per-process subdirectories are created after the sandbox is up already. In many cases AppArmor, Tomoyo, SELinux will serve you better. Do you have a particular kernel module in mind? Perhaps something can be done using the |
To further complicate this, there are programs that check whether /proc/PATH is somehow tampered with: https://docs.rs/rustix/latest/rustix/procfs/index.html |
Somewhat experimental whitelist implementation. Fixes #2041.
All top level directories are allowed except /proc, /sys and /run. As an exception from the exception, /sys/module and /run/user/$UID are allowed. This way all profiles will continue to work. Another special case is /usr, where the subdirectories (like /usr/share) are top level directories for the purpose of whitelisting.
For now all restrictions regarding symbolic links are gone (and
follow-symlink-as-user
from firejail.config is without effect). I'm not entirely sure if that is sustainable, but it can always be added back.Otherwise this implementation should be very close to the current one.
Maybe it would also make sense to reimplement private-lib as whitelist then, in order to prevent name collisions as in #3236