Demi Marie Obenour <demiobenour@gmail.com> writes:

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

This doesn't really make sense to me.  All of this is quite trivial find
behaviour — not the sort of thing that's unlikely to have been widely
tested.  No objection to GNU find though if it helps.

>> How are -Werror=pedantic and -DNDEBUG getting enabled in the first place?
>
> I believe Meson sets -DNDEBUG in some cases.

Yes, if the user explicitly asks for it.

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

Doing this in a tool that's only used by sysupdate is a very ad-hoc way
to protect against that.  I think if we want to protect against that
sort of thing it should be done in one place, probably in virtiofsd.