grub2/0250-grub-set-bootflag-More-complete-fix-for-CVE-2024-104.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Solar Designer <solar@openwall.com>
Date: Tue, 6 Feb 2024 21:56:21 +0100
Subject: [PATCH] grub-set-bootflag: More complete fix for CVE-2024-1048

Switch to per-user fixed temporary filenames along with a weird locking
mechanism, which is explained in source code comments.  This is a more
complete fix than the previous commit (temporary files can't accumulate).
Unfortunately, it introduces new risks (by working on a temporary file
shared between the user's invocations), which are _hopefully_ avoided by
the patch's elaborate logic.  I actually got it wrong at first, which
suggests that this logic is hard to reason about, and more errors or
omissions are possible.  It also relies on the kernel's primitives' exact
semantics to a greater extent (nothing out of the ordinary, though).

Remaining issues that I think cannot reasonably be fixed without a
redesign (e.g., having per-flag files with nothing else in them) and
without introducing new issues:

A. A user can still revert a concurrent user's attempt of setting the
other flag - or of making other changes to grubenv by means other than
this program.

B. One leftover temporary file per user is still possible.

Signed-off-by: Solar Designer <solar@openwall.com>
---
 util/grub-set-bootflag.c | 95 ++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 79 insertions(+), 16 deletions(-)

diff --git a/util/grub-set-bootflag.c b/util/grub-set-bootflag.c
index 5bbbef80439..514c4f9091a 100644
--- a/util/grub-set-bootflag.c
+++ b/util/grub-set-bootflag.c
@@ -33,6 +33,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <sys/file.h>
 #include <sys/stat.h>
 #include <sys/resource.h>
 
@@ -60,15 +61,12 @@ int main(int argc, char *argv[])
 {
   /* NOTE buf must be at least the longest bootflag length + 4 bytes */
   char env[GRUBENV_SIZE + 1 + 2], buf[64], *s;
-  /* +1 for 0 termination, +6 for "XXXXXX" in tmp filename */
-  char env_filename[PATH_MAX + 1], tmp_filename[PATH_MAX + 6 + 1];
+  /* +1 for 0 termination, +11 for ".%u" in tmp filename */
+  char env_filename[PATH_MAX + 1], tmp_filename[PATH_MAX + 11 + 1];
   const char *bootflag;
   int i, fd, len, ret;
   FILE *f;
-  struct rlimit rlim;
 
-  if (getrlimit(RLIMIT_FSIZE, &rlim) || rlim.rlim_cur < GRUBENV_SIZE || rlim.rlim_max < GRUBENV_SIZE)
-    return 1;
   umask(077);
 
   if (argc != 2)
@@ -105,7 +103,7 @@ int main(int argc, char *argv[])
    */
   if (setegid(0))
     {
-      perror ("Error setegid(0) failed");
+      perror ("setegid(0) failed");
       return 1;
     }
 
@@ -176,19 +174,82 @@ int main(int argc, char *argv[])
     return 0; /* nothing to do */
   memcpy(s, buf, len + 3);
 
+  struct rlimit rlim;
+  if (getrlimit(RLIMIT_FSIZE, &rlim) || rlim.rlim_cur < GRUBENV_SIZE || rlim.rlim_max < GRUBENV_SIZE)
+    {
+      fprintf (stderr, "Resource limits undetermined or too low\n");
+      return 1;
+    }
+
+  /*
+   * Here we work under the premise that we shouldn't write into the target
+   * file directly because we might not be able to have all of our changes
+   * written completely and atomically.  That was CVE-2019-14865, known to
+   * have been triggerable via RLIMIT_FSIZE.  While we've dealt with that
+   * specific attack via the check above, there may be other possibilities.
+   */
 
   /*
    * Create a tempfile for writing the new env.  Use the canonicalized filename
    * for the template so that the tmpfile is in the same dir / on same fs.
+   *
+   * We now use per-user fixed temporary filenames, so that a user cannot cause
+   * multiple files to accumulate.
+   *
+   * We don't use O_EXCL so that a stale temporary file doesn't prevent further
+   * usage of the program by the user.
    */
-  snprintf(tmp_filename, sizeof(tmp_filename), "%sXXXXXX", env_filename);
-  fd = mkstemp(tmp_filename);
+  snprintf(tmp_filename, sizeof(tmp_filename), "%s.%u", env_filename, getuid());
+  fd = open(tmp_filename, O_CREAT | O_WRONLY, 0600);
   if (fd == -1)
     {
       perror ("Creating tmpfile failed");
       return 1;
     }
 
+  /*
+   * The lock prevents the same user from reaching further steps ending in
+   * rename() concurrently, in which case the temporary file only partially
+   * written by one invocation could be renamed to the target file by another.
+   *
+   * The lock also guards the slow fsync() from concurrent calls.  After the
+   * first time that and the rename() complete, further invocations for the
+   * same flag become no-ops.
+   *
+   * We lock the temporary file rather than the target file because locking the
+   * latter would allow any user having SIGSTOP'ed their process to make all
+   * other users' invocations fail (or lock up if we'd use blocking mode).
+   *
+   * We use non-blocking mode (LOCK_NB) because the lock having been taken by
+   * another process implies that the other process would normally have already
+   * renamed the file to target by the time it releases the lock (and we could
+   * acquire it), so we'd be working directly on the target if we proceeded,
+   * which is undesirable, and we'd kind of fail on the already-done rename.
+   */
+  if (flock(fd, LOCK_EX | LOCK_NB))
+    {
+      perror ("Locking tmpfile failed");
+      return 1;
+    }
+
+  /*
+   * Deal with the potential that another invocation proceeded all the way to
+   * rename() and process exit while we were between open() and flock().
+   */
+  {
+    struct stat st1, st2;
+    if (fstat(fd, &st1) || stat(tmp_filename, &st2))
+      {
+        perror ("stat of tmpfile failed");
+        return 1;
+      }
+    if (st1.st_dev != st2.st_dev || st1.st_ino != st2.st_ino)
+      {
+        fprintf (stderr, "Another invocation won race\n");
+        return 1;
+      }
+  }
+
   f = fdopen (fd, "w");
   if (!f)
     {
@@ -213,6 +274,14 @@ int main(int argc, char *argv[])
       return 1;     
     }
 
+  ret = ftruncate (fileno (f), GRUBENV_SIZE);
+  if (ret)
+    {
+      perror ("Error truncating tmpfile");
+      unlink(tmp_filename);
+      return 1;
+    }
+
   ret = fsync (fileno (f));
   if (ret)
     {
@@ -221,15 +290,9 @@ int main(int argc, char *argv[])
       return 1;
     }
 
-  ret = fclose (f);
-  if (ret)
-    {
-      perror ("Error closing tmpfile");
-      unlink(tmp_filename);
-      return 1;
-    }
-
   /*
+   * We must not close the file before rename() as that would remove the lock.
+   *
    * And finally rename the tmpfile with the new env over the old env, the
    * linux kernel guarantees that this is atomic (from a syscall pov).
    */