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

whitelist restructuring #4985

Merged
merged 3 commits into from
Mar 5, 2022
Merged

whitelist restructuring #4985

merged 3 commits into from
Mar 5, 2022

Conversation

smitsohu
Copy link
Collaborator

@smitsohu smitsohu commented Feb 25, 2022

Some reorganization/cleanup of the whitelist code. Also a new mechanism to detect earlier whitelist mounts.

There should be no functional changes.

as functions operate on a file descriptor
it should be safe to remove them; this
sets the stage for improvements to the
whitelist code
@smitsohu smitsohu force-pushed the whitelist branch 4 times, most recently from a4694b5 to 72deb4f Compare February 26, 2022 12:57
@smitsohu smitsohu marked this pull request as draft February 26, 2022 14:37
@smitsohu smitsohu force-pushed the whitelist branch 2 times, most recently from 791bc75 to ed87884 Compare February 27, 2022 00:41
@smitsohu smitsohu marked this pull request as ready for review February 27, 2022 00:50
smitsohu added 2 commits March 1, 2022 01:37
some cleanup, simplify extending the code (for example adding additional members to the TopDir struct)
Check mountids while creating path of a new mount target.
If the mountid differs from the top level directory (tmpfs)
mountid, this proves an earlier whitelist command.

It is important to note though that this check is not exhaustive,
as besides nested whitelist commands there are also nested
top level directories. So a user could run:
firejail --whitelist=/a/b --whitelist=/a/b/c where both
a and b are (whitelist) top level directories. Such a command
may result in b and c sharing the filesystem and hence mountid.
In this case the nested nature of the whitelist commands
will go unnoticed.

A more rigorous version will probably need to apply some
sorting to the whitelist command, possibly by means of
glob(3).
@glitsj16
Copy link
Collaborator

glitsj16 commented Mar 3, 2022

I didn't respond sooner as I noticed you were actively working on this and regularly pushing changes. Having some free time over the weekend I was planning on checking out your whitelist branch and give it a test run. Might be redundant to ask for a branch described as 'no functional changes', but if there's anything specific to look out for, feel free to throw in pointers.

@smitsohu
Copy link
Collaborator Author

smitsohu commented Mar 4, 2022

@glitsj16 thanks for trying it out :)

If you find any functional change it means that I got something wrong. If you do things like --whitelist=~/.local --whitelist=~/.local/share there will be a new debug message Debug 108: whitelisted already.

That's about it. Other than that it is just writing stuff in a different way but do the same thing. I was just hoping to organize the code a bit better.

@netblue30 netblue30 merged commit a19fb8f into netblue30:master Mar 5, 2022
@netblue30
Copy link
Owner

all in!

@smitsohu smitsohu deleted the whitelist branch March 5, 2022 12:33
kmk3 added a commit that referenced this pull request Mar 9, 2022
This amends commit 4813218 ("merges", 2022-03-05).

Relates to #4985 #4990 #5011.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

3 participants