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

Alyssa Ross
2025-09-08T08:36:47Z

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