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

[Alyssa Ross <hi@alyssa.is>]

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