#2286 PR merged: Do not ckeck untrusted files for missing libraries (issue 2279)

Labels: enhancement, cleanup, fixed / solved / done

jsmeix opened issue at 2019-11-25 16:26:

On my openSUSE Leap 15.0 system I did "rear -D mkrescue" and
still got the same files in the recovery system as before this changes
with this etc/rear/local.conf

OUTPUT=ISO
BACKUP=NETFS
BACKUP_OPTIONS="nfsvers=3,nolock"
BACKUP_URL=nfs://192.168.122.1/nfs
SSH_ROOT_PASSWORD="rear"
USE_DHCLIENT="yes"
PROGRESS_MODE=plain
PROGRESS_WAIT_SECONDS=3
KEEP_BUILD_DIR="yes"
  • Brief description of the changes in this pull request:

Now the RequiredSharedObjects function skips running ldd
for non-regular files, kernel modules, firmware files and
in particular for files that are not owned by 'root'
to mitigate possible ldd security issues
(see the comments in the code).

Additionally I added at several places

2>>/dev/$DISPENSABLE_OUTPUT_DEV

cf. https://github.com/rear/rear/pull/2024
which reduced the "rear -D mkrescue" log file size
from about 5 MiB to about 2 MiB.
Without that the rear -D mkrescue" log file size
with the changes here would "explode" to about 17 MiB
(from about 5 MiB before the changes here)
because with the changes here ldd is run separatedly
for each file in RequiredSharedObjects so we would get
additional tons of set -x output for each ldd call.

jsmeix commented at 2019-11-25 16:27:

@OliverO2
FYI here is the subsequent pull request for
https://github.com/rear/rear/issues/2279#issuecomment-557516738

pcahyna commented at 2019-11-25 16:30:

in particular for files that are not owned by 'root'

What about files owned by a system user, like bin?

jsmeix commented at 2019-11-25 16:40:

@pcahyna
currently I test numerical owner ID
and skip all files wher the owner ID is not 0
which skips all files owned by non-root system users.

So I think I need to better check for owner user names
because names should be same on various Linux distributions
and I think I need a new config variable where the by default trusted
owner names are listed so that the user could add his additionally
trusted owners.

I will enhance this pull request soon...

jsmeix commented at 2019-11-25 17:08:

Now it skips ldd for files that are not owned by a user
in the new TRUSTED_FILE_OWNERS config array.

jsmeix commented at 2019-11-25 17:18:

@pcahyna
FYI when you need full debug output to see
what RequiredSharedObjects actually does
you must use --debugscripts x e.g. as in

usr/sbin/rear --debugscripts x mkrescue

cf. https://github.com/rear/rear/pull/2024

OliverO2 commented at 2019-11-25 17:23:

Another note: It might be (I haven't researched this) that some backup method integrated into ReaR pulls in executables and/or libraries with their own user ids, say something like bacula. In that case there would be a regression potential, as these will no longer be checked for dependencies, correct?

jsmeix commented at 2019-11-25 17:29:

@OliverO2
yes this regression potential exists.

The manual solution for a particular user is in his etc/rear/local.conf

TRUSTED_FILE_OWNERS+=( 'my_backup_prog_owner' )

The long-term solution at ReaR upstream is code like

test "$BACKUP" = "my_backup" && TRUSTED_FILE_OWNERS+=( 'my_backup_prog_owner' )

and/or additional backup specific config variables in default.conf
that define additional backup specific trusted users that need
to get added to TRUSTED_FILE_OWNERS with similar code.

jsmeix commented at 2019-11-25 17:35:

Fortunately (actually intentionally) build/default/990_verify_rootfs.sh
has become meanwhile rather good and should in very most cases
detect when there are executables in the recovery system
without required libraries.

OliverO2 commented at 2019-11-25 17:38:

build/default/990_verify_rootfs.sh uses ldd in the same way as the original copy-as-is, so it would need the same protection against accidental root-execution as well, right?

jsmeix commented at 2019-11-26 11:41:

@OliverO2
you are right - I had falsely assumed all files in the recovery system
are owned by root because that is the only user in the recovery system
but in fact the files in the recovery system preserve owner group and
permissions from what they have on the original system.

So I need to implement that also for 'ldd' in 990_verify_rootfs.sh

OliverO2 commented at 2019-11-26 12:51:

@jsmeix

but in fact the files in the recovery system preserve owner group and
permissions from what they have on the original system.

The problem is that files from an unprivileged user may be executed with root privileges. It would not help if these files became owned by root due copying as their origin would still be unprivileged.

So retaining user ids actually saves you a lot of effort. Otherwise you'd have to remember the original user id of each file copied and things would become even more complex.

Have you thought about using objdump instead of ldd, which would be the recommended safe solution? Yes I know that would involve doing recursive checks but the overall effort seems less than what is currently required.

jsmeix commented at 2019-11-26 13:19:

I meant if all files in the recovery system were owned by 'root'
I could not have tested for ownership to get the trusted ones
so I had no idea what to do for 'ldd' in 990_verify_rootfs.sh.

From our glibc experts at SUSE I learned that
at least since SLES11 (probably even before) our 'ldd' is a
modified version that does not directly execute the program
so from a SUSE point of view there is no need to change anything here
and I also learned that objdump would be preferred over ldd but
even more I perfer to do things step by step and
most of all I prefer to keep ReaR 2.x backward compatible
(as far as possible with reasonable effort) which means
to not cause regressions by changing things in a rush
or by changing several things at once.

In particular because I am not at all an expert in this area
I must change the currently working things in careful little steps.
There are so many subtle interdependencies in ReaR that
I cannot imagine what may break when I change something.
My little step by little step way is my attempt to get a bit better
understanding how things actually work in ReaR while I am doing
those little steps so that I can a little bit better avoid regressions.
If I understood all ReaR code I would...

My next step is to use TRUSTED_FILE_OWNERS to check
all files before they are copied into the recovery system
or directly after they were copied into the recovery system
(depending on what works better or is simpler to implement).

jsmeix commented at 2019-11-28 09:25:

Since my last commit here
https://github.com/rear/rear/pull/2286/commits/450447b9349472785a04fb8947de489c587b9dcd
Log messages are explicitly appended to the log file to ensure
that Log "some log message" actually appears in the log file
even inside { ... } 2>>/dev/$DISPENSABLE_OUTPUT_DEV
that is now used more often since my first commit here
https://github.com/rear/rear/pull/2286/commits/1b11a296a1ba813e265f8219247889aa09f2e454

jsmeix commented at 2019-11-28 09:30:

From my point of view what this pull request intends to do
Do not ckeck untrusted files for missing libraries
is now implemented (but not more)
and it works for me
so that I would like to merge it tomorrow
unless there are objections.

My next step is to use TRUSTED_FILE_OWNERS to check
all files before they are copied into the recovery system
or directly after they were copied into the recovery system
(depending on what works better or is simpler to implement)
which will be done via a separated pull request (as time permits).

jsmeix commented at 2019-11-29 11:18:

With my latest
https://github.com/rear/rear/pull/2286/commits/8343ba60ab0a713232bc7ba936dbe1fbe7b8a0ed
things even look good to me on the terminal:

# usr/sbin/rear -v mkrescue
...
Testing that the recovery system in /tmp/rear.JHPrACQMdwXn5Oo/rootfs contains a usable system
Skipped ldd test for '/home/JohnDoe/.xinitrc.template' (owner 'JohnDoe' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/JohnDoe/bin/parted' (owner 'JohnDoe' not in TRUSTED_FILE_OWNERS)
Creating recovery/rescue system initramfs/initrd initrd.cgz with gzip default compression
...

# usr/sbin/rear mkrescue
Skipped ldd test for '/home/JohnDoe/.xinitrc.template' (owner 'JohnDoe' not in TRUSTED_FILE_OWNERS)
Skipped ldd test for '/home/JohnDoe/bin/parted' (owner 'JohnDoe' not in TRUSTED_FILE_OWNERS)
#

so the user cannot say he was not informed when later his
special /home/JohnDoe/bin/parted does not work
because it needs a special /home/JaneDoe/libparted.so.666.
Perhaps a bit technical message but I prefer technically correct messages
over convenient to read but technically vague messages and
I do not like to sound patronizing by telling the user what to do
via whatever you should or please do phrases.

jsmeix commented at 2019-11-29 11:19:

I will merge it today afternoon if there are no objections.

[Export of Github issue for rear/rear.]