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

Demi Marie Obenour
2025-09-08T18:21:56Z

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