#3424 PR `merged`: TRUSTED_OWNERS and TRUSTED_PATHS to protect against code injection via 'source'¶

Labels: fixed / solved / done, severe improvement

jsmeix opened issue at 2025-03-11 11:44:¶

Type: Enhancement
Impact: High

There will be regressions when third-party scripts
use '.' for sourcing because currently sourcing via '.'
is always forbidden in ReaR.

I would like to wait a bit and see if sourcing via '.'
is sometimes actually needed in practice.
If sourcing via '.' is sometimes needed,
I will further enhance things to make sourcing via '.'
again possible for the user by explicitly specifying
in another config array which specific file paths
he allows to be sourced via '.' by ReaR,
cf. https://github.com/rear/rear/pull/3434#issuecomment-2742598092
and https://github.com/rear/rear/issues/3259#issuecomment-2730011077
and https://github.com/rear/rear/pull/3437

Reference to related issue (URL):
https://github.com/rear/rear/issues/3259
How was this pull request tested?

See below and see also my tests in
https://github.com/rear/rear/pull/3379

Description of the changes in this pull request:

In default.conf describe the new config variables
TRUSTED_OWNERS and TRUSTED_PATHS
which are used to provide the new
general protection against code injection via 'source'
see https://github.com/rear/rear/issues/3259
and "Protecting Against Code Injections" in
https://relax-and-recover.org/documentation/security-architecture

jsmeix commented at 2025-03-11 13:47:¶

I tested "rear mkrescue" and "rear mkbackuponly".
Tomorrow I will test "rear recover"...

jsmeix commented at 2025-03-12 07:44:¶

I tested "rear recover".

So for me on a SLES15-SP6 test VM
(with the SLES15 default btrfs structure)
"rear mkrescue" plus "rear mkbackuponly"
and then on another same test VM "rear recover"
"just worked" - i.e. with the defaults for
TRUSTED_OWNERS and TRUSTED_PATHS.

I use this etc/rear/local.conf
(with the usual settings for the SLES15 default btrfs structure)

OUTPUT=ISO
BACKUP=NETFS
BACKUP_OPTIONS="nfsvers=3,nolock"
BACKUP_URL=nfs://192.168.178.66/nfs
REQUIRED_PROGS+=( snapper chattr lsattr )
COPY_AS_IS+=( /usr/lib/snapper/installation-helper /etc/snapper/config-templates/default )
POST_RECOVERY_SCRIPT=( 'if snapper --no-dbus -r $TARGET_FS_ROOT get-config | grep -q "^QGROUP.*[0-9]/[0-9]" ; then snapper --no-dbus -r $TARGET_FS_ROOT set-config QGROUP= ; snapper --no-dbus -r $TARGET_FS_ROOT setup-quota && echo snapper setup-quota done || echo snapper setup-quota failed ; else echo snapper setup-quota not used ; fi' )
SSH_ROOT_PASSWORD="rear"
USE_DHCLIENT="yes"
FIRMWARE_FILES=( 'no' )
MODULES=( 'loaded_modules' )

cf.
https://github.com/rear/rear/wiki/Test-Matrix-ReaR-2.8#sles-15-sp-6-with-default-btrfs-structure

jsmeix commented at 2025-03-12 07:54:¶

As far as I understand it
the currently failing checks

testing-farm:centos-stream-8-x86_64 — Failed to provision test environment.
Please check https://status.testing-farm.io for possible outages.
If the situation persists after retrying, please report an issue.

testing-farm:centos-stream-9-x86_64 — Failed to provision test environment.
Please check https://status.testing-farm.io for possible outages.
If the situation persists after retrying, please report an issue.

show on https://status.testing-farm.io the reason

Public Ranch Outage - bug in security groups cleanup.
...
This issue is not resolved yet

So it seems the reason for those currently failing checks
is outside of the ReaR environment - in particular the
reason is not the code changes in this pull request.

jsmeix commented at 2025-03-12 08:04:¶

@schlomo @gdha @pcahyna
please have a look here - as time permits.

For the new script that combines
lib/_input-output-functions.sh and lib/framework-functions.sh
I chose the name _framework-setup-and-functions.sh
because the shorter name _framework-functions.sh would be
misleading because in contrast to the other ...functions.sh scripts
this special _... script does not only define framework functions
but also does some framework setup actions so it does both
framework setup and defining framework functions, in particular
input-output functions (from _input-output-functions.sh)
and sourcing functions (from framework-functions.sh).

jsmeix commented at 2025-03-13 06:04:¶

@rear/contributors
as noone objected against my chosen name
_framework-setup-and-functions.sh
I will now change all ReaR scripts that mention
the old name _input-output-functions.sh (usually in comments)
to show the new name _framework-setup-and-functions.sh

jsmeix commented at 2025-03-13 11:06:¶

@rear/contributors
I need your help regarding how to properly use

[[ ... =~ ... ]]

in particular with variables like

[[ $foo =~ ...$bar... ]]

My current usage in _framework-setup-and-functions.sh

actual_path="$( readlink -e "$file" )" || Error "is_trusted_path(): 'readlink -e $file' failed"
for trusted_path in "${TRUSTED_PATHS[@]}" ; do
    # Skip when trusted_path is empty (otherwise the [[ expression ]] would be falsely true):
    test "$trusted_path" || continue
    [[ $actual_path =~ ^$trusted_path ]] && return 0
done

cf. https://github.com/rear/rear/pull/3424#discussion_r1992872215
and https://github.com/rear/rear/pull/3424#discussion_r1993048851

This is how [[ ... =~ ... ]] is currently used in ReaR

# find usr/sbin/rear usr/share/rear/ -type f | xargs grep -oh '\[\[.*=~.*\]\]'

[[ $latest_tar_restore_file =~ .*/.* ]]
[[ $latest_tar_restore_file =~ .*/.* ]]
[[ "$BLOCKCLONE_ALLOW_MOUNTED" =~ ^[nN0] ]]
[[ $ID_NEW =~ ^dm- ]]
[[ $1 =~ /dev/mapper/ ]]
[[ $fs_spec =~ ^/dev/disk/by-id/ ]]
[[ $DEV_NAME =~ ^dm- ]]
[[ $1 =~ /dev/disk/by-id ]]
[[ $major_version =~ ^[0-9]+$ ]]
[[ $BORGBACKUP_ARCHIVE_PREFIX =~ [^a-zA-Z0-9] ]]
[[ "\$REPLY" =~ ^[Yy1] ]]
[[ "$UDEV_UID_LED" =~ ^[yY1] ]]
[[ "$UDEV_UID_LED" =~ ^[yY1] ]]
[[ "$DEVPATH" && "$UDEV_SUSPEND" =~ ^[yY1] ]]
[[ "$UDEV_BEEP" =~ ^[yY1] ]]
[[ $disk_name =~ ^dm- ]]
[[ "$name" =~ ^mapper/ ]]
[[ "$name" =~ ^dm- ]]
[[ ${device_name: -1} =~ [0-9] ]]
[[ "$test_ip" =~ $ip_pattern ]]
[[ $length =~ [^0-9] ]]
[[ $BACKUP_INTEGRITY_CHECK =~ ^[yY1] && "$(basename ${BACKUP_PROG})" = "tar" ]]
[[ "$BLOCKCLONE_ALLOW_MOUNTED" =~ ^[nN0] ]]

Currently I use totally unquoted

[[ $actual_path =~ ^$trusted_path ]]

which seems to work rather well for me, see
https://github.com/rear/rear/pull/3424#discussion_r1993048851

But probably I should better use

[[ "$actual_path" =~ ^$trusted_path ]]

or

[[ "$actual_path" =~ ^"$trusted_path" ]]

which also work for me, see
https://github.com/rear/rear/pull/3424#discussion_r1993048851

Because "rear mkrescue" and "rear mkbackuponly" work for me with

[[ "$actual_path" =~ ^"$trusted_path" ]]

I use this now to be more on the safe side via
https://github.com/rear/rear/pull/3424/commits/634401289f8103816f9750d556575b03e87aaa47

jsmeix commented at 2025-03-13 12:58:¶

@rear/contributors
tomorrow I will test it again with all my recent changes
and if it still works well for my test case, cf.
https://github.com/rear/rear/pull/3424#issuecomment-2716931938
and when there are no objections
I would like to merge it next Monday afternoon

jsmeix commented at 2025-03-17 14:51:¶

Regarding
https://github.com/rear/rear/pull/3424#issuecomment-2720862367
I did
https://github.com/rear/rear/pull/3424/commits/634401289f8103816f9750d556575b03e87aaa47
so I now use the more secure

[[ "$actual_path" =~ ^"$trusted_path" ]]

instead of totally unquoted

[[ $actual_path =~ ^$trusted_path ]]

because

[[ "$actual_path" =~ ^"$trusted_path" ]]

also works for me.

jsmeix commented at 2025-03-17 15:22:¶

Because "All checks have passed" and because
it also works for me (as far as I tested it)
I will merge it as is right now.

I must postpone the "cosmetic" part in
https://github.com/rear/rear/pull/3424#issuecomment-2720074872

... change all ReaR scripts that mention
the old name _input-output-functions.sh (usually in comments)
to show the new name _framework-setup-and-functions.sh

to a subsequent pull request (hopefully rather soon - but I cannot promise it)
and here it is: https://github.com/rear/rear/pull/3431

jsmeix commented at 2025-03-19 08:39:¶

Via https://github.com/rear/rear/pull/3379
where this pull request here is based on
I introduced a regression, see
https://github.com/rear/rear/pull/3379#issuecomment-2735731302
which I fixed now via
https://github.com/rear/rear/commit/d25816643715ee7c7fb4477be6bd8993663ca268

jsmeix commented at 2025-03-24 07:02:¶

While I did
https://github.com/rear/rear/pull/3424#discussion_r2009556786
I found another regression:

# usr/sbin/rear -s mkrescue
Relax-and-Recover 2.9 / 2025-01-31
Running rear mkrescue (PID 23165 date 2025-03-24 07:55:42)
Using log file: /root/rear.github.master/var/log/rear/rear-localhost.log
Simulation mode activated, Relax-and-Recover base directory: /root/rear.github.master/usr/share/rear
Source lib/array-functions.sh
...
Source lib/mkrescue-workflow.sh
...
Source lib/write-protect-functions.sh
Source conf/Linux-i386.conf
Source conf/GNU/Linux.conf
Source init/default/001_verify_config_arrays.sh
Source init/default/002_check_rear_recover_mode.sh
Source init/default/010_EFISTUB_check.sh
Source init/default/010_set_drlm_env.sh
Source init/default/030_update_recovery_system.sh
Source init/default/100_check_stale_nfs_mounts.sh
Source init/default/950_check_missing_programs.sh
Source init/default/998_dump_variables.sh
ERROR: The specified command 'mkrescue' does not exist!
Exiting rear mkrescue (PID 23165) and its descendant processes ...
Running exit tasks

This is because of
https://github.com/rear/rear/pull/3424/commits/7df7dc6499b7703b98d7293fbbf5de4d514a29ac
which results that also lib/*-workflow.sh scripts
get sourced by the Source function in simulation mode
so that e.g. in lib/mkrescue-workflow.sh
the WORKFLOW_mkrescue() function is not set.

jsmeix commented at 2025-03-24 09:30:¶

https://github.com/rear/rear/pull/3436
intends to fix the above
https://github.com/rear/rear/pull/3424#issuecomment-2747079736

jsmeix commented at 2025-03-25 09:57:¶

The regression described in the initial comment here

There will be regressions when third-party scripts
use '.' for sourcing because currently sourcing via '.'
is always forbidden in ReaR.

[Export of Github issue for rear/rear.]

#3424 PR merged: TRUSTED_OWNERS and TRUSTED_PATHS to protect against code injection via 'source'¶