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 > 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. > > Signed-off-by: Demi Marie Obenour > --- > host/rootfs/Makefile | 6 +- > lib/kcmdline-utils.mk | 6 ++ > tools/default.nix | 1 + > tools/meson.build | 1 + > tools/updates-dir-check/meson.build | 4 ++ > tools/updates-dir-check/updates-dir-check.c | 94 +++++++++++++++++++++++++++++ > 6 files changed, 110 insertions(+), 2 deletions(-) I still don't really understand why this needs to be a C program instead of find -H /path/to/dir -not -type f. None of the other checks seem very necessary? > diff --git a/host/rootfs/Makefile b/host/rootfs/Makefile > index 00d125774bb7b98736d0928c69cb307740cee034..15752286f5924291768f0655a12b90c702730520 100644 > --- a/host/rootfs/Makefile > +++ b/host/rootfs/Makefile > @@ -62,6 +62,9 @@ build/fifo: > build/empty: > mkdir -p $@ > > +build/etc: > + mkdir -p $@ > + > # s6-rc-compile's input is a directory, but that doesn't play nice > # with Make, because it won't know to update if some file in the > # directory is changed, or a file is created or removed in a > @@ -69,8 +72,7 @@ build/empty: > # including files that aren't intended to be part of the input, like > # temporary editor files or .license files. So for all these reasons, > # only explicitly listed files are made available to s6-rc-compile. > -build/etc/s6-rc: $(S6_RC_FILES) file-list.mk > - mkdir -p $$(dirname $@) > +build/etc/s6-rc: $(S6_RC_FILES) file-list.mk build/etc > rm -rf $@ > set -uo pipefail && dir=$$(mktemp -d) && \ > { tar -c $(S6_RC_FILES) | tar -C $$dir -x --strip-components 3; } && \ > diff --git a/lib/kcmdline-utils.mk b/lib/kcmdline-utils.mk > new file mode 100644 > index 0000000000000000000000000000000000000000..fa228552e583f15fc77a746985060ad5d04cdf2c > --- /dev/null > +++ b/lib/kcmdline-utils.mk > @@ -0,0 +1,6 @@ > +# SPDX-License-Identifier: EUPL-1.2+ > +# SPDX-FileCopyrightText: 2021-2024 Alyssa Ross > +READ_ROOTHASH = { \ > + set -eufo pipefail; \ > + read -r version < ../../version; \ > + LC_ALL=C expr "x$$version" : '^\(x0\|x[1-9][0-9]*\)\(\.\(0\|[1-9][0-9]*\)\)\{2\}$$' >/dev/null; } None of these changes seem to have anything to do with this patch. Did they end up in here by mistake? > diff --git a/tools/default.nix b/tools/default.nix > index ca165b5ec8ae1a63b75af4a34f33e320b262ba7b..e644f4e710e56f32de27ea10047cba3cffd0ecdf 100644 > --- a/tools/default.nix > +++ b/tools/default.nix > @@ -78,6 +78,7 @@ stdenv.mkDerivation (finalAttrs: { > ./start-vmm > ./subprojects > ./sd-notify-adapter > + ./updates-dir-check > ] ++ lib.optionals driverSupport [ > ./xdp-forwarder > ])); > diff --git a/tools/meson.build b/tools/meson.build > index 5d0ae81042fd3d77646594500f32cb1d48a6af0c..7da3bb451a5f1a797bc7e50a67c44dbd37ba60bf 100644 > --- a/tools/meson.build > +++ b/tools/meson.build > @@ -28,6 +28,7 @@ if get_option('host') > subdir('lsvm') > subdir('start-vmm') > subdir('sd-notify-adapter') > + subdir('updates-dir-check') > endif > > if get_option('app') > diff --git a/tools/updates-dir-check/meson.build b/tools/updates-dir-check/meson.build > new file mode 100644 > index 0000000000000000000000000000000000000000..e19691d0e35f8a051e897990f0376384b3625c1a > --- /dev/null > +++ b/tools/updates-dir-check/meson.build > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: EUPL-1.2+ > +# SPDX-FileCopyrightText: 2025 Demi Marie Obenour > + > +executable('updates-dir-check', 'updates-dir-check.c', install: true, c_args: ['-D_GNU_SOURCE=1', '-UNDEBUG', '-Wno-error=pedantic']) How are -Werror=pedantic and -DNDEBUG getting enabled in the first place? > diff --git a/tools/updates-dir-check/updates-dir-check.c b/tools/updates-dir-check/updates-dir-check.c > new file mode 100644 > index 0000000000000000000000000000000000000000..94c7d54bec38d6efbd5b8aca257f3ec4ad3fae35 > --- /dev/null > +++ b/tools/updates-dir-check/updates-dir-check.c > @@ -0,0 +1,94 @@ > +// SPDX-License-Identifier: EUPL-1.2+ > +// SPDX-FileCopyrightText: 2025 Demi Marie Obenour > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > + > +#include > + > +static void checkdir(int fd) > +{ > + DIR *d = fdopendir(fd); > + if (d == NULL) > + err(1, "fdopendir"); Usually we use EXIT_FAILURE. > + bool found_sha256sums = false; > + bool found_sha256sums_gpg = false; > + for (;;) { > + errno = 0; > + struct dirent *entry = readdir(d); > + if (entry == NULL) { > + if (errno) > + err(1, "readdir"); > + break; > + } > + assert(entry->d_reclen > offsetof(struct dirent, d_name)); > + size_t len = strnlen(entry->d_name, entry->d_reclen - offsetof(struct dirent, d_name)); > + assert(len < entry->d_reclen - offsetof(struct dirent, d_name)); > + assert(len > 0); We do not need to second guess the kernel/libc. > + if (entry->d_name[0] == '.') > + if (len == 1 || (len == 2 && entry->d_name[1] == '.')) > + continue; > + if (strcmp(entry->d_name, "SHA256SUMS") == 0) { > + found_sha256sums = true; > + continue; > + } > + if (strcmp(entry->d_name, "SHA256SUMS.gpg") == 0) { > + found_sha256sums_gpg = true; > + continue; > + } > + unsigned char c = (unsigned char)entry->d_name[0]; > + if (!((c >= 'A' && c <= 'Z') || > + (c >= 'a' && c <= 'z'))) > + errx(1, "Filename must begin with an ASCII letter"); > + 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(1, "Forbidden subsequent character in filename: '%c'", (int)c); > + else > + errx(1, "Forbidden subsequent character in filename: byte %d", (int)c); > + } > + } Why do we care? Surely we don't expect systemd-sysupdate to put filenames unescaped into a shell or something. > + if (entry->d_name[len - 1] == '.') > + errx(1, "Filename must not end with a '.'"); > + if (entry->d_type != DT_REG) > + errx(1, "Entry contains non-regular file %s", entry->d_name); > + } > + if (!found_sha256sums) > + errx(1, "SHA256SUMS not found"); > + if (!found_sha256sums_gpg) > + errx(1, "SHA256SUMS.gpg not found"); > + closedir(d); > +} > + > +int main(int argc, char **argv) > +{ > + for (int i = 1; i < argc; ++i) { > + // Avoid symlink attacks. > + struct open_how how = { > + .flags = O_DIRECTORY|O_RDONLY|O_CLOEXEC|O_NOFOLLOW, > + .resolve = RESOLVE_NO_SYMLINKS|RESOLVE_NO_MAGICLINKS, > + }; For opening files given on the command line, wouldn't we want to use the mount's symlink behaviour? The VM presumably can't replace the root directory shared with it with a symlink. > + int fd = (int)syscall((long)SYS_openat2, (long)AT_FDCWD, (long)argv[i], > + (long)&how, (long)sizeof(how)); > + if (fd < 0) > + err(1, "open(%s)", argv[i]); > + checkdir(fd); > + } > + return 0; > +}