#3319 Issue `closed`: Checking where ReaR sources files with '.' and trying to verify those cases¶

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2024-09-17 09:07:¶

See
https://github.com/rear/rear/issues/3260#issuecomment-2331033842
and
https://github.com/rear/rear/issues/3260#issuecomment-2248252572
(excerpt)

usr/share/rear/skel/default/bin/dhclient-script:
                    . ${f}
            . ${ETCDIR}/dhclient-${interface}-down-hooks
            . ${ETCDIR}/dhclient-down-hooks
                    . ${f}

usr/share/rear/skel/default/etc/scripts/dhcp-setup-functions.sh:
        . ${ETCDIR}/dhclient-exit-hooks
            . ${ETCDIR}/dhclient-${interface}-up-hooks
            . ${ETCDIR}/dhclient-up-hooks

usr/share/rear/skel/default/etc/scripts/run-syslog:
    . /usr/share/rear/lib/layout-functions.sh

usr/share/rear/skel/default/etc/scripts/system-setup.d/00-functions.sh:
. /usr/share/rear/lib/global-functions.sh
. /usr/share/rear/lib/network-functions.sh

usr/share/rear/restore/GALAXY/default/400_restore_with_galaxy.sh:
       . ./GxCmd

jsmeix commented at 2024-09-17 09:23:¶

Regarding

usr/share/rear/skel/default/bin/dhclient-script:
                    . ${f}
            . ${ETCDIR}/dhclient-${interface}-down-hooks
            . ${ETCDIR}/dhclient-down-hooks
                    . ${f}

usr/share/rear/skel/default/etc/scripts/dhcp-setup-functions.sh:
        . ${ETCDIR}/dhclient-exit-hooks
            . ${ETCDIR}/dhclient-${interface}-up-hooks
            . ${ETCDIR}/dhclient-up-hooks

According to

# git log -p --follow usr/share/rear/skel/default/bin/dhclient-script | egrep '^commit| \. '
...
commit 38d5bd280654dd4e05a8a408daad8e08925c3ab0
+                    . ${f}
+            . ${ETCDIR}/dhclient-${interface}-down-hooks
+            . ${ETCDIR}/dhclient-down-hooks
+                    . ${f}

# git log -p --follow usr/share/rear/skel/default/etc/scripts/dhcp-setup-functions.sh | egrep '^commit| \. '
...
commit 38d5bd280654dd4e05a8a408daad8e08925c3ab0
+        . ${ETCDIR}/dhclient-exit-hooks
+            . ${ETCDIR}/dhclient-${interface}-up-hooks
+            . ${ETCDIR}/dhclient-up-hooks

sourcing via '.' in
usr/share/rear/skel/default/bin/dhclient-script and
usr/share/rear/skel/default/etc/scripts/dhcp-setup-functions.sh
originate from
https://github.com/rear/rear/commit/38d5bd280654dd4e05a8a408daad8e08925c3ab0
which has only this commit message

Added DHCP client support to rear. Sponsored by J&J.

but this commit is also where
usr/share/rear/lib/network-functions.sh
originated and this contains

+# set of functions that will be used by our own implementation
+# of dhclient-script, but these can/could be used by other
+# scripts as well
+#
+# Most of the functions are coming from the fedora dhclient-script

So I guess that also
usr/share/rear/skel/default/bin/dhclient-script and
usr/share/rear/skel/default/etc/scripts/dhcp-setup-functions.sh
came from Fedora at the time of that commit
i.e. on Dec 10, 2010

On the one hand this indicates that this code in ReaR
could be OK (because we may assume Fedora code is OK).

On the other hand this code in ReaR is rather old
(more than 13 years meanwhile) and I assume
possible (security) bug fixes at Fedora
were not backported into ReaR.

This code in ReaR should be checked if it is considered
to be still sufficiently OK for what ReaR needs
(DHCP client support in the ReaR recovery system)
so I filed
https://github.com/rear/rear/issues/3320

pcahyna commented at 2024-09-17 09:31:¶

I would like to use some more sophisticated parser to identify this use of ., but I have not started this yet.

If you want to make sure that no . is used and source is used instead, enable -n . is your friend. The bash manual says about . that "This builtin is equivalent to ‘source’.", but they can be enabled or disabled separately:

$ cat print_foo 
echo foo
$ enable -n .
$ source print_foo 
foo
$ . print_foo 
bash: .: command not found

jsmeix commented at 2024-09-17 09:32:¶

Regarding

usr/share/rear/skel/default/etc/scripts/run-syslog:
    . /usr/share/rear/lib/layout-functions.sh

usr/share/rear/skel/default/etc/scripts/system-setup.d/00-functions.sh:
. /usr/share/rear/lib/global-functions.sh
. /usr/share/rear/lib/network-functions.sh

/usr/share/rear/lib/layout-functions.sh
/usr/share/rear/lib/global-functions.sh
/usr/share/rear/lib/network-functions.sh
are ReaR's own files so no third-party file is sourced here.

I will replace '.' by 'source' - done right now via
https://github.com/rear/rear/commit/d19d51993853e763c1aba5f9828075acf0dfdf07
and
https://github.com/rear/rear/commit/a1bcb21a54cb27a7e2ce5564a7ef05b79c5cdacd

jsmeix commented at 2024-09-17 09:55:¶

Regarding

usr/share/rear/restore/GALAXY/default/400_restore_with_galaxy.sh:
       . ./GxCmd

From plain looking at the code I cannot make sense of it.
In particular 'GxCmd' appears only in
usr/share/rear/restore/GALAXY/default/400_restore_with_galaxy.sh
(excerpts)

# GxCmd checks for $# and calls the GxCmdLine script if there are any cmdline args.
...
        . ./GxCmd
...
        ./GxCmd -cmd restore ...

so it seems GxCmd (and GxCmdLine) belong to
the BACKUP=GALAXY software.
In this case it is probably OK to source GxCmd
because the BACKUP=GALAXY software can be trusted.
Nevertheless it should be verified whether or not
my above assumption that '. ./GxCmd' can be trusted
actually holds so I filed
https://github.com/rear/rear/issues/3321

I will replace '.' by 'source' - done right now via
https://github.com/rear/rear/commit/f3de9e84949f181b5eb8ab8a1e702e87c5d236d5

jsmeix commented at 2024-09-17 10:28:¶

All cases that I found and listed in this issue
are now done so I close this issue as done.

For possibly further cases where '.' is used
to source files, see
https://github.com/rear/rear/issues/3260#issuecomment-2355192852
and
https://github.com/rear/rear/issues/3260#issuecomment-2355205283

jsmeix commented at 2024-09-17 10:49:¶

@pcahyna
regarding your
https://github.com/rear/rear/issues/3319#issuecomment-2355033810
in particular the part about enable -n .

'man bash' reads

enable [-a] [-dnps] [-f filename] [name ...]
  Enable and disable builtin shell commands.
  Disabling a builtin allows a disk command
  which has the same name as a shell builtin
  to be  executed ...

Could it be ever possible that a disk command
named '.' can exist?
I think it cannot because '.' is the current directory
but I don't know for sure.

In ReaR we use already in
usr/share/rear/lib/_input-output-functions.sh

# Make sure nobody else can use trap:
function trap () {
    BugError "Forbidden usage of trap with '$*'. Use AddExitTask instead."
}

which seems to also work:

# function . () { echo "Forbidden usage of '.'" ; }

# type -a .
. is a function
. () 
{ 
    echo "Forbidden usage of '.'"
}
. is a shell builtin

# . print_foo
Forbidden usage of '.'

# builtin . print_foo
foo

Perhaps it is better to use a '.' function?
In both cases one can still call the shell builtin '.'
via 'builtin':

# unset -f .

# type -a .
. is a shell builtin

# enable -n .

# type -a .
bash: type: .: not found

# builtin . print_foo
foo

jsmeix commented at 2024-09-19 09:58:¶

I think it is better to use a '.' function
instead of enable -n . because with a function
we can implement what should happen when '.' is used
(e.g. BugError or whatever else we like).

I tested with current GitHub master code
if '.' is used in my personal test case
by adding at the beginning of sbin/rear

function . () { echo "Forbidden usage of '.'" ; exit 99 ; }

My personal test case etc/rear/local.conf

OUTPUT=ISO
BACKUP=NETFS
BACKUP_OPTIONS="nfsvers=3,nolock"
BACKUP_URL=nfs://192.168.178.66/nfs
REQUIRED_PROGS=( "${REQUIRED_PROGS[@]}" snapper chattr lsattr )
COPY_AS_IS=( "${COPY_AS_IS[@]}" /usr/lib/snapper/installation-helper /etc/snapper/config-templates/default )
BACKUP_PROG_INCLUDE=( /boot/grub2/x86_64-efi /boot/grub2/i386-pc /home /opt /root /srv /tmp /var /usr/local )
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' )
PROGRESS_MODE="plain"
PROGRESS_WAIT_SECONDS="5"
USE_SERIAL_CONSOLE=''

on a SLES15-SP6 test VM with 15 GiB disk and default btrfs structure.

I tested the following workflows

mkbackup
recover (on another test VM)
help
checklayout
dump
savelayout
shell
validate

All worked for me in my personal test case.
Of course this cannot check if '.' is used for other use cases.

jsmeix commented at 2024-09-20 11:56:¶

While thinking about
using a '.' function to forbid usage of '.'
it crossed my mind
that we could also use a 'source' wrapper function
so normal usage of 'source' in scripts
calls our 'source' wrapper function
so we get a central place in ReaR
where we keep control over what is sourced
and where we could implement what we need
like path-based checks or whatever else
(e.g. log details in debug mode).

Of course this is not for ReaR 3.0
but for a ReaR version after ReaR 3.0.

What I will never ever do again is to
manually search in a clumsy and annoying way
through all our code where something is sourced.

I did a quick proof of concept
with current GitHub master code
by adding at the beginning of sbin/rear

function source () { echo "Sourcing '$*'" 1>&2 ; builtin source "$@" ; }

on the same SLES15-SP6 test VM
with 15 GiB disk and default btrfsstructure as above and
"rear -D mkrescue" worked for my particular test case.

Redirection to stderr is mandatory in the 'source' function
otherwise things badly error out in 400_copy_modules.sh
which happens - as far as I see at first glance - because of
usr/share/rear/rescue/GNU/Linux/220_load_modules_from_initrd.sh

# Fedora, Red Hat & new SUSE uses dracut
if test -s /etc/dracut.conf ; then
    MODULES_LOAD+=(
        $(
            add_drivers=
            source /etc/dracut.conf
            for s in /etc/dracut.conf.d/*.conf ; do
                source $s
            done
            echo $add_drivers
        )
    )
fi

cf. https://github.com/rear/rear/issues/3285#issuecomment-2244555297

So stdout of 'source ...' gets added to MODULES_LOAD
which makes 400_copy_modules.sh error out with

Copying only currently loaded kernel modules (MODULES contains 'loaded_modules') and those in MODULES_LOAD
ERROR: Sourcing loaded or to be loaded but no module file?

when 'Sourcing' appears on stdout of the 'source' wrapper function.

By the way:
I found the reason why 400_copy_modules.sh errors out
rather fast and easily via

# grep '^Sourcing ' var/log/rear/rear-localhost.log | grep -v 'usr/share/rear/'
Sourcing '/root/rear.github.master/etc/rear/local.conf'
Sourcing '/etc/dracut.conf'
Sourcing '/etc/dracut.conf.d/10-persistent_policy.conf'
Sourcing '/etc/dracut.conf.d/99-debug.conf'
Sourcing '/etc/dracut.conf.d/ostree.conf'

i.e. the /etc/dracut.conf* files are the only ones
we 'source' which look "suspicious" ;-)

[Export of Github issue for rear/rear.]

#3319 Issue closed: Checking where ReaR sources files with '.' and trying to verify those cases¶

jsmeix opened issue at 2024-09-17 09:07:¶

jsmeix commented at 2024-09-17 09:23:¶

pcahyna commented at 2024-09-17 09:31:¶

jsmeix commented at 2024-09-17 09:32:¶

jsmeix commented at 2024-09-17 09:55:¶

jsmeix commented at 2024-09-17 10:28:¶

jsmeix commented at 2024-09-17 10:49:¶

jsmeix commented at 2024-09-19 09:58:¶

jsmeix commented at 2024-09-20 11:56:¶

#3319 Issue `closed`: Checking where ReaR sources files with '.' and trying to verify those cases¶