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

[Demi Marie Obenour <demiobenour@gmail.com>]

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

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.

>> 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?

>> +		(-*)
>> +			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.

>> +			:
>> +			;;
>> +		(*)
>> +			printf 'Path "%s" is neither relative nor a Nix store path\n' "$i" >&2
>> +			exit 1
>> +			;;
>> +		esac
>> +	done
>> +}
>> +
>>  while read -r arg1; do
>>  	read -r arg2 || ex_usage
>>  
>> @@ -38,6 +66,7 @@ while read -r arg1; do
>>  	echo
>>  
>>  	if [ "$arg2" = / ]; then
>> +		check_path "$arg1"
>>  		cp -RT -- "$arg1" "$root"
>>  		# Nix store paths are read-only, so fix up permissions
>>  		# so that subsequent copies can write to directories
>> @@ -47,6 +76,8 @@ while read -r arg1; do
>>  		continue
>>  	fi
>>  
>> +	check_path "$arg1" "$arg2"
>> +
>>  	parent=$(dirname "$arg2")
>>  	mkdir -p -- "$root/$parent"
>>  	cp -RT -- "$arg1" "$root/$arg2"
>>
>> -- 
>> 2.51.0


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