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

[Alyssa Ross <hi@alyssa.is>]

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