Re: [PATCH v3 03/14] tools: Add directory checker for updates

Alyssa Ross
2025-11-19T14:55:28Z

[-- Attachment #1: Type: text/plain, Size: 7415 bytes --]

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

> 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>

> 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?

> +}
> +
> +[[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.)

> +{
> + 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.

> + 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 (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.

> + checkdir(fd, check_sig);
> + }
> + return 0;
> +}
>
> --
> 2.52.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]