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

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

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

On 11/19/25 09:45, 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
> 
> Just one "containing" is fine. :P

Nice catch :)

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

Do you want me to send another version, or would you rather fix this
up on commit?  All your changes look good.

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

I totally forgot that %hhu exists!

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

Good catch!

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

It isn't necessarily obvious that any file with this prefix is
a signature.

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

In this case, only the directory got changed, so I only need to flush
the directory.

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

Do you mean opendir()?  That works, thanks!

>> +		checkdir(fd, check_sig);
>> +	}
>> +	return 0;
>> +}
>>
>> -- 
>> 2.52.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 --]