#3258 PR open: On hold: Implement SourceTrustworthy function

Labels: enhancement, critical / security / legal

jsmeix opened issue at 2024-06-19 15:27:

In lib/framework-functions.sh
new SourceTrustworthy function
to source only a trustworthy file.

A file is considered trustworthy to be sourced
when its file owner is one of the TRUSTED_FILE_OWNERS.

Because only the file owner can 'chmod'
cf. "man 2 chmod" that reads

The effective UID of the calling process
must match the owner of the file,
or the process must be privileged
(Linux: it must have the CAP_FOWNER capability).

we can sufficiently safely assume that a file
which is onwed by one of the TRUSTED_FILE_OWNERS
can be trusted to be sourced without further additional checks
(e.g. if other users may have permissions to modify the file)
and it should not be ReaR's task to prevent TRUSTED_FILE_OWNERS
from doing what they want (a.k.a. "final power to the user")
or simply put: TRUSTED_FILE_OWNERS means we do trust them.

jsmeix commented at 2024-06-19 15:29:

Currently this is WIP.

I need to replace 'source' with 'SourceTrustworthy'
at the 43 places where we basically carelessly "just source"
various files, see
https://github.com/rear/rear/pull/3203#issuecomment-2063439858

jsmeix commented at 2024-06-19 15:35:

Additionally I like to rename the 'Source' function
into a more descriptive name that better tells
for what use case that function is used.

Here where the 'Source' function is used:

# find usr/sbin/rear usr/share/rear/ -type f \
 | xargs grep 'Source ' \
 | grep -v ': *#' \
 | egrep -v 'CallerSource|Netbackup Client Source'

usr/sbin/rear:
    Source "$CONFIG_DIR/os.conf" || Error "Failed to Source $CONFIG_DIR/os.conf"
    Source "$CONFIG_DIR/$WORKFLOW.conf" || Error "Failed to Source $CONFIG_DIR/$WORKFLOW.conf"
        Source "$SHARE_DIR/conf/$config.conf" || BugError "Failed to Source $SHARE_DIR/conf/$config.conf"
        Source "$CONFIG_DIR/$config.conf" || Error "Failed to Source $CONFIG_DIR/$config.conf"
            Source "$config_append_file_path" || Error "Failed to Source $config_append_file_path"
            Source "$config_append_file_path.conf" || Error "Failed to Source $config_append_file_path.conf"

usr/share/rear/layout/recreate/default/200_run_layout_code.sh:
                    Source $SHARE_DIR/layout/recreate/default/150_wipe_disks.sh

usr/share/rear/lib/rear-shell.bashrc:
Source .../script.sh        runs a single ReaR script
WORKING_DIR=$SHARE_DIR # ensure that we can run Source ...script.sh via tab completion and that the Source function will stay there

usr/share/rear/lib/savelayout-workflow.sh:
    Source $SHARE_DIR/prep/default/320_include_uefi_env.sh

usr/share/rear/lib/framework-functions.sh:
function Source () {
    test -d "$source_file" && Error "Source file '$source_file' is a directory, cannot source"
        LogPrint "Source $relname"
    test "0" -eq "$source_return_code" || Debug "Source function: 'SourceTrustworthy $source_file' returns $source_return_code"
        Source $SHARE_DIR/$stage/"$script"

From this use cases I think a better name could be

SourceScript

because also ReaR config files are sourced as bash scripts.

jsmeix commented at 2024-06-19 15:44:

Hooray!
All "testing-farm" tests are green!

Only

Build Packages / build (push) Failing after ...

but I blindly guess that package build failures
cannot come from my code changes here
or could I be wrong @pcahyna ?

jsmeix commented at 2024-06-20 07:32:

Oh - how interesting - I gain trust in our CI tests again:

My yesterday evening (I had to leave) blind guess in
https://github.com/rear/rear/pull/3258#issuecomment-2179011767
was actually wrong - I really did not expect that!

The "Details" link at "Build Packages / build (push) Failing"
https://github.com/rear/rear/actions/runs/9592719660/job/26451792170?pr=3258
shows at "Check rear dump"

Run tools/run-in-docker -- rear dump
********** ubuntu:20.04                             **********
Refused 'SourceTrustworthy /rear/usr/share/rear/conf/Linux-i386.conf' because file owner 'UNKNOWN' is not in TRUSTED_FILE_OWNERS
ERROR: 
====================
BUG in /rear/usr/sbin/rear line 749:
Failed to Source /rear/usr/share/rear/conf/Linux-i386.conf
--------------------
Please report it at https://github.com/rear/rear/issues
and include all related parts from /tmp/rear/log/rear/rear-ubuntu-20-04.log.lockless
preferably the whole debug information via 'rear -D dump'
====================
Use debug mode '-d' for some debug messages or debugscript mode '-D' for full debug messages with 'set -x' output
Aborting due to an error, check /tmp/rear/log/rear/rear-ubuntu-20-04.log.lockless for details
Terminated
ERROR: ############### DOCKER RUN FAILED FOR ubuntu:20.04
** SCRIPT RUN TIME 1 SECONDS **
Error: Process completed with exit code 1.

But this contradicts what I get for "rear -D mkrescue"
on my openSUSE Leap 15.5 system in the ReaR log file

2024-06-20 09:16:39.052955910 Relax-and-Recover 2.7 / Git
2024-06-20 09:16:39.057979558 Running rear mkrescue (PID 10137 date 2024-06-20 09:16:38)
2024-06-20 09:16:39.062392894 Command line options: usr/sbin/rear -D mkrescue
2024-06-20 09:16:39.065588235 Using log file: /root/rear.github.master/var/log/rear/rear-localhost.log
2024-06-20 09:16:39.069734508 Using build area: /var/tmp/rear.5cSX88Kc9xrjAap
2024-06-20 09:16:39.074266908 Setting TMPDIR to ReaR's '/var/tmp/rear.5cSX88Kc9xrjAap/tmp' (was unset when ReaR was launched)
2024-06-20 09:16:39.078916770 Current set of flags is 'hB'
2024-06-20 09:16:39.082586936 The debugscripts flags are 'x'
2024-06-20 09:16:39.086138348 Combining configuration files
2024-06-20 09:16:39.105047229 Including conf/Linux-i386.conf
2024-06-20 09:16:39.109768796 Entering debugscript mode via 'set -x'.
+ SourceTrustworthy /root/rear.github.master/usr/share/rear/conf/Linux-i386.conf
+ local source_file=/root/rear.github.master/usr/share/rear/conf/Linux-i386.conf
+ local source_return_code=0
+ local source_file_owner_name=
+ local saved_bash_flags_and_options_commands=
+ test -f /root/rear.github.master/usr/share/rear/conf/Linux-i386.conf
+ test root
++ stat -c %U /root/rear.github.master/usr/share/rear/conf/Linux-i386.conf
+ source_file_owner_name=root
+ IsInArray root root bin daemon sync shutdown halt operator
+ source /root/rear.github.master/usr/share/rear/conf/Linux-i386.conf
++ REQUIRED_PROGS+=(sfdisk)
++ PROGS+=(grub lilo)
+ source_return_code=0
+ test 0 -eq 0
+ cd /root/rear.github.master
+ return 0
+ source_return_code=0
+ test 0 -eq 0
+ test 1
+ Debug 'Leaving debugscript mode (back to previous bash flags and options settings).'
2024-06-20 09:16:39.122919106 Leaving debugscript mode (back to previous bash flags and options settings).
+ [[ -n '' ]]
+ return 0

Of course also rear -D dump and rear dump
"just work" for me.

The matching code in usr/sbin/rear is ('cat -n' output):

   745  for config in "$ARCH" "$OS" \
   746          "$OS_MASTER_VENDOR" "$OS_MASTER_VENDOR_ARCH" "$OS_MASTER_VENDOR_VERSION" "$OS_MASTER_VENDOR_VERSION_ARCH" \
   747          "$OS_VENDOR" "$OS_VENDOR_ARCH" "$OS_VENDOR_VERSION" "$OS_VENDOR_VERSION_ARCH" ; do
   748      if test -r "$SHARE_DIR/conf/$config.conf" ; then
   749          Source "$SHARE_DIR/conf/$config.conf" || BugError "Failed to Source $SHARE_DIR/conf/$config.conf"
   750      fi
   751  done

But again it seems the root cause of this CI failure
is not actually in my code changes here
(my code changes here only trigger it)
but the root cause is actually in the environment
where usr/sbin/rear is is run for this CI task.

I guess that in this environment

stat -c %U /rear/usr/share/rear/conf/Linux-i386.conf

results 'UNKNOWN'.
I have no idea how this could happen.

# strings /usr/bin/stat | grep UNKNOWN
UNKNOWN (0x%lx)
UNKNOWN

indicates that 'UNKNOWN' really comes from 'stat'
and "man 2 stat" indicates it its "EXAMPLE" code
that 'unknown' could/should be printed as fallback.
But I have no idea how it could happen that

%U     user name of owner

(from "man 1 stat") could be 'UNKNOWN' for a file,
cf. the "EXAMPLE" code in "man 2 stat"
that prints the "Ownership" unconditioned
(but that prints only the numerical value).

Regardless how this could happen
it seems the CI failure

Build Packages / build (push) Failing

is nothing but false alarm
which only wastes my precious time
and distracts me from getting done what I want to get done
because I have to do time consuming investigations
where I need to prove that CI failures
are not caused by my code changes.

So I am back to ignoring CI failures :-(

jsmeix commented at 2024-06-20 08:03:

On SLES15-SP5 with its default LVM and btrfs, cf.
https://github.com/rear/rear/wiki/Test-Matrix-rear-2.6#sles-15-sp-1-with-default-lvm-and-btrfs-structure
I did a full "rear mkbackup" plus "rear recover"
and all worked well for me.

jsmeix commented at 2024-06-20 08:08:

I will keep separated issues separated
so this pull request will be only
to implement the new SourceTrustworthy function
plus adaption of the Source function
to use the new SourceTrustworthy function.

I will replace 'source' with 'SourceTrustworthy'
at the 43 places where we basically carelessly "just source"
various files via a separated second pull request
after this one here was merged.

Finally - as time permits - I may optionally do
https://github.com/rear/rear/pull/3258#issuecomment-2178996703
as a third pull request.

schlomo commented at 2024-06-20 08:56:

Maybe I missed some info or discussion, so I need to unfortunately ask again: I don't fully understand how this change will significantly increase our security posture? I'd assume that an attacker who can place a script into /usr/share/rear/foo/bar can also chown it to belong to root.

I'm not aware of world writeable directories that we SourceStage() from. So I'm afraid that this check won't really do much in real life.

About the implementation:

  • Maybe a simpler change would be to add the file owner check into the Source() function?
  • Maybe simply rename 'Source()→SourceScript()` if you want the function name to be more specific?

Finally, can it be that the finding in usr/share/rear/layout/recreate/default/200_run_layout_code.sh is actually a bug? Like could we reorder the scripts so that this explicit call is not required.

jsmeix commented at 2024-06-20 09:08:

@schlomo
I think you missed to read
https://github.com/rear/rear/pull/3258#issuecomment-2178983222
and you missed to have a look at
https://github.com/rear/rear/pull/3203#issuecomment-2178737640
https://github.com/rear/rear/pull/3203#issuecomment-2063439858

jsmeix commented at 2024-06-20 09:28:

The assumption that ReaR code is always located
under a system directory with sufficiently secure
permissions, groups, and file owners does not hold.

In particular I do not know what could be possible
for "git clone" things done by whatever user and
stored under arbitrary directories and then
"just run" that via 'sudo' or something like that.

On the one hand I think it is not ReaR's task
to protect 'root' against any foolish things.

But because ReaR is meant to be run as 'root'
I think we must implement some reasonable protection
to avoid at least too obviously bad things
which can be easily avoided.

In the end I think we are back at where we had been in
https://github.com/rear/rear/issues/2967
where @schlomo basically doesn't care
and leaves security things to the user while
I have an opposite point of view as described in
https://github.com/rear/rear/issues/2967#issuecomment-1498627825
and
https://github.com/rear/rear/issues/2967#issuecomment-1510856524

In this case I think when making software this means
that one's program must not knowingly omit to
provide basic protection against something bad
in particular when it became known that
basic protection against something bad
is missing in a particular specific case
and it is known how basic protection
can be implemented with reasonable effort.

schlomo commented at 2024-06-20 09:53:

Ah, maybe a much simpler check would be to ensure that we only source files that have the same owner as the rear main script? For installed ReaR that will typically be root and for a git checkout it will be some user ID. However, I'd also allow sourcing files owned by the effective UID, in addition to the user owning the rear main script (so that you can include /etc/rear/*.conf from ReaR running out of a git checkout via sudo.

schlomo commented at 2024-06-20 09:58:

BTW, I'm not opposed to adding a security check to Source, I'm more thinking about the question how to make that as simple as possible. Hence my suggestions for simplification.

jsmeix commented at 2024-06-20 10:19:

Actually using SourceTrustworthy in the Source function
was more or less a "side effect" of my original intent
to implement a reasonable simple and sufficiently safe
function that should replace the plain 'source' calls
of various files that we do at 43 places in our code.

While or ReaR scripts are sufficiently under our control
those various third-party files that get "just sourced"
are basically out of our control.
Primarily for those I implemented SourceTrustworthy.

As a test how far SourceTrustworthy may work
I used it in the Source function and because
it "just works" there I kept it there.

In contrast implementing better protection
directly in the Source function would not help
for those actually scaring 43 cases.

gdha commented at 2024-06-20 10:28:

Wouldn't it be safer to reduce the amount of configuration files in place?

jsmeix commented at 2024-06-20 11:15:

@gdha
perhaps I misunderstand you but I assume you mean that
instead of only replacing the plain 'source' calls
by calling SourceTrustworthy
if it wouldn't be safer to actually fix each
plain 'source' call individually
as needed at each place?

Yes, of course that would be better and safer.

It just requires several people who just know about
each one of those 43 places what each code there
is meant to do and WHY it is implemeted there
by plain 'source' calls and not better/safer
which also requires understanding of the
contents of each one of those sourced files
(some cases are clear like 'source default.conf')
but some cases look rather obscure - at least to me.

So, yes, when someone contributes that major cleanup
to ReaR, I would much appreciate it because that
would allow me to do more pleasant things than trying
to somehow generically mitigate old mess in ReaR
that I unfortunately detected by accident so now
it is me who tries to mitigate it as far as I could
with reasonable effort within reasonable time.

I feel this one will end same as
https://github.com/rear/rear/pull/3203
and
https://github.com/rear/rear/pull/3171

schlomo commented at 2024-06-20 19:36:

@schlomo I think you missed to read #3258 (comment) and you missed to have a look at #3203 (comment) #3203 (comment)

@jsmeix I looked at that and I actually don't see how checking file ownership relates to the original problem of #3203 (reading shell-style config files).

I'd suggest holding back on this change till we understand what exactly we are trying to protect against. And I'd suggest for us to be careful to add more complexity to ReaR without a clear user requirement.

Personally, I'd be much more happy to add a security check for a problem that was either reported by a user or that we understood from threat modeling.

BTW, It seems to me that the file owner check came much later into #3203 compared to the issue description of the PR. Maybe our discussion got carried away there?

[Export of Github issue for rear/rear.]