#3260 Issue open: Generic method how to safely and reliably read shell-style config files

Labels: critical / security / legal, blocker

jsmeix opened issue at 2024-06-21 05:43:

ReaR uses various different methods
to read variable values from files
in particular from shell-style config files
that are insecure because they execute foreign code
in particular

source FILE

cf. https://github.com/rear/rear/issues/3259
or
methods that work more or less reliable
for example like

eval "$(grep "^VARIABLE=" FILE)"

cf. https://github.com/rear/rear/pull/3171
which is also insecure because it may evaluate foreign code
i.e. code that comes after VARIABLE= like

VARIABLE=$( foreign code )

Attempts to generically mitigate and deal with it
were not accepted, see
https://github.com/rear/rear/pull/3203
and
https://github.com/rear/rear/pull/3258

schlomo commented at 2024-06-21 06:21:

Thank you @jsmeix for aggregating this topic here!

What is our reasoning for putting this issue into the 3.0 milestone?

Can you or @pcahyna maybe help us to get support for threat modelling from SUSE or Red Hat? I'd really love to work on our security improvements based on threat modelling and I'd assume that you have experts for that.

Finally, I was wondering what prevents ReaR from running as regular user instead of root, because that could alleviate a lot of the security concerns for the rear mkrescue part.

I also believe that we should treat the rescue/recovery mode separately from a security perspective. There might be different requirements.

gdha commented at 2024-07-01 14:11:

@jsmeix Personally, I think this task is so complicated that release 3.0 would be delayed by many, many months. Perhaps, release 3.1 would make it more feasible?

jsmeix commented at 2024-07-03 06:22:

I am afraid I could not reply earlier because
last week I was away on a team event and
this week I have to catch up with fixing other
security issues for my other software packages
that I maintain at SUSE.

jsmeix commented at 2024-07-03 06:31:

@schlomo
yes, this is my plan to ask SUSE colleagues
if there is perhaps already a commonly established
generic method how to safely and reliably read
variable values from shell-style config files.

If there is none ("it depends on your case" won't help)
I would like to get the SUSE security people involved
how to further proceed with that generic issue
(it is not ReaR alone that has this issue).

Hopefully I find time to do that properly next week
I don't want to do that "just somehow by the way".

jsmeix commented at 2024-07-03 06:39:

@gdha
because this is a security issue
I think we cannot easily delay it,
in particular not because ReaR runs as root
so when a non-trusted user could somehow modify a
shell-style config file which is executed by ReaR
(completely via 'source' or partially via 'eval')
that user could execute any command as root.

So at least for now until things are better understood
I would like to keep ReaR 3.0 as the intended target
where this issue should be at least somehow mitigated.

jsmeix commented at 2024-07-03 06:58:

Some thoughts regarding running ReaR an non-root user:

First and foremost it matters here when ReaR runs
on the original system:

"rear mkrescue" might be possible as non-root user.
We should try out how far this works.

"rear mkbackuponly" is likely not possible as non-root user
because only root can read all files to make a complete
'tar' backup.

When ReaR runs within the ReaR recovery system
(e.g. "rear recover") is a different case
that may or may not matter here:

Think about a shell-style config file
or any other file (e.g. a ReaR script) which was
modified by a non-trusted user which gets copied
into the ReaR recovery system because it needs to be
executed by ReaR (via 'source' or 'eval')
during "rear recover".
This way that non-trusted user could get the system
recreated in a modified way as he likes.

jsmeix commented at 2024-07-04 09:07:

With probability one
no chance to do something for ReaR next week.
Right now got several new security issues
assigned that I need to fix at SUSE.

gdha commented at 2024-07-04 09:22:

With probability one no chance to do something for ReaR next week. Right now got several new security issues assigned that I need to fix at SUSE.

No problem Johannes - security first ;-)

jsmeix commented at 2024-07-04 11:40:

It hurts because this issue is also security related
so "security first" would mean this issue first.
But "customers first" implies "upstream later"
so issues get fixed in backwards ordering:
First in released software then at upstream
so users and customers first get issues
because of not enough manpower upstream to fix them.
But I fear this is how business works:
Customers or users usually won't pay upstream directly.
Customers pay others for support (e.g. Linux distributors).
So customers get what they pay for:
Released upstream software with issues which require
support by others (e.g. Linux distributors).
Let's wait and see how long it still takes until
most business decision makers may finally understand
that Free Software does not mean "free as in free beer".

jsmeix commented at 2024-07-04 12:41:

I think we need a new generic function

is_trustworthy_file

that checks if a file is trustworthy.

Each file which will be executed by ReaR
(completely via 'source' or partially via 'eval')
must pass this function check.

Each file which is included
in the ReaR recovery system
must pass this function check.

What exactly trustworthy means can be further
adapted and enhanced step by step as needed
in the implementation of this function.

As an initial first step I suggest (guess what! ;-)
to implement this function for now to only ensure
the file owner is one of the TRUSTED_FILE_OWNERS
with the for now reasoning as in
https://github.com/rear/rear/pull/3258/files
i.e.:

A file is considered trustworthy
when its file owner is one of the TRUSTED_FILE_OWNERS.
Because only the file owner can 'chmod'
(cf. "man 2 chmod": caller's EUID must match owner)
we may sufficiently safely assume that a file
which is onwed by one of the TRUSTED_FILE_OWNERS
is sufficiently trustworthy without the need
for further additional checks
(e.g. if other users have permissions
to modify the file or special ACLs).
Furthermore 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.

For example (offhanded, untested) like

function is_trustworthy_file () {
    local file_name="$1"
    test "$TRUSTED_FILE_OWNERS" || return 0
    owner_name="$( stat -c %U "$file_name" )" || return 1
    IsInArray "$owner_name" "${TRUSTED_FILE_OWNERS[@]}" || return 1
}

schlomo commented at 2024-07-04 13:02:

No surprise indeed. TBH, I'm not sure however if checking for the file ownership in the rescue system is so helpful. What should happen if the file ownership doesn't match? Abort the recovery?

BTW, we also have the MD5 checksum test - is this also a security feature?

jsmeix commented at 2024-07-04 13:35:

Obviously the check that each file which is included
in the ReaR recovery system must pass this function
happens during "rear mkrescue" for the original file
on the original system with its original file owner
because inside the ReaR recovery system such a test
does not make sense where 'root' is basically the only owner.

By the way, a quick test shows "interesting" files in
the ReaR recovery system that are not owned by 'root'

crw--w---- 1 johannes tty 136, 2 Jul 4 12:53 ./dev/pts/2
crw--w---- 1 johannes tty 136, 3 Jul 4 15:22 ./dev/pts/3
crw--w---- 1 johannes tty 136, 0 Jul 4 15:15 ./dev/pts/0
crw--w---- 1 johannes tty 136, 4 Jul 4 12:57 ./dev/pts/4
crw--w---- 1 johannes tty 136, 1 Jul 4 13:45 ./dev/pts/1
crw--w---- 1 johannes tty 4, 2 Jul 4 07:29 ./dev/tty2
crw-rw---- 1 tss root 10, 224 Jul 4 07:28 ./dev/tpm0

and many other device nodes with owner group

root audio
root dialout
root disk
root input
root kmem
root kvm
root lp
root render
root tty
root video

but - as far as my quick test shows - all
directories and files in the ReaR recovery system
have owner group 'root root'

jsmeix commented at 2024-07-04 13:39:

Now back to SUSE work - I have no time for discussions here.

gdha commented at 2024-07-05 07:11:

@jsmeix @schlomo @pcahyna Why checking for ownership? I would compare md5sums of the config files with the original md5sums. I guess now you will say that can may tampered as well, but ownership can also be modified once an intruder has the root shell. What I already did for a customer is checking a certificate serial number (and that is full-proof and extreme difficult to go around it). Just some random ideas. The be clear the certificate check only is needed on the md5sum check script to ensure that script is not modified.

[Export of Github issue for rear/rear.]