rpms/grub2
6
0
Fork 0
Code Issues Pull Requests Releases Activity

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

Browse Source 
(CVE-2024-1048)

Resolves: #RHEL-20747
Signed-off-by: Nicolas Frayer <nfrayer@redhat.com>
This commit is contained in:
Nicolas Frayer 2024-02-08 10:54:24 +01:00
parent dc354eb1d9
commit 624933c2c9
5 changed files with 378 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

146
0332-grub-set-bootflag-Conservative-partial-fix-for-CVE-2.patch Normal file
View File

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

187
0333-grub-set-bootflag-More-complete-fix-for-CVE-2024-104.patch Normal file
View File

@ -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).
*/

36
0334-grub-set-bootflag-Exit-calmly-when-not-running-as-ro.patch Normal file
View File

@ -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.
*/

3
grub.patches
View File

@ -329,3 +329,6 @@ Patch0328: 0328-grub2-mkconfig-Pass-all-boot-params-when-used-by-ana.patch
Patch0329: 0329-kern-ieee1275-init-ppc64-Restrict-high-memory-in-pre.patch
Patch0330: 0330-normal-Remove-grub_env_set-prefix-in-grub_try_normal.patch
Patch0331: 0331-search-command-add-flag-to-only-search-root-dev.patch
Patch0332: 0332-grub-set-bootflag-Conservative-partial-fix-for-CVE-2.patch
Patch0333: 0333-grub-set-bootflag-More-complete-fix-for-CVE-2024-104.patch
Patch0334: 0334-grub-set-bootflag-Exit-calmly-when-not-running-as-ro.patch

7
grub2.spec
View File

@ -16,7 +16,7 @@
Name: grub2
Epoch: 1
Version: 2.06
Release: 74%{?dist}
Release: 75%{?dist}
Summary: Bootloader with support for Linux, Multiboot and more
License: GPLv3+
URL: http://www.gnu.org/software/grub/
@ -533,6 +533,11 @@ mv ${EFI_HOME}/grub.cfg.stb ${EFI_HOME}/grub.cfg
%endif
%changelog
* Wed Feb 7 2024 Nicolas Frayer <nfrayer@redhat.com> - 2.06-75
- grub-set-bootflag: Fix for CVE-2024-1048
- (CVE-2024-1048)
- Resolves: #RHEL-20747
* Mon Feb 5 2024 Nicolas Frayer <nfrayer@redhat.com> - 2.06-74
- Don't run 20-grub.install for UKIs
- Resolves: #RHEL-21368