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.

Sign up for GitHub

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

pkexec: Use realpath when comparing org.freedesktop.policykit.exec.path #509

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

pkexec: Use realpath when comparing org.freedesktop.policykit.exec.path #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions src/programs/pkexec.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,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 @@ -507,6 +508,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 @@ -623,6 +625,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 @@ -646,7 +650,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 @@ -661,9 +665,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;
Copy link
Member

@jrybar-rh jrybar-rh Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Either path shall be stored in a new variable path_abs to use it later in unchanged form XOR argv[n] can be replaced with strdup(path) directly (which I hate). But this solution makes path_abs redundant. Or I don't follow.

Copy link
Author

@wdoekes wdoekes Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Your quoting skips this bit, which is also relevant:

        /* Must copy because we might replace path later on. */
        path_abs = g_strdup(path);

The case is as follows:

  • Absolute path -> no problem:

    • argv[n] stays untouched
    • path_abs stays NULL
    • realpath is invoked, path gets freed/replaced

  • Relative path (before path_abs):

    • argv[n] got path
    • realpath is invoked, path is freed
    • argv[n] points to freed mem

  • Relative path (now):

    • argv[n] gets copy of path in path_abs
    • realpath is invoked, path is freed
    • argv[n] points to path_abs
    • path_abs is freed at the end (just like path)

I could do without path_abs, but then we don't know what to free() at the end.

path shall be stored in a new variable path_abs to use it later in unchanged form

Before my patch, we had this:

  • argv[n] is untouched XOR argv[n] is path (1st strdup)

This means we only had to free(path) at the end.

After my patch, we have this:

  • argv[n] is untouched XOR argv[n] is path_abs (2nd strdup)

At line 688 (g_free (path)), we don't know whether argv[n] has path or is untouched. And if we did we'd have to free(argv[n]) later on, which seems very ugly to me.

So, path is now stored in path_abs and used in unchanged form, but as a copy. Correct?

XOR argv[n] can be replaced with strdup(path)

Yes, so I didn't want to do:

argv[n] = g_strdup (path);

For two reasons:

  1. We don't know whether to free(argv[n]) at the end. And it is ugly.

  2. If we're out of mem, then argv[n] becomes NULL. I'd rather have it stay relative-pathed.

Is that the issue?

The alternative of keeping path in argv[n] would require additional bookkeeping of whether argv[n] is set, which makes for harder to read code. You'd get something like this (on top of my patch):

diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index e7d6cc8..208751e 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -629,7 +629,7 @@ main (int argc, char *argv[])
    * And then we resolve the real path of the program.
    */
   g_assert (argv[argc] == NULL);
-  path = g_strdup (argv[n]);
+  path = path_abs = g_strdup (argv[n]);
   if (path == NULL)
     {
       GPtrArray *shell_argv;
@@ -658,18 +658,15 @@ main (int argc, char *argv[])
           goto out;
         }
       g_free (path);
-      path = s;
+      path = path_abs = s;
 
       /* argc<2 and pkexec runs just shell, argv is guaranteed to be null-terminated.
        * /-less shell shouldn't happen, but let's be defensive and don't write to null-termination
        */
       if (argv[n] != NULL)
         {
-        /* 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;
         }
     }
@@ -685,7 +682,11 @@ main (int argc, char *argv[])
        * argv[n] this time. The called program still sees the original
        * called path. This is very important for multi-call binaries like
        * busybox. */
+      if (path != argv[n])
+        {
+          /* path/path_abs was not used in argv[n] */
           g_free (path);
+        }
       path = s;
     }
   if (access (path, F_OK) != 0)
@@ -1102,6 +1103,7 @@ main (int argc, char *argv[])
     }
 
   g_free (original_cwd);
+  if (path_abs != path)
     g_free (path_abs);
   g_free (path);
   g_free (command_line);

Copy link
Author

Choose a reason for hiding this comment

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

Or.. unconditionally copy path_abs always. Which is also an option and keeps things sane:

diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index e7d6cc8..e145e11 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -630,7 +630,8 @@ main (int argc, char *argv[])
    */
   g_assert (argv[argc] == NULL);
   path = g_strdup (argv[n]);
-  if (path == NULL)
+  path_abs = g_strdup (path);
+  if (path == NULL || path_abs == NULL)
     {
       GPtrArray *shell_argv;
 
@@ -650,26 +651,27 @@ main (int argc, char *argv[])
     }
   if (path[0] != '/')
     {
+      gchar *s2;
       /* g_find_program_in_path() is not susceptible to attacks via the environment */
       s = g_find_program_in_path (path);
-      if (s == NULL)
+      s2 = g_strdup(s);
+      if (s == NULL || s2 == NULL)
         {
           g_printerr ("Cannot run program %s: %s\n", path, strerror (ENOENT));
           goto out;
         }
       g_free (path);
+      g_free (path_abs);
       path = s;
+      path_abs = s2;
 
       /* argc<2 and pkexec runs just shell, argv is guaranteed to be null-terminated.
        * /-less shell shouldn't happen, but let's be defensive and don't write to null-termination
        */
       if (argv[n] != NULL)
         {
-        /* 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;
         }
     }

Copy link
Author

Choose a reason for hiding this comment

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

That last diff as absolute patch would be:

diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index 61c9100..c5e3703 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -451,6 +451,7 @@ main (int argc, char *argv[])
   gchar *action_id;
   gboolean allow_gui;
   gchar **exec_argv;
+  gchar *path_noncanonical;
   gchar *path;
   struct passwd pwstruct;
   gchar pwbuf[8192];
@@ -507,6 +508,7 @@ main (int argc, char *argv[])
   result = NULL;
   action_id = NULL;
   saved_env = NULL;
+  path_noncanonical = NULL;
   path = NULL;
   exec_argv = NULL;
   command_line = NULL;
@@ -623,10 +625,13 @@ 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]);
-  if (path == NULL)
+  path_noncanonical = g_strdup (path);
+  if (path == NULL || path_noncanonical == NULL)
     {
       GPtrArray *shell_argv;
 
@@ -646,23 +651,45 @@ main (int argc, char *argv[])
     }
   if (path[0] != '/')
     {
-      /* g_find_program_in_path() is not suspectible to attacks via the environment */
+      gchar *s2;
+      /* g_find_program_in_path() is not susceptible to attacks via the environment */
       s = g_find_program_in_path (path);
-      if (s == NULL)
+      s2 = g_strdup(s);
+      if (s == NULL || s2 == NULL)
         {
           g_printerr ("Cannot run program %s: %s\n", path, strerror (ENOENT));
           goto out;
         }
       g_free (path);
+      g_free (path_noncanonical);
       path = s;
+      path_noncanonical = s2;
 
       /* argc<2 and pkexec runs just shell, argv is guaranteed to be null-terminated.
        * /-less shell shouldn't happen, but let's be defensive and don't write to null-termination
        */
       if (argv[n] != NULL)
         {
-        argv[n] = 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. */
+          argv[n] = path_noncanonical;
+        }
     }
+#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 (path_noncanonical). This is very important for multi-call
+       * binaries like busybox. */
+      g_free (path);
+      /* We use the canonical path for security/permission lookups. */
+      path = s;
     }
   if (access (path, F_OK) != 0)
     {
@@ -1078,6 +1105,7 @@ main (int argc, char *argv[])
     }
 
   g_free (original_cwd);
+  g_free (path_noncanonical);
   g_free (path);
   g_free (command_line);
   g_free (cmdline_short);

That would work for me too 🤷

}
}
#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 @@ -1078,6 +1102,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
Open in desktop
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