On 11/13/25 08:21, 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 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. It also ensures that both SHA256SUMS and SHA256SUMS.gpg are present.
This needs updated, because it doesn't any more.
Will fix in v3.
Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com> --- tools/default.nix | 1 + tools/meson.build | 4 +++ tools/updates-dir-check.c | 78 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+)
diff --git a/tools/default.nix b/tools/default.nix index 18d4dd6353edf5c128213fa5c1716717f90edf07..a1b352e660f02a53e97ed1e3420a4de90bb24ce3 100644 --- a/tools/default.nix +++ b/tools/default.nix @@ -77,6 +77,7 @@ stdenv.mkDerivation (finalAttrs: { ./sd-notify-adapter.c ./start-vmm ./subprojects + ./updates-dir-check.c ] ++ lib.optionals driverSupport [ ./xdp-forwarder ])); diff --git a/tools/meson.build b/tools/meson.build index d465e99c2ef597fdf7e818748d08db3d96f4ec6b..a7c21684dd64ad9e87c85bcdf31792e81b55faa4 100644 --- a/tools/meson.build +++ b/tools/meson.build @@ -29,6 +29,10 @@ if get_option('host') c_args : '-D_GNU_SOURCE', install: true)
+ executable('updates-dir-check', 'updates-dir-check.c', + c_args : '-D_GNU_SOURCE', + install: true) + subdir('lsvm') subdir('start-vmm') endif diff --git a/tools/updates-dir-check.c b/tools/updates-dir-check.c new file mode 100644 index 0000000000000000000000000000000000000000..15b58204476299d8e7fe7ffdbac5245e04332e7d --- /dev/null +++ b/tools/updates-dir-check.c @@ -0,0 +1,78 @@ +// 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 <linux/openat2.h> +#include <sys/syscall.h> +#include <unistd.h> + +#include <err.h>
Kernel headers should always follow libc headers. Sometimes they rely on defines from libc (especially with musl). Although openat2 doesn't actually seem to be used any more, so these headers just need a prune.
Will fix.
+static void checkdir(int fd) +{ + 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"); + for (;;) { + errno = 0; + struct dirent *entry = readdir(d); + if (entry == NULL) { + if (errno) + err(EXIT_FAILURE, "readdir"); + break; + } + assert(entry->d_reclen > offsetof(struct dirent, d_name));
Would be a POSIX violation for d_name not to be valid I think.
Indeed it would be, but I preferred to check that explicitly.
+ size_t len = strnlen(entry->d_name, entry->d_reclen - offsetof(struct dirent, d_name));
POSIX also guarantees a terminating null byte, so strlen() would be fine here.
I preferred to double-check libc, but if you prefer to get rid of those assertions I'm okay with that.
+ if (entry->d_name[0] == '.') + if (len == 1 || (len == 2 && entry->d_name[1] == '.')) + continue; + unsigned char c = (unsigned char)entry->d_name[0]; + if (!((c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z'))) + errx(EXIT_FAILURE, "Filename must begin with an ASCII letter");
Would the comparison not be valid without the cast?
It would be in this case, but the subsequent cast to int in the error path assumes that the cast was done. Signed characters are much trickier and casting to unsigned makes the code easier to reason about.
+ for (size_t i = 1; i < len; ++i) { + c = (unsigned char)entry->d_name[i]; + if (!((c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') || + (c == '_') || + (c == '-') || + (c == '.'))) { + if (c >= 0x20 && c <= 0x7E) + errx(EXIT_FAILURE, "Forbidden subsequent character in filename: '%c'", (int)c); + else + errx(EXIT_FAILURE, "Forbidden subsequent character in filename: byte %d", (int)c); + } + } + if (entry->d_name[len - 1] == '.') + errx(EXIT_FAILURE, "Filename must not end with a '.'");
I'm still not sold on this validation, but as long as it doesn't cause problems I guess it's fine.
+ if (entry->d_type != DT_REG) + errx(EXIT_FAILURE, "Entry contains non-regular file %s", entry->d_name); + } + closedir(d); +} + +int main(int argc, char **argv) +{ + for (int i = 1; i < argc; ++i) {
+ int fd = open(argv[i], O_DIRECTORY|O_RDONLY|O_CLOEXEC|O_NOFOLLOW);
Wasn't O_NOFOLLOW going to be removed here?
It should never be called on a symlink, so if it does that is a bug in the caller. I can remove it if you prefer, though.
+ if (fd < 0) + err(EXIT_FAILURE, "open(%s)", argv[i]); + checkdir(fd); + } + return 0; +}
-- 2.51.2
-- Sincerely, Demi Marie Obenour (she/her/hers)