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

Upstreaming a set of fixes from Sailfish's packaging #3998

Merged
merged 5 commits into from
Feb 24, 2021

Conversation

Tomin1
Copy link
Contributor

@Tomin1 Tomin1 commented Feb 19, 2021

This is upstreaming first set of fixes from Sailfish's firejail packaging. These were developed as part of implementing firejail sandboxing in Sailfish OS. See also #3960 for discussion.

These are general fixes to firejail touching logging, private-etc, memory handing and symbolic link construction around /proc filesystem. Please see the commit messages for explanations. These were written by my colleague and previously reviewed by me or one of my other colleagues.

When constructing sandbox fs, /etc/mtab which is symlink to
/proc/self/mounts gets resolved as /proc/PID/mounts. Where
PID is not the pid of the process that is going to get
executed in the firejail -> the result is broken/unaccessible
symlink from the application point of view.

Use /proc/self/xxx type symlink target if it resolves similarly
as the /proc/PID/xxx type would at the time of mapping.

Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
These have little consequences as the tool exits anyway,
but fs_copydir() leaks memory on success path and check()
on failure path.

Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
@reinerh
Copy link
Collaborator

reinerh commented Feb 19, 2021

One of these changes causes a regression in the test suite:
https://github.com/netblue30/firejail/pull/3998/checks?check_run_id=1933635335

Private /etc installed in 22.23 ms
Error mount bind: fs_etc.c:194 fs_private_dir_mount: No such file or directory
Error: proc 4439 cannot sync with peer: unexpected EOF
Peer 4441 unexpectedly exited with status 1
runner@fv-az102-314:~/work/firejail/firejail/test/profiles$ TESTING ERROR 0

(the other errors are mostly the same)

It might be related to this line:

fs_private_dir_mount("/usr/etc", RUN_USR_ETC_DIR);

because /usr/etc doesnot exist in the test environment.

spiiroin and others added 3 commits February 22, 2021 10:01
Firejail uses file bind-mounts to filter /etc/passwd and /etc/group
content. If private-etc is used, these mounts are left underneath
the /etc directory mount and this seems to be causing problems in
devices with older kernels: attempts to modify passwd or group
data fails with EBUSY.

Make it possible to perform fs_private_dir_list() actions in two
separate phases.

Undo the file mounts in /etc before mounting private-etc content.

Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
Lacking linefeed chars cause messages to get concatenated.

Signed-off-by: Simo Piiroinen <simo.piiroinen@jolla.com>
Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
Check that the directory exists before attempting to mount it.

Signed-off-by: Tomi Leppänen <tomi.leppanen@jolla.com>
@Tomin1
Copy link
Contributor Author

Tomin1 commented Feb 22, 2021

Nice catch from the test suite! I added the same check that fs_private_dir_copy() has to fs_private_dir_mount() which results in some duplicated work but I think one more stat() is not that bad of a tradeoff. Also added those asserts just in case.

@netblue30 netblue30 merged commit 9a47e2d into netblue30:master Feb 24, 2021
@netblue30
Copy link
Owner

all set, thanks!

netblue30 added a commit that referenced this pull request Feb 24, 2021
smitsohu added a commit that referenced this pull request Feb 24, 2021
musl stdlib (Alpine Linux) doesn't know about canonicalize_file_name,
replace with equivalent realpath calls
@Tomin1 Tomin1 mentioned this pull request Mar 3, 2021
@matu3ba matu3ba mentioned this pull request Oct 7, 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.

4 participants