Skip to content
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

Merged
merged 5 commits into from
May 18, 2021
Merged

Whitelist2 #4229

merged 5 commits into from
May 18, 2021

Conversation

smitsohu
Copy link
Collaborator

@smitsohu smitsohu commented May 1, 2021

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

@rusty-snake
Copy link
Collaborator

except /run

No way to target #3189 here too?

@smitsohu
Copy link
Collaborator Author

smitsohu commented May 2, 2021

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.

@smitsohu smitsohu force-pushed the whitelist2 branch 2 times, most recently from 7c63d76 to 3ab6474 Compare May 3, 2021 00:29
@smitsohu
Copy link
Collaborator Author

smitsohu commented May 3, 2021

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.

@glitsj16
Copy link
Collaborator

glitsj16 commented May 3, 2021

@smitsohu Another great piece of coding! Whenever you think this is ready for testing, feel free to ping.

@smitsohu smitsohu force-pushed the whitelist2 branch 4 times, most recently from 44f3981 to 4e66e55 Compare May 4, 2021 20:17
@smitsohu
Copy link
Collaborator Author

smitsohu commented May 7, 2021

@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.

@smitsohu smitsohu force-pushed the whitelist2 branch 2 times, most recently from 4019e3c to 62fd782 Compare May 8, 2021 14:28
@netblue30 netblue30 merged commit ed7db09 into netblue30:master May 18, 2021
@netblue30
Copy link
Owner

Let's try it out, thanks!

smitsohu added a commit to smitsohu/firejail that referenced this pull request May 23, 2021
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)
smitsohu added a commit to smitsohu/firejail that referenced this pull request May 23, 2021
@smitsohu smitsohu mentioned this pull request May 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[enhancement] Allow whitelisting arbitrary directories
4 participants