From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Solar Designer 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 --- 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 3b4c25ca2ac..5bbbef80439 100644 --- a/util/grub-set-bootflag.c +++ b/util/grub-set-bootflag.c @@ -33,6 +33,8 @@ #include #include #include +#include +#include #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);