libsandbox: resolve_dirfd_path /proc/<pid> namespace safety #1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
If /proc was mounted by a process in a different pid namespace, getpid cannot be used create a valid /proc/<pid> path. Instead use sb_get_fd_dir() which works in any case. This implements option 3 of these choices: 1) Always create a mount namespace when creating a pid namespace, and remount /proc so that /proc/<pid> entries are always consistent with the current pid namespace. 2) Use readlink on /proc/self instead of getpid to determine the pid of self in the pid namespace of the /proc mount. 3) Use /proc/self or /dev/fd directly. Bug: https://bugs.gentoo.org/670966 Signed-off-by: Zac Medico <zmedico@gentoo.org>
|
Thanks. Given no feedback, I'm going to merge this. |
gentoo-bot
pushed a commit
that referenced
this pull request
Mar 15, 2021
In bug #775842 Sam noticed sandbox crash on libabigail-1.8.2 testsuite:
#0 0x00007f0e0d10e392 in sb_new_envp ()
at libsandbox.c:1184
#1 0x00007f0e0d112745 in system_DEFAULT ()
at wrapper-funcs/__wrapper_exec.c:315
#2 0x000055ece6570b52 in test_task::perform (this=0x55ece72978b0)
at /usr/lib/gcc/x86_64-pc-linux-gnu/11.0.1/include/g++-v11/bits/basic_string.h:186
#3 0x00007f0e0d05fce5 in abigail::workers::worker::wait_to_execute_a_task (p=0x55ece728f380)
at /tmp/portage/dev-util/libabigail-1.8.2/work/libabigail-1.8.2/src/abg-workers.cc:415
#4 0x00007f0e0c8f7cae in start_thread (arg=0x7f0e0b433640)
at pthread_create.c:473
#5 0x00007f0e0ca0eb2f in clone ()
at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
The test roughly does call system() from spawned parallel threads:
for_each_test(){
spawn_thread([]{
system("cmd"); wait();
}
}
Sandbox has to inject sandbox-specific environment variables where
they don't exist. This is fundamentally racy in multithreaded
environment:
for_each_test(){
spawn_thread([]{
environ = modified_env;
system("cmd"); wait();
environ = orig_env;
}
}
Most of the time environment does not have to change after initial
environment injection. f3e51a9 ("exec*() wrappers: never mutate
'environ' of host process") exposed a bug in sandbox's logic of
checking existing environment: unset variables like `SANDBOX_DENY`
The change treats unset/expected-unset variables as non deviating.
Reported-by: Sam James
Bug: https://bugs.gentoo.org/775842
Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If /proc was mounted by a process in a different pid namespace,
getpid cannot be used create a valid /proc/ path. Instead
use sb_get_fd_dir() which works in any case. This implements
option 3 of these choices:
Always create a mount namespace when creating a pid namespace,
and remount /proc so that /proc/ entries are always consistent
with the current pid namespace.
Use readlink on /proc/self instead of getpid to determine the pid
of self in the pid namespace of the /proc mount.
Use /proc/self or /dev/fd directly.
Bug: https://bugs.gentoo.org/670966
Signed-off-by: Zac Medico zmedico@gentoo.org