| |
@@ -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).
|
| |
+ */
|
| |
nfrayer
commented 3 months ago
(CVE-2024-1048)
Resolves: #2256678
Signed-off-by: Nicolas Frayer nfrayer@redhat.com