On 11/19/25 09:45, Alyssa Ross wrote:
Demi Marie Obenour <demiobenour@gmail.com> writes:
Spectrum OS's host has no network access. Updates must be downloaded by VMs. The downloads are placed into a bind-mounted directory. The VM can write whatever it wants into that directory. This includes symlinks that subsequent code might open, which would create a path traversal vulnerability. It also includes paths with names containing containing
Just one "containing" is fine. :P
Nice catch :)
terminal escape sequences, newlines, or other nastiness. Furthermore, the directory should not have any subdirectories either.
Add a simple C program that checks for such ugliness and indicates (via its exit code) if the VM misbehaved. systemd-sysupdate can leave behind temporary files with names starting with '.', so delete them instead of failing. Linux can lose cache coherency if there is an I/O error, so call syncfs() on the directory before checking anything. For the same reason, fsync() the directory if any hidden files were deleted.
The directory checker also serves another critical function: it checks if the VM actually downloaded anything. Otherwise, network problems could cause updates to silently do nothing. Specifically, it checks that the VM provided a file starting with the prefix "SHA256SUMS.". These will be the last ones the in-VM updater downloads. An additional mode is provided to clean out all such files. This will be used to ensure that before the in-VM updater runs, no such files are present. Hence, if the VM didn't actually download anything, the user will get a clear error instead of a false success message or a confusing error.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- Changes since v2:
- Purge leftover temporary files rather than returning an error.
- Split into two modes: one that deletes signature files, and one that checks that at least one signature file exists. This allows checking that the VM actually sent something. --- tools/default.nix | 1 + tools/meson.build | 4 ++ tools/updates-dir-check.c | 133 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+)
Looking good. Left some style comments, but
Reviewed-by: Alyssa Ross <hi@alyssa.is>
Do you want me to send another version, or would you rather fix this up on commit? All your changes look good.
diff --git a/tools/updates-dir-check.c b/tools/updates-dir-check.c new file mode 100644 index 0000000000000000000000000000000000000000..07eb059f2718e1ad8ab087fe6509c1437ea3e96c --- /dev/null +++ b/tools/updates-dir-check.c @@ -0,0 +1,133 @@ +// SPDX-License-Identifier: EUPL-1.2+ +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour <demiobenour@gmail.com> +#include <assert.h> +#include <errno.h> +#include <stddef.h> +#include <stdlib.h> +#include <string.h> + +#include <fcntl.h> +#include <sys/types.h> +#include <dirent.h> +#include <unistd.h> + +#include <err.h> + +[[noreturn]] static void bad_char(char c, char *msg_component) +{ + if (c >= 0x20 && c <= 0x7E) + errx(EXIT_FAILURE, "Forbidden %s character in filename: '%c'", + msg_component, (int)c); + errx(EXIT_FAILURE, + "Forbidden %s character in filename: byte %d", + msg_component, (int)(unsigned char)c);
Why not %hhu, so you don't need two layers of casts?
I totally forgot that %hhu exists!
+} + +[[noreturn]] static void usage(void) +{ + errx(EXIT_FAILURE, "Usage: updates-dir-check [cleanup|check] DIRECTORIES..."); +} + +static void checkdir(int fd, bool check_sig)
[[gnu::fd_arg_read (1)]]
(I'm bad at remembering this too so you'll see other code missing it, but it's good to add.)
Good catch!
+{ + bool found_sig = false; + DIR *d = fdopendir(fd); + if (d == NULL) + err(EXIT_FAILURE, "fdopendir"); + // If there is an I/O error while there are dirty pages outstanding, + // the dirty pages are silently discarded. This means that the contents + // of the filesystem can change behind userspace's back. Flush all + // dirty pages in the filesystem with the directory to prevent this. + if (syncfs(fd) != 0) + err(EXIT_FAILURE, "syncfs"); + bool changed = false; + for (;;) { + errno = 0; + struct dirent *entry = readdir(d); + if (entry == NULL) { + if (errno) + err(EXIT_FAILURE, "readdir"); + break; + } + const char *ptr = entry->d_name; + if (ptr[0] == '.') { + if (ptr[1] == '\0') + continue; + if (ptr[1] == '.' && ptr[2] == '\0') + continue; + // systemd-sysupdate uses these for temporary files. + // It normally cleans them up itself, but if there is an error + // it does not always clean them up. I'm not sure if it is + // guaranteed to clean up temporary files from a past run, so + // delete them instead of returning an error. + if (unlinkat(fd, ptr, 0)) + err(EXIT_FAILURE, "Failed to unlink temporary file"); + changed = true; + continue; + } + char c = ptr[0]; + if (!((c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z'))) + bad_char(c, "initial"); + while ((c = *++ptr)) { + if (!((c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') || + (c == '_') || + (c == '-') || + (c == '.'))) + bad_char(c, "subsequent"); + } + // Empty filenames are rejected as having a bad initial character, + // and POSIX forbids them from being returned anyway. Therefore, + // this cannot be out of bounds. + if (ptr[-1] == '.') + errx(EXIT_FAILURE, "Filename %s ends with a '.'", entry->d_name); + if (entry->d_type == DT_UNKNOWN) + errx(EXIT_FAILURE, "Filesystem didn't report type of file %s", entry->d_name); + if (entry->d_type != DT_REG) + errx(EXIT_FAILURE, "Entry contains non-regular file %s", entry->d_name); + if (strncmp(entry->d_name, "SHA256SUMS.", sizeof("SHA256SUMS.") - 1) == 0) { + // Found a signature file!
This comment seems a bit redundant.
It isn't necessarily obvious that any file with this prefix is a signature.
+ if (check_sig) + found_sig = true; + else { + if (unlinkat(fd, entry->d_name, 0)) + err(EXIT_FAILURE, "Unlinking old signature file"); + changed = true; + } + } + } + // fsync() the directory if it was changed, to avoid the above + // cache-incoherency problem.
Above where?
// If there is an I/O error while there are dirty pages outstanding, // the dirty pages are silently discarded. This means that the contents // of the filesystem can change behind userspace's back. Flush all // dirty pages in the filesystem with the directory to prevent this. In this case, only the directory got changed, so I only need to flush the directory.
+ if (changed && fsync(fd)) + errx(EXIT_FAILURE, "fsync"); + if (check_sig && !found_sig) { + warnx("sys.appvm-systemd-sysupdate didn't send a signature file."); + warnx("There was probably a problem downloading the update."); + errx(EXIT_FAILURE, "Check its logs for more information."); + } + closedir(d); +} + +int main(int argc, char **argv) +{ + if (argc != 3) + usage(); + + bool check_sig; + if (strcmp(argv[1], "cleanup") == 0) + check_sig = false; + else if (strcmp(argv[1], "check") == 0) + check_sig = true; + else + usage(); + + for (int i = 2; i < argc; ++i) { + int fd = open(argv[i], O_DIRECTORY|O_RDONLY|O_CLOEXEC); + if (fd < 0) + err(EXIT_FAILURE, "open(%s)", argv[i]);
Maybe we could just fdopen(argv[1]) inside checkdir()? We don't need any special flags AFAICT.
Do you mean opendir()? That works, thanks!
+ checkdir(fd, check_sig); + } + return 0; +}
-- 2.52.0
-- Sincerely, Demi Marie Obenour (she/her/hers)