grub-set-bootflag: Fix for CVE-2024-1048

(CVE-2024-1048)

Resolves: #2256678
Signed-off-by: Nicolas Frayer <nfrayer@redhat.com>

0351-grub-set-bootflag-Conservative-partial-fix-for-CVE-2.patch

@@ -0,0 +1,146 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Solar Designer <solar@openwall.com>
+Date: Tue, 6 Feb 2024 21:39:41 +0100
+Subject: [PATCH] grub-set-bootflag: Conservative partial fix for CVE-2024-1048
+
+Following up on CVE-2019-14865 and taking a fresh look at
+grub2-set-bootflag now (through my work at CIQ on Rocky Linux), I saw some
+other ways in which users could still abuse this little program:
+
+1. After CVE-2019-14865 fix, grub2-set-bootflag no longer rewrites the
+grubenv file in-place, but writes into a temporary file and renames it
+over the original, checking for error returns from each call first.
+This prevents the original file truncation vulnerability, but it can
+leave the temporary file around if the program is killed before it can
+rename or remove the file.  There are still many ways to get the program
+killed, such as through RLIMIT_FSIZE triggering SIGXFSZ (tested,
+reliable) or by careful timing (tricky) of signals sent by process group
+leader, pty, pre-scheduled timers, SIGXCPU (probably not an exhaustive
+list).  Invoking the program multiple times fills up /boot (or if /boot
+is not separate, then it can fill up the root filesystem).  Since the
+files are tiny, the filesystem is likely to run out of free inodes
+before it'd run out of blocks, but the effect is similar - can't create
+new files after this point (but still can add data to existing files,
+such as logs).
+
+2. After CVE-2019-14865 fix, grub2-set-bootflag naively tries to protect
+itself from signals by becoming full root.  (This does protect it from
+signals sent by the user directly to the PID, but e.g. "kill -9 -1" by
+the user still works.)  A side effect of such "protection" is that it's
+possible to invoke more concurrent instances of grub2-set-bootflag than
+the user's RLIMIT_NPROC would normally permit (as specified e.g. in
+/etc/security/limits.conf, or say in Apache httpd's RLimitNPROC if
+grub2-set-bootflag would be abused by a website script), thereby
+exhausting system resources (e.g., bypassing RAM usage limit if
+RLIMIT_AS was also set).
+
+3. umask is inherited.  Again, due to how the CVE-2019-14865 fix creates
+a new file, and due to how mkstemp() works, this affects grubenv's new
+file permissions.  Luckily, mkstemp() forces them to be no more relaxed
+than 0600, but the user ends up being able to set them e.g. to 0.
+Luckily, at least in my testing GRUB still works fine even when the file
+has such (lack of) permissions.
+
+This commit deals with the abuses above as follows:
+
+1. RLIMIT_FSIZE is pre-checked, so this specific way to get the process
+killed should no longer work.  However, this isn't a complete fix
+because there are other ways to get the process killed after it has
+created the temporary file.
+
+The commit also fixes bug 1975892 ("RFE: grub2-set-bootflag should not
+write the grubenv when the flag being written is already set") and
+similar for "menu_show_once", which further reduces the abuse potential.
+
+2. RLIMIT_NPROC bypass should be avoided by not becoming full root (aka
+dropping the partial "kill protection").
+
+3. A safe umask is set.
+
+This is a partial fix (temporary files can still accumulate, but this is
+harder to trigger).
+
+While at it, this commit also fixes potential 1- or 2-byte over-read of
+env[] if its content is malformed - this was not a security issue since the
+grubenv file is trusted input, and the fix is just for robustness.
+
+Signed-off-by: Solar Designer <solar@openwall.com>
+---
+ util/grub-set-bootflag.c | 29 ++++++++++++++++-------------
+ 1 file changed, 16 insertions(+), 13 deletions(-)
+
+diff --git a/util/grub-set-bootflag.c b/util/grub-set-bootflag.c
+index 3b4c25ca2ac6..5bbbef804391 100644
+--- a/util/grub-set-bootflag.c
++++ b/util/grub-set-bootflag.c
+@@ -33,6 +33,8 @@
+ #include <stdlib.h>
+ #include <string.h>
+ #include <unistd.h>
++#include <sys/stat.h>
++#include <sys/resource.h>
+ 
+ #include "progname.h"
+ 
+@@ -57,12 +59,17 @@ static void usage(FILE *out)
+ int main(int argc, char *argv[])
+ {
+   /* NOTE buf must be at least the longest bootflag length + 4 bytes */
+-  char env[GRUBENV_SIZE + 1], buf[64], *s;
++  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];
+   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)
+     {
+@@ -94,20 +101,11 @@ int main(int argc, char *argv[])
+   len = strlen (bootflag);
+ 
+   /*
+-   * Really become root. setuid avoids an user killing us, possibly leaking
+-   * the tmpfile. setgid avoids the new grubenv's gid being that of the user.
++   * setegid avoids the new grubenv's gid being that of the user.
+    */
+-  ret = setuid(0);
+-  if (ret)
++  if (setegid(0))
+     {
+-      perror ("Error setuid(0) failed");
+-      return 1;
+-    }
+-
+-  ret = setgid(0);
+-  if (ret)
+-    {
+-      perror ("Error setgid(0) failed");
++      perror ("Error setegid(0) failed");
+       return 1;
+     }
+ 
+@@ -136,6 +134,9 @@ int main(int argc, char *argv[])
+ 
+   /* 0 terminate env */
+   env[GRUBENV_SIZE] = 0;
++  /* not a valid flag value */
++  env[GRUBENV_SIZE + 1] = 0;
++  env[GRUBENV_SIZE + 2] = 0;
+ 
+   if (strncmp (env, GRUB_ENVBLK_SIGNATURE, strlen (GRUB_ENVBLK_SIGNATURE)))
+     {
+@@ -171,6 +172,8 @@ int main(int argc, char *argv[])
+ 
+   /* The grubenv is not 0 terminated, so memcpy the name + '=' , '1', '\n' */
+   snprintf(buf, sizeof(buf), "%s=1\n", bootflag);
++  if (!memcmp(s, buf, len + 3))
++    return 0; /* nothing to do */
+   memcpy(s, buf, len + 3);
+ 
+ 

0352-grub-set-bootflag-More-complete-fix-for-CVE-2024-104.patch

@@ -0,0 +1,187 @@
+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 5bbbef804391..514c4f9091ac 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).
+    */

0353-grub-set-bootflag-Exit-calmly-when-not-running-as-ro.patch

@@ -0,0 +1,36 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Solar Designer <solar@openwall.com>
+Date: Tue, 6 Feb 2024 22:05:45 +0100
+Subject: [PATCH] grub-set-bootflag: Exit calmly when not running as root
+
+Exit calmly when not installed SUID root and invoked by non-root.  This
+allows installing user/grub-boot-success.service unconditionally while
+supporting non-SUID installation of the program for some limited usage.
+
+Signed-off-by: Solar Designer <solar@openwall.com>
+---
+ util/grub-set-bootflag.c | 11 +++++++++++
+ 1 file changed, 11 insertions(+)
+
+diff --git a/util/grub-set-bootflag.c b/util/grub-set-bootflag.c
+index 514c4f9091ac..31a868aeca8a 100644
+--- a/util/grub-set-bootflag.c
++++ b/util/grub-set-bootflag.c
+@@ -98,6 +98,17 @@ int main(int argc, char *argv[])
+   bootflag = bootflags[i];
+   len = strlen (bootflag);
+ 
++  /*
++   * Exit calmly when not installed SUID root and invoked by non-root.  This
++   * allows installing user/grub-boot-success.service unconditionally while
++   * supporting non-SUID installation of the program for some limited usage.
++   */
++  if (geteuid())
++    {
++      printf ("grub-set-bootflag not running as root, no action taken\n");
++      return 0;
++    }
++
+   /*
+    * setegid avoids the new grubenv's gid being that of the user.
+    */

grub.patches

@@ -347,4 +347,7 @@ Patch0346: 0346-chainloader-remove-device-path-debug-message.patch
 Patch0347: 0347-normal-Remove-grub_env_set-prefix-in-grub_try_normal.patch
 Patch0348: 0348-add-flag-to-only-search-root-dev.patch
 Patch0349: 0349-Ignore-warnings-for-incompatible-types.patch
-Patch0350: 0350-fs-xfs-Fix-XFS-directory-extent-parsing.patch
+Patch0350: 0350-fs-xfs-Fix-XFS-directory-extent-parsing.patch
+Patch0351: 0351-grub-set-bootflag-Conservative-partial-fix-for-CVE-2.patch
+Patch0352: 0352-grub-set-bootflag-More-complete-fix-for-CVE-2024-104.patch
+Patch0353: 0353-grub-set-bootflag-Exit-calmly-when-not-running-as-ro.patch

grub2.spec

@@ -17,7 +17,7 @@
 Name:		grub2
 Epoch:		1
 Version:	2.06
-Release:	118%{?dist}
+Release:	119%{?dist}
 Summary:	Bootloader with support for Linux, Multiboot and more
 License:	GPL-3.0-or-later
 URL:		http://www.gnu.org/software/grub/
@@ -554,6 +554,11 @@ mv ${EFI_HOME}/grub.cfg.stb ${EFI_HOME}/grub.cfg
 %endif
 
 %changelog
+* Wed Feb 7 2024 Nicolas Frayer <nfrayer@redhat.com> - 2.06-119
+- grub-set-bootflag: Fix for CVE-2024-1048
+- (CVE-2024-1048)
+- Resolves: #2256678
+
 * Tue Jan 23 2024 Leo Sandoval <lsandova@redhat.com> - 2.06-118
 - xfs: include the 'directory extent parsing patch', otherwise
 XFS-formatted partitions do not boot. This change effectively