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

[Demi Marie Obenour <demiobenour@gmail.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 9213 bytes --]

On 10/29/25 08:01, 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.
>>
>> 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?

I trust this code more than I trust (especially) the Busybox
implementation of find.

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

Correct.

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

I believe Meson sets -DNDEBUG in some cases.

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

Will fix in v2.

>> +	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);

Will fix in v2.

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

Prevent escape sequence injection into terminals and logs is the
main reason.  Qubes OS has similar checks in some places, though they
are off by default for file copying.

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

Right now, the directory shared is actually the parent of the directory
with the updates.  This is because of a limitation in the updated
start-vm script.  As you point out later, that's not the best approach.
Much better to bind-mount only the directories needed.

>> +		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;
>> +}


-- 
Sincerely,
Demi Marie Obenour (she/her/hers)

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 7253 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]