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

Filter environment variables #3322

Merged

Conversation

topimiettinen
Copy link
Collaborator

Save all environment variables for later use in sandboxes, clear
environment and re-apply only whitelisted variables for the main
firejail process. All variables will be reapplied for the sandboxes by
env_apply().

@topimiettinen topimiettinen added enhancement New feature request security Security issues and discussions labels Apr 6, 2020
@topimiettinen
Copy link
Collaborator Author

The commit eliminates the difference between start_application(0, fp) and start_application(1, fp) as now variables are applied unconditionally (because some of them don't exist anymore after clearenv()). To be honest, I'm not sure if there is now a change in behavior in some cases.

@topimiettinen
Copy link
Collaborator Author

Actually I'm pretty sure that all variables changed with setenv() in main Firejail process may be overwritten with the original values. So those setenv()s have to be changed to env_store()s.

@topimiettinen topimiettinen changed the title Filter environment variables WIP: Filter environment variables Apr 6, 2020
"LC_MESSAGES",
"PATH",
"SHELL",
"TMP",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why TMP is whitelisted?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used TMP to fix #2685

@smitsohu
Copy link
Collaborator

smitsohu commented Apr 6, 2020

To be honest, I'm not sure if there is now a change in behavior in some cases.

A while back I had what I think is the same problem with sanitizing the umask. So if you like you can look out for umask(orig_umask) calls in order to find the exit points.

@Fred-Barclay Fred-Barclay added the WIP: DON'T MERGE A PR that is still being worked on label Apr 6, 2020
@topimiettinen topimiettinen force-pushed the filter-environment-variables branch 3 times, most recently from 8820019 to d30d3d5 Compare April 7, 2020 15:25
@topimiettinen
Copy link
Collaborator Author

In this version, the variables which Firejail uses internally are also moved to env storage for consistency. Thus the whitelist only applies to variables used by external libraries (libc/pthreads, libapparmor) and then it can be much shorter: only LANG, LANGUAGE, LC_MESSAGES.

Perhaps the sandboxed helper apps (fseccomp & friends) should also be run with a short list of variables: LANG etc. and internal variables starting with FIREJAIL_, otherwise they might need similar sanitizing (or perhaps they also need some sanitizing in any case to be sure). Only the final user app should get all variables restored.

@topimiettinen
Copy link
Collaborator Author

Latest build failure happens because tests expect that using --env=FIREJAIL_TEST_ARGUMENTS=yes (or exporting the environment variable FIREJAIL_TEST_ARGUMENTS from shell), the variable is passed also to the helper apps. Maybe this should be instead flagged with some other means, for example adding arguments to faudit like faudit --test-arguments. It would be simpler to just let the the variable pass.

@msva
Copy link
Contributor

msva commented Feb 8, 2021

@topimiettinen ping?

Any progress here? ;)

We're all struggling (from env length checks) without your work ;)

@topimiettinen
Copy link
Collaborator Author

Sorry, I actually made a version which lets FIREJAIL_TEST_ARGUMENTS pass, but forgot to follow up. Thanks for the reminder. Let's see what the CIs think of the rebased version.

Save all environment variables for later use in the application, clear
environment and re-apply only whitelisted variables for the main
firejail process. The whitelisted environment is only used by C
library. Sandboxed tools will get further variables used
internally (FIREJAIL_*).

All variables will be reapplied for the firejailed application.

This also lifts the length restriction for environment variables,
except for the variables used by Firejail itself or the sandboxed
tools.
@topimiettinen
Copy link
Collaborator Author

Adding also PATH let the CI pass!

@topimiettinen topimiettinen removed the WIP: DON'T MERGE A PR that is still being worked on label Feb 8, 2021
@topimiettinen topimiettinen changed the title WIP: Filter environment variables Filter environment variables Feb 8, 2021
@msva
Copy link
Contributor

msva commented Feb 9, 2021

@netblue30 ping? :D

@netblue30 netblue30 merged commit 06e6dfe into netblue30:master Feb 9, 2021
@netblue30
Copy link
Owner

all merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request security Security issues and discussions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants