WIP: secontext: fixed retrieval of symlink context and /proc/self/ handling #336
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #336 +/- ##
=======================================
Coverage 88.03% 88.03%
=======================================
Files 289 289
Lines 25418 25418
Branches 3671 3671
=======================================
Hits 22377 22377
Misses 2073 2073
Partials 968 968 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if ((get_expected_filecontext(buf, expected, st.st_mode) < 0) && (errno != ENOENT)) | ||
| *expected = (char *) unknown_expected_context; |
There was a problem hiding this comment.
Given that this unknown_expected_context assignment logic is repeated twice, what would you think about moving it inside get_expected_filecontext()?
There was a problem hiding this comment.
We could, but then get_expected_filecontext() should always return 0, even in case of failure (due to SELinux library for example) and we would lose the reason for failure.
I think it's better to handle this outside of the function, since it's kind of customization: "I couldn't guess the context, so I'm printing "unknown""
There was a problem hiding this comment.
If changing get_expected_filecontext() doesn't sound appealing, what about creating a thin wrapper around it that would implement this unknown_expected_context assignment?
There was a problem hiding this comment.
By the way, get_expected_filecontext currently doesn't set errno in case of cached disabled == true, leading to undefined behavior.
There were multiple issues with symlink handling.
1. When the path being queried is a symlink, we expect the context of
the symlink to be displayed, not the one of the target. Due to this,
lgetfilecon() has to be used instead of getfilecon() in
selinux_getfilecon(). For selinux_getfdcon(), this doesn't apply
because we cannot query a symlink.
2. When in 'mismatch' mode, querying the expected context for symlinks
becomes harder, because realpath() cannot be used, since it would
resolve the symlink, hence the query would end up on the target
instead of the symlink itself. To solve this, only the dirname of the
path is resolved.
3. SELinux context queries are performed through prefixing by
/proc/<PID>/{root,cwd,fd/<FD>}/. This led to querying strace's
namespace instead of PID's namespace when resolving paths such as
/proc/self/something, because /proc/self always points to ourselves
(hence strace). Because of this /proc/self must be substituted as
/proc/<PID> first, but this may not always work, because there could
be a "fake" /proc/self" in the path. To solve this, the context query
is performed on the substituted path and if there is no result, the
original path is queried as fallback.
4. Unrelated to symlinks, there was no distinction between not being
able to retrieve the expected context due to some error and not
having any known expected context. To solve this, on error, a "??"
static context is returned.
| if (stat_file(linkpath, &st)) | ||
| if (stat_file(buf, &st) < 0) |
There was a problem hiding this comment.
Sorry, I don't follow this particular change. Isn't stat(linkpath) more reliable in this case than stat(buf)?
There was a problem hiding this comment.
We want here the context of the target pointed by the symlink /proc/<pid>/fd/<fd>, not the symlink itself, hence the stat on buf.
There was a problem hiding this comment.
As stat_file() follows symlinks, there shouldn't be an issue, unless the file is deleted, in that case stat_file(buf) wouldn't be able to access it while stat_file(linkpath) would.
| "/proc/self", "/proc/self/root/proc/self" | ||
| }; | ||
|
|
||
| for (unsigned int i = 0; i < sizeof(proc_self_paths)/sizeof(*proc_self_paths); i++) { |
There was a problem hiding this comment.
for (unsigned int i = 0; i < ARRAY_SIZE(proc_self_paths); i++) {| char *c; | ||
| for (c = secontext + strlen(secontext) - 1; c > secontext; c--) | ||
| *c = *(c - 1); | ||
| *c = ' '; |
There was a problem hiding this comment.
memmove(secontext + 1, secontext, strlen(secontext) - 1);
secontext[0] = ' ';| fd = syscall(__NR_openat, cwd_fd, proc_self_paths[i], O_RDONLY); | ||
| printf("%s%s(%d%s, \"%s\"%s, O_RDONLY) = %s%s\n", | ||
| my_secontext, "openat", | ||
| cwd_fd, cwd_secontext, | ||
| proc_self_paths[i], secontext, | ||
| sprintrc(fd), secontext); |
There was a problem hiding this comment.
For some reason this doesn't pass CI on ubuntu-22.04 runners with the following diagnostics:
-[unconfined] openat(4, "/proc/self" [unconfined], O_RDONLY) = 5 [unconfined]
-[unconfined] openat(4, "/proc/self/root/proc/self" [unconfined], O_RDONLY) = 5 [unconfined]
+[unconfined] openat(4, "/proc/self", O_RDONLY) = 5
+[unconfined] openat(4, "/proc/self/root/proc/self", O_RDONLY) = 5
There were multiple issues with symlink handling.
When the path being queried is a symlink, we expect the context of the symlink to be displayed, not the one of the target. Due to this, lgetfilecon() has to be used instead of getfilecon() in selinux_getfilecon(). For selinux_getfdcon(), this doesn't 8000 apply because we cannot query a symlink.
When in 'mismatch' mode, querying the expected context for symlinks becomes harder, because realpath() cannot be used, since it would resolve the symlink, hence the query would end up on the target instead of the symlink itself. To solve this, only the dirname of the path is resolved.
SELinux context queries are performed through prefixing by /proc//{root,cwd,fd/}/. This led to querying strace's namespace instead of PID's namespace when resolving paths such as /proc/self/something, because /proc/self always points to ourselves (hence strace). Because of this /proc/self must be substituted as /proc/ first, but this may not always work, because there could be a "fake" /proc/self" in the path. To solve this, the context query is performed on the substituted path and if there is no result, the original path is queried as fallback.
Unrelated to symlinks, there was no distinction between not being able to retrieve the expected context due to some error and not having any known expected context. To solve this, on error, a "??" static context is returned.