Re: [PATCH 04/20] scripts/make-erofs.sh: Validate all paths

Alyssa Ross
2025-09-10T18:55:03Z

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

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

> On 9/8/25 04:36, Alyssa Ross wrote:
>> Demi Marie Obenour <demiobenour@gmail.com> writes:
>>
>>> This isn't a security feature as the input is trusted, but it might
>>> catch some bugs in the future. Additionally, it will allow replacing an
>>> external command with builtin string manipulation, as paths that the
>>> builtin manipulation would mishandle will instead be rejected.
>>
>> In general this feels a bit overkill to me, but it depends — have you
>> encountered bugs this would help prevent?
>
> No, but it does make me more confident about omitting calls to an
> external dirname command, which should speed stuff up.

I see. I suppose it comes down to whether not running dirname speeds
things up enough to justify it.

>>> Signed-off-by: Demi Marie Obenour <demiobenour@gmail.com>
>>> ---
>>> scripts/make-erofs.sh | 31 +++++++++++++++++++++++++++++++
>>> 1 file changed, 31 insertions(+)
>>>
>>> diff --git a/scripts/make-erofs.sh b/scripts/make-erofs.sh
>>> index e63bcbed9c3028f0f2b55431d46ba9ec67bc26ef..cf942972910c76e1835dc5b0084c2d04bf084a9d 100755
>>> --- a/scripts/make-erofs.sh
>>> +++ b/scripts/make-erofs.sh
>>> @@ -28,6 +28,34 @@ trap 'chmod -R +w -- "$root" && rm -rf -- "$superroot"' EXIT
>>> root=$superroot/real_root
>>> mkdir -- "$root"
>>>
>>> +check_path () {
>>> + # Various code can only handle paths that do not end with /
>>> + # and are in canonical form. Reject others.
>>> + for i; do
>>> + case $i in
>>> + (''|.|..|./*|../*|*/|*/.|*/..|*//*|*/./*|*/../*)
>>> + printf 'Path "%s" is /, //, empty, or not canonical\n' "$i" >&2
>>> + exit 1
>>> + ;;
>>> + (*[!A-Za-z0-9._@+/-]*)
>>> + printf 'Path "%s" has forbidden characters\n' "$i" >&2
>>> + exit 1
>>> + ;;
>>
>> Not sure why we'd want to rule out most characters? We're not really in
>> control of what characters packages choose to use in their store paths.
>
> I believe Nix has an allowlist of permitted characters in store paths.
> Is this documented, or is it just in the C++ source code?

I'm not sure! I've not heard of such a thing.

>>> + (-*)
>>> + printf 'Path "%s" begins with -\n' "$i" >&2
>>> + exit 1
>>> + ;;
>>> + (/nix/store/*|[!/]*)
>>
>> It's technically possible to use Nix with a different store path, so I'd
>> like to avoid anything that requires us to hardcode /nix/store.
>
> Right now, the generated images depend on the store paths, so
> the scripts would need to be adapted to support this. If we
> are going to generalize this, I recommend using a proper
> scripting language like Python, Perl, or Lua.

The only place I see where we hardcode a store path is
host/initramfs/default.nix, which is a bug and easy to fix with Nix
code.

Of course you wouldn't reproduce the same image if you built with a
different store directory, but it shouldn't be invalid to do so.

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