From 9ca4c3fe1c7dbd62e8ad6a23cb1b1fda695fdb44 Mon Sep 17 00:00:00 2001
From: Solar Designer <solar@openwall.com>
Date: Tue, 6 Feb 2024 21:39:41 +0100
Subject: [PATCH 1/3] 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 a85f11fceacb..5b932f76b6f4 100644
--- a/util/grub-set-bootflag.c
+++ b/util/grub-set-bootflag.c
@@ -32,6 +32,8 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <sys/stat.h>
+#include <sys/resource.h>
 
 #define GRUBENV "/" GRUB_BOOT_DIR_NAME "/" GRUB_DIR_NAME "/" GRUB_ENVBLK_DEFCFG
 #define GRUBENV_SIZE 1024
@@ -54,12 +56,17 @@ static void usage(void)
 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)
     {
@@ -81,20 +88,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)
-    {
-      perror ("Error setuid(0) failed");
-      return 1;
-    }
-
-  ret = setgid(0);
-  if (ret)
+  if (setegid(0))
     {
-      perror ("Error setgid(0) failed");
+      perror ("Error setegid(0) failed");
       return 1;
     }
 
@@ -123,6 +121,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)))
     {
@@ -158,6 +159,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);
 
 
-- 
2.43.0