Skip to content

Commit

Permalink
pkexec: Use realpath when comparing org.freedesktop.policykit.exec.path
Browse files Browse the repository at this point in the history 
This changes the pkexec path that is compared from the original supplied
path to the path resolved by realpath(3).

That means that "/bin/something" might now be matched as
"/usr/bin/something", a review of your
  <annotate key="org.freedesktop.policykit.exec.path">
actions might be in order.

Fixes: #194

See also: systemd/systemd#34714
  • Loading branch information
@wdoekes @jrybar-rh
wdoekes authored and jrybar-rh committed Feb 18, 2025
1 parent 11c4a81 commit 9aa43e0
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 2 deletions.
29 changes: 27 additions & 2 deletions src/programs/pkexec.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ main (int argc, char *argv[])
gchar *action_id;
gboolean allow_gui;
gchar **exec_argv;
gchar *path_abs;
gchar *path;
struct passwd pwstruct;
gchar pwbuf[8192];
Expand Down Expand Up @@ -508,6 +509,7 @@ main (int argc, char *argv[])
result = NULL;
action_id = NULL;
saved_env = NULL;
path_abs = NULL;
path = NULL;
exec_argv = NULL;
command_line = NULL;
Expand Down Expand Up @@ -624,6 +626,8 @@ main (int argc, char *argv[])
* but do check this is the case.
*
* We also try to locate the program in the path if a non-absolute path is given.
*
* And then we resolve the real path of the program.
*/
g_assert (argv[argc] == NULL);
path = g_strdup (argv[n]);
Expand All @@ -647,7 +651,7 @@ main (int argc, char *argv[])
}
if (path[0] != '/')
{
/* g_find_program_in_path() is not suspectible to attacks via the environment */
/* g_find_program_in_path() is not susceptible to attacks via the environment */
s = g_find_program_in_path (path);
if (s == NULL)
{
Expand All @@ -662,9 +666,29 @@ main (int argc, char *argv[])
*/
if (argv[n] != NULL)
{
argv[n] = path;
/* Must copy because we might replace path later on. */
path_abs = g_strdup(path);
/* argv[n:] is used as argv arguments to execv(). The called program
* sees the original called path, but we make sure it's absolute. */
if (path_abs != NULL)
argv[n] = path_abs;
}
}
#if _POSIX_C_SOURCE >= 200809L
s = realpath(path, NULL);
#else
s = NULL;
# error We have to deal with realpath(3) PATH_MAX madness
#endif
if (s != NULL)
{
/* The called program resolved to the canonical location. We don't update
* argv[n] this time. The called program still sees the original
* called path. This is very important for multi-call binaries like
* busybox. */
g_free (path);
path = s;
}
if (access (path, F_OK) != 0)
{
g_printerr ("Error accessing %s: %s\n", path, g_strerror (errno));
Expand Down Expand Up @@ -1084,6 +1108,7 @@ main (int argc, char *argv[])
}

g_free (original_cwd);
g_free (path_abs);
g_free (path);
g_free (command_line);
g_free (cmdline_short);
Expand Down
23 changes: 23 additions & 0 deletions test/integration/pkexec/test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,26 @@ sudo -u "$TEST_USER" expect "$TMP_DIR/SIGTRAP-on-EOF.exp" | tee "$TMP_DIR/SIGTRA
grep -q "AUTHENTICATION FAILED" "$TMP_DIR/SIGTRAP-on-EOF.log"
grep -q "Not authorized" "$TMP_DIR/SIGTRAP-on-EOF.log"
rm -f "$TMP_DIR/SIGTRAP-on-EOF.log"

: "Check absolute (but not canonicalized) path"
BASH_ABS=$(command -v bash)
ln -s "$BASH_ABS" ./my-bash
sudo -u "$TEST_USER" expect "$TMP_DIR/basic-auth.exp" "$TEST_USER_PASSWORD" ./my-bash -c true | tee "$TMP_DIR/absolute-path.log"
grep -Eq "Authentication is needed to run \`/.*/${PWD##*/}/./my-bash -c true' as the super user" "$TMP_DIR/absolute-path.log"
grep -q "AUTHENTICATION COMPLETE" "$TMP_DIR/absolute-path.log"
rm -f "$TMP_DIR/absolute-path.log"
rm -f "./my-bash"

: "Check canonicalized path"
if command -v strace; then
BASH_ABS=$(command -v bash)
ln -s "$BASH_ABS" ./my-bash
sudo -u "$TEST_USER" strace -s 512 -o "$TMP_DIR/canonical-path.strace" -feexecve \
expect "$TMP_DIR/basic-auth.exp" "$TEST_USER_PASSWORD" ./my-bash -c true | tee "$TMP_DIR/canonical-path.log"
cat "$TMP_DIR/canonical-path.strace"
grep -qF "execve(\"$BASH_ABS\", [\"$PWD/./my-bash\"," "$TMP_DIR/canonical-path.strace"
grep -q "AUTHENTICATION COMPLETE" "$TMP_DIR/canonical-path.log"
rm -f "$TMP_DIR/canonical-path.log" "$TMP_DIR/canonical-path.strace"
rm -f "./my-bash"
rm -f "$TMP_DIR/preload.c" "$TMP_DIR/preload.so"
fi

2 comments on commit 9aa43e0

@jrybar-rh
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, seems like _POSIX_C_SOURCE >= 200809L is too new for Alpine, breaking our CI, which I hadn't noticed before merging. I'll investigate it later today.

@mrc0mmand
Copy link
Member

@mrc0mmand mrc0mmand commented on 9aa43e0 Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, the issue here is that glibc defines _POSIX_C_SOURCE itself if not defined, whereas musl doesn't this:

#include <stdio.h>

int main(void) {
        printf("_POSIX_C_SOURCE: %ld\n", _POSIX_C_SOURCE);
        return 0;
}

glibc:

$ rpm -q glibc
glibc-2.39-30.fc40.x86_64
glibc-2.39-30.fc40.i686
$ gcc -o test test.c
$ ./test 
_POSIX_C_SOURCE: 200809

musl:

# apk info musl
musl-1.2.5-r9
...
# gcc -o test test.c
test.c: In function 'main':
test.c:4:42: error: '_POSIX_C_SOURCE' undeclared (first use in this function)
    4 |         printf("_POSIX_C_SOURCE: %ld\n", _POSIX_C_SOURCE);
      |                                          ^~~~~~~~~~~~~~~
test.c:4:42: note: each undeclared identifier is reported only once for each function it appears in

So, again IIUC, to make this work we'd have to define this feature macro ourselves (i.e. throw it next to our _GNU_SOURCE definition here in meson.build) to make this work properly.

Please sign in to comment.