On 11/19/25 09:45, Alyssa Ross wrote: > Demi Marie Obenour 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 >> --- >> 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 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 >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> + >> +[[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)