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

[wdoekes]

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

actions might be in order.

Fixes: #194

See also: systemd/systemd#34714

NOTES

• This might cause backwards incompatibility issues.
• And that also has security implications. Although I think for the better.

[jrybar-rh]

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.

[wdoekes]

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);

[wdoekes]

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;
         }
     }

[wdoekes]

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 🤷