Re: [PATCH 3/7] tools: Add directory checker for updates

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 8303 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
> 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 <demiobenour@gmail.com>
> ---
>  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 <hi@alyssa.is>
> +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 <demiobenour@gmail.com>
> +
> +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 <demiobenour@gmail.com>
> +#include <assert.h>
> +#include <errno.h>
> +#include <stddef.h>
> +#include <string.h>
> +
> +#include <sysexits.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>
> +
> +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;
> +}

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