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

Demi Marie Obenour
2025-11-04T22:56:54Z

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

On 11/4/25 10:27, Alyssa Ross wrote:
> Demi Marie Obenour <demiobenour@gmail.com> writes:
>
>> On 11/2/25 07:18, Alyssa Ross wrote:
>>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>>>
>>>> On 11/1/25 08:17, Alyssa Ross wrote:
>>>>> 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.
>>>>
>>>> I see: find with a -exec false to return an error if anything matching
>>>> is found?
>>>>
>>>> I'm way more familiar with C than with find, which is why I missed this.
>>>
>>> Hmm, thinking about it some more I suppose there's a problem with find:
>>> there's no way to get it to exit as soon as it finds a matching file,
>>> with a failing error code, so it could end up running way too long.
>>>
>>> So the C program is fine, I guess.
>>>
>>>>>>> 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.
>>>>
>>>> I thought it was default for release builds.
>>>
>>> Doesn't look like it:
>>>
>>> https://github.com/mesonbuild/meson/blob/d00f840c573103c2d51aed2b169386f7acfe7026/mesonbuild/compilers/compilers.py#L255-L264
>>>
>>> b_ndebug defaults to false.
>>
>> Got it, thanks!
>>
>>>>>>>> + 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.
>>>>
>>>> I think sysupdate is more likely to log unsanitized data, especially
>>>> as systemd-journald has no problems with it.
>>>
>>> What's the difference between systemd-journald's behaviour and the
>>> logging we have?
>>
>> I'm not familiar with s6 at all, but I think it is at least worth
>> investigating. Also, all else equal it is best to reject invalid
>> untrusted input as early as possible.
>
> As early as possible would be in virtiofsd, not ad-hoc for this one
> service here.

That’s actually an interesting idea, but I don’t know if it would
be upstreamable.
--
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 --]