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

Alyssa Ross
2025-11-06T10:15:13Z

[-- Attachment #1: Type: text/plain, Size: 3187 bytes --]

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

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

I imagine this could fit with the work that's being done on pluggable
backends[1][2].

[1]: https://youtu.be/qsFc234tzz4?si=Qw2b4MzerLWCX39J&t=239
[2]: https://gitlab.com/virtio-fs/virtiofsd/-/issues/147

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