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

[Alyssa Ross <hi@alyssa.is>]

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

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?

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

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

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

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