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

[Alyssa Ross <hi@alyssa.is>]

[-- Attachment #1: Type: text/plain, Size: 5517 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.

This needs updated, because it doesn't any more.

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

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

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

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

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

> +		if (fd < 0)
> +			err(EXIT_FAILURE, "open(%s)", argv[i]);
> +		checkdir(fd);
> +	}
> +	return 0;
> +}
>
> -- 
> 2.51.2

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