Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On hold: New source_variable_from_file() #3203

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jsmeix
Copy link
Member

@jsmeix jsmeix commented Apr 9, 2024

  • Type: Enhancement

  • Impact: Normal

  • Reference to related issue (URL):

#3171

  • How was this pull request tested?

Currently mostly tested only on command line, see
#3171
and
#3203 (comment)

  • Description of the changes in this pull request:

In lib/global-functions.sh added
new function get_shell_file_config_variable()

For details see the comment
at get_shell_file_config_variable()
in lib/global-functions.sh

In lib/global-functions.sh added
new function get_var_from_file()
see #3171
@jsmeix jsmeix added the enhancement Adaptions and new features label Apr 9, 2024
@jsmeix jsmeix added this to the ReaR v2.8 milestone Apr 9, 2024
@jsmeix jsmeix requested review from pcahyna and a team April 9, 2024 11:44
@jsmeix jsmeix self-assigned this Apr 9, 2024
@jsmeix
Copy link
Member Author

jsmeix commented Apr 9, 2024

@lzaoral
in your
#3171 (comment)
you wrote

the final '|| return 1' is necessary because
bash returns 127 on unbound variables

Why does it matter what non-zero return code
the get_var_from_file() function returns?

Or in other words:
Why not have the get_var_from_file() implementation
simpler by omitting the final '|| return 1' like

function get_var_from_file() {
    bash -c 'unset $1 || exit 1 ; source "$0" >/dev/null ; set -u ; echo "${!1}"' "$1" "$2"
}

?

@lzaoral
Copy link
Contributor

lzaoral commented Apr 9, 2024

Why does it matter what non-zero return code the get_var_from_file() function returns?

@jsmeix I suppose it does not. I just did that to be consistent with surrounding functions which return either 0 or 1. It should be safe to drop this part.

Dropped the  final '|| return 1' from the
get_var_from_file() implementation, see
#3203 (comment)
@jsmeix
Copy link
Member Author

jsmeix commented Apr 9, 2024

@lzaoral
thank you for your prompt reply.
I dropped it via
ef14f5f
but I did not test it with the dropped final '|| return 1'

@lzaoral
Copy link
Contributor

lzaoral commented Apr 9, 2024

@jsmeix Thank you! It seems to work as advertised:

$ get_var_from_file /etc/os-release VERSION; echo $?
39 (Thirty Nine)
0
$ get_var_from_file /etc/os-release VERSIO; echo $?
/etc/os-release: line 1: !1: unbound variable
127
$ get_var_from_file /etc/os-releas VERSIO; echo $?
/etc/os-releas: line 1: /etc/os-releas: No such file or directory
/etc/os-releas: line 1: !1: unbound variable
127
$ get_var_from_file /etc/os-releas VERSION; echo $?
/etc/os-releas: line 1: /etc/os-releas: No such file or directory
/etc/os-releas: line 1: !1: unbound variable
127

Explain why we source the file in a separated shell
and why stdout of the sourced file must be discarded
and add specific URLs to pull request comments
that show our resoning behind
Typo fix "the the" -> "the"
Typo fix in comment: Removed false full stop '.'
because the sentence does not end there.
Typo fix in comment "interits" -> "inherits"
Copy link
Member

@pcahyna pcahyna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!
Please squash all commits into one commit with the title New global function get_var_from_file() before merging to reduce noise in the history.

Copy link
Member

@schlomo schlomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry @jsmeix that I couldn't look earlier at this. After a bit of thinking I came to the conclusion that I see the danger of using source as way too big for this kind of problem. I think that this unnecessarily opens up attack vectors into ReaR or that it can lead to unexpected behaviour due to bad configuration files.

It is bad enough that the ReaR configs are actually code, but here at least the user understands the context.

Let's please not solve this problem in this way but look for a better solution.

What is bad with something simple like the examples in https://stackoverflow.com/a/25251400/2042547?

Or maybe use sed to filter out enclosing quotation marks for values?

@pcahyna
Copy link
Member

pcahyna commented Apr 11, 2024

@schlomo I really hope we are not going to implement a parser for shell assignments when this parser already exists - in the shell itself, just because of vague security justifications. The os-release file is documented to be readable this way (see the manual page), as are /etc/sysconfig files - guess how they are read by ifup/ifdown (See source_config() in /etc/sysconfig/network-scripts/network-functions) ? What attack vectors are you speaking about when the files that we intend to source this way are already sourced at every boot?

@schlomo
Copy link
Member

schlomo commented Apr 11, 2024

For such shell files I agree with your logic, but the function claims to be more generic than shell config files.

For shell config files one can source them and echo, even in a subshell. But then I'd call the function differently, e.g. get_shell_file_config_variable or such.

@pcahyna
Copy link
Member

pcahyna commented Apr 11, 2024

@schlomo

For such shell files I agree with your logic

That's the intent of the function, as documented in the comment:

# The get_var_from_file function is meant to be used with files
# like /etc/os-release or certain files in /etc/sysconfig or /etc/default
# that basically only do shell-compatible variable assignments (VAR="value")

but the function claims to be more generic than shell config files.

But then I'd call the function differently, e.g. get_shell_file_config_variable or such.

If you feel that the function name promises more than the function is able to do, I agree with renaming it.

@schlomo
Copy link
Member

schlomo commented Apr 11, 2024

Yes, I think that would be beneficial

@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

I prefer names that tell what a function actually is about.
get_var_from_file is too unspecific.

Regarding get_shell_file_config_variable:
I wonder why it is limited to config variables?
I.e. why not only get_shell_file_variable
because it can get the value of any variable
that is set in a shell syntax file?
Or is get_shell_file_variable confusing
because one does not know if it is about
a 'shell file' or a 'file variable'
so get_shell_file_config_variable
is perhaps really what tells best what it is about?

Renamed get_var_from_file into get_shell_file_config_variable
to tell what that function actually is about, see
#3203 (comment)
@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

Via
12c3ccb
I renamed get_var_from_file into get_shell_file_config_variable
to tell what that function actually is about
plus adapted and enhanced/fixed comments.

@jsmeix jsmeix changed the title New global function get_var_from_file() New global function get_shell_file_config_variable() Apr 12, 2024
@schlomo
Copy link
Member

schlomo commented Apr 12, 2024

@jsmeix may I suggest an alternative implementation that doesn't call the bash binary and provides a little bit more error reporting compared to #3203 (comment)?

This implementation also uses Bash Restricted mode to add a little bit of security protection.

The error message also makes it clear that the config file was executed by Bash, hopefully this helps understanding errors better

About the name: Yes, I believe that the limit to shell config files is important because it doesn't read other key-value config files that use : as separator or that allow whitespace around the =

code s.sh

function Error() {
    echo -e "ERROR: $*"
    exit 1
}

function get_shell_file_config_variable() {
    [[ -r "$1" && "$2" =~ [a-zA-Z_][a-zA-Z_0-9]* ]] || Error "Arg 1 '$1' is not a readable file or arg 2 '$2' is not valid variable name"
    local file="$1"
    local key="$2"

    (
        function die() {
            echo "ERROR: $*"
            exit 1
        }
        unset $key
        set -r -e -u -o pipefail
        eval "$(< "$file")" || die "Bash errors while executing '$file'"
        [[ -v $key ]] || die "$key is not set in $file"
        echo ${!key}
    )
}

get_shell_file_config_variable /etc/os-release VERSION
echo $?

get_shell_file_config_variable /etc/os-release SCHLOMO
echo $?

get_shell_file_config_variable /etc/passwd VERSION
echo $?

get_shell_file_config_variable /etc/cron.daily/dpkg foo
echo $?

get_shell_file_config_variable /etc/nothing VERSION
echo $?

example run s.sh

root@84cc40add750:/rear/test# bash s.sh
22.04.4 LTS (Jammy Jellyfish)
0
ERROR: SCHLOMO is not set in /etc/os-release
1
s.sh: line 18: root:x:0:0:root:/root:/bin/bash: restricted: cannot specify `/' in command names
s.sh: line 19: daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 20: bin:x:2:2:bin:/bin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 21: sys:x:3:3:sys:/dev:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 22: sync:x:4:65534:sync:/bin:/bin/sync: restricted: cannot specify `/' in command names
s.sh: line 23: games:x:5:60:games:/usr/games:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 24: man:x:6:12:man:/var/cache/man:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 25: lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 26: mail:x:8:8:mail:/var/mail:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 27: news:x:9:9:news:/var/spool/news:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 28: uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 29: proxy:x:13:13:proxy:/bin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 30: www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 31: backup:x:34:34:backup:/var/backups:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 32: list:x:38:38:Mailing: command not found
s.sh: line 33: irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: eval: line 34: syntax error near unexpected token `('
s.sh: eval: line 34: `gnats:x:41:41:Gnats Bug-Reporting System (admin):/var/lib/gnats:/usr/sbin/nologin'
ERROR: Bash errors while executing '/etc/passwd'
1
s.sh: line 25: /usr/libexec/dpkg/dpkg-db-backup: restricted: cannot specify `/' in command names
ERROR: Bash errors while executing '/etc/cron.daily/dpkg'
1
ERROR: Arg 1 '/etc/nothing' is not a readable file or arg 2 'VERSION' is not valid variable name
root@84cc40add750:/rear/test# 

WDYT?

@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

I think I better withdraw my pull request here
and leave it to you and @pcahyna and @lzaoral
how to get the value of a variable out of a file
because I am neither a sufficient expert in this area
nor will I find the needed time to get this task done.

@jsmeix jsmeix marked this pull request as draft April 12, 2024 07:56
@jsmeix jsmeix changed the title New global function get_shell_file_config_variable() On hold: New global function get_shell_file_config_variable() Apr 12, 2024
@schlomo
Copy link
Member

schlomo commented Apr 12, 2024

@jsmeix why? I think that you identified a relevant problem that we have again and again. Please don't be offended by the fact that somebody might have a different perspective.

You proposed a solution, got feedback and a even suggestion for a different solution. We even already aligned on a suitable name.

Besides that you gave us the opportunity for an interesting technical challenge (secure parsing of shell configs) which is always a good thing.

I think my context is simply slightly different (and I see it as an asset for ReaR that we all are so different), for example I already implemented a set_variable_from_commvault_status function:

function set_variable_from_commvault_status {
(( $# == 2 )) || BugError "set_variable_from_commvault_status not called with 2 args: $*"
local var_name="$1" ; shift
local -n var=$var_name # $var is not a pointer to the variable to set
local match="$1" ; shift
var=$(sed -n -E -e "/$match/s/.*= //p" <<<"$commvault_status")
var=${var## *} # strip trailing blanks
contains_visible_char "$var" || Error "Could not set $var_name variable matching $match from 'commvault status':$LF$commvault_status"
}
# CommVault base paths, matching what commvault status knows
# example
# [ General ]
# Version = 11.26.35
# Media Revision = 1014
# CommServe Host Name = commserv.some.domain
# CommServe Client Name = commserv
# Home Directory = /opt/commvault/Base
# Log Directory = /var/log/commvault/Log_Files

Maybe you just collect all the feedback and proceed with that? I at least greatly appreciate the love to the details that you bring to ReaR!

@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

@schlomo
at least for now I would like in this particular case
to relax from this particular Relax-and-Recover issue.
I spent already way too many hours on this issue
which seems to have basically unlimited depth
so at least for now I feel a bit "fed up" with it.
Let's wait and see how things look to me
after the weekend (but no promises).

@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

@pcahyna @lzaoral
feel free to use what I further developed here so far
which is all based on your foundations during
#3171
to make whatever
"get the value of a variable out of a file"
functionality as you like most for your needs.

@jsmeix
Copy link
Member Author

jsmeix commented Apr 12, 2024

Since
#3177
is merged there is a merge conflict here
which I solved right now:

Renamed get_shell_file_config_variable
into source_variable_from_file
to make it explicit that the file is sourced.
@jsmeix jsmeix changed the title On hold: New global function get_shell_file_config_variable() On hold: New source_variable_from_file() Apr 15, 2024
@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

Via
f79454f
I renamed the function again,
now from get_shell_file_config_variable
into source_variable_from_file
to make it explicit that the file is sourced
and
to make the name less generic (no longer generic 'get')
so we can - as needed - implement alternative functions
how to get a variable value out of a file
with different methods.

Some "forensics" why we use 'source' at all:

Using 'source' was introduced via
https://github.com/rear/rear/pull/3165/files
therein see
#3165 (review)
which shows the following outdated code diff

28  -  local version_id=$(grep "^VERSION_ID=" /etc/os-release | cut -d\" -f2 ) # 7

28  +
29  +  local version_id
30  +  version_id="$(. /etc/os-release && echo "$VERSION_ID")"

where 'grep' was replaced by '.'
There I only liked to have '.' replaced by explicit 'source'
but I did not question why 'grep' was replaced by 'source'.

Because 'source' fails for readonly variables, see
#3165 (comment)
@lzaoral "reworked the parsing completely", cf.
#3165 (comment)
so that 'source' was no longer used and currently
we use 'grep' again but now together with 'eval', see
https://github.com/rear/rear/pull/3165/files

Then during
#3171
in
#3171 (review)
@pcahyna proposed again to use 'source'
(now with a separated "bash in between")
because

I think this is simpler and more robust than using eval and grep
(think of possible multi-line strings, for example).

see also
#3171 (comment)

... the "bash in between"
which was up to now more a workaround for source
that has a problem with readonly variables

(End of "forensics" why we use 'source'.)

From there we further developed
how to get a variable value out of a file
with the 'source' method.

For me the generic advantage of the 'source' method is
that we let 'bash' interpret and execute the file
so we get the value that 'bash' actually sets
(think of possible 'if' conditions, for example).

Of course the generic disadvantage of the 'source' method is
that we let 'bash' interpret and execute the file
so we execute a (shell-syntax) file only
to get a variable value out of a file.

I think when we restrict the sourcing shell
or when we let it abort at any errors, then
the generic advantage of the 'source' method
does no longer hold, see my experiments in
#3171

I think we either use 'source' as it normally works
and then the sourced file must be trusted
or we better use another method
when the sourced file cannot be trusted.

But even then we would need additional security things
to avoid unintended bash interpretation/execution
of the variable value when it is e.g. something like

$( COMMAND )

or whatever kind of bash code that could trigger
interpretation/execution when used later in bash.

This all looks rather hopeless to get it secure.
Perhaps it actually is hopeless to securely
process unknown files or use data from unknown files?

@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

An offhanded generic idea how to avoid problems
with
"safely processing/interpreting untrustworthy files"
or with
"safely using data from untrustworthy files"
or generically with
"safely using untrustworthy files":

My idea is to "simply" not use untrustworthy files.
(wow - what a great ingenious new idea ;-)

Actually my idea is that a file is untrustworthy
for a particular user account,
if other users could have modified that file.

Or in other words:
Only those files are trustworthy for a particular user account,
where only that particular user could have written the file.

For ReaR this means:
Only those files are trustworthy to be used by ReaR
where only 'root' could have written the file.

To check if only 'root' could have written a file
the only possible way in practice is
to check the current file owner, group, and permissions.

So I suggest another new helper function:

function is_trustworthy_for_root () {
    local resolved_file
    resolved_file="$( readlink -e "$1" )" || return 1
    # Owner name and group name must be 'root root':
    test "$( stat -c '%U %G' $resolved_file )" = "root root" || return 1
    # Neither group nor others must have write permission
    # so the human readable permission string must be '-'
    # for group and others for example like "-rwxr-xr-x"
    [[ "$( stat -c '%A' $resolved_file )" == ?????-??-? ]]
}

This helper function can then be called where needed
e.g. in source_variable_from_file() like

    is_trustworthy_for_root "$1" || return 1
    bash -c ...

Added new helper function is_trustworthy_for_root
to check if only 'root' could have written a file
which makes such files trustworthy to be used by ReaR
and call that helper function in source_variable_from_file(), see
#3203 (comment)
@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

My curent is_trustworthy_for_root implementation
is likely on the one hand too sceptical because
e.g. on my openSUSE Leap 15.5 system:

# stat -c '%U %G %A' /etc/chrony.keys 
root chrony -rw-r-----

# ( set -x ; is_trustworthy_for_root /etc/chrony.keys && echo Y || echo N )
+ is_trustworthy_for_root /etc/chrony.keys
+ local resolved_file
++ readlink -e /etc/chrony.keys
+ resolved_file=/etc/chrony.keys
++ stat -c '%U %G' /etc/chrony.keys
+ test 'root chrony' = 'root root'
+ return 1
+ echo N
N

but on the other hand it is likely too careless
because it considers only traditional
owner, group, and permissions
but not newer things like ACLs.

@schlomo
Copy link
Member

schlomo commented Apr 15, 2024

@jsmeix I like your idea to reduce to attack surface by checking for trustworthy files (and you could use stat --format "%A %u %g" to check for all requirements in one comparison, if you like).

I'm wondering if we really need that as we actually don't have that check in the Source() function of ReaR and don't care about the ownership of the ReaR files themselves.

So maybe your initial thought was correct and we don't need to care so much about that?

@schlomo
Copy link
Member

schlomo commented Apr 15, 2024

@jsmeix I was thinking about further along the lines of "preventing command execution" and came up with this approach: Setting the PATH in the restricted shell to something without binaries in it effectively prevents running anything.

Here is the amended version of my suggestion embedded in a shell script to test the behavior:

function Error() {
    echo -e "ERROR: $*"
    exit 1
}

function get_shell_file_config_variable() {
    [[ -r "$1" && "$2" =~ [a-zA-Z_][a-zA-Z_0-9]* ]] || Error "Arg 1 '$1' is not a readable file or arg 2 '$2' is not valid variable name"
    local file="$1"
    local key="$2"

    (
        function die {
            echo "ERROR: $*"
            exit 1
        }
        set -e -u -o pipefail
        unset $key
        declare -rx PATH=/dev/null
        set -r
        eval "$(< "$file")" || die "Bash errors while executing '$file'"
        [[ -v $key ]] || die "$key is not set in $file"
        echo ${!key}
    )
}

get_shell_file_config_variable /etc/os-release VERSION
echo $?

get_shell_file_config_variable /etc/os-release SCHLOMO
echo $?

get_shell_file_config_variable /etc/passwd VERSION
echo $?

get_shell_file_config_variable /etc/cron.daily/dpkg foo
echo $?

bad_file=$(mktemp)
echo -e "date\nFOO='bar in $bad_file'" >$bad_file
get_shell_file_config_variable $bad_file FOO
echo $?

get_shell_file_config_variable /etc/nothing VERSION
echo $?

Is there a reason why you prefer to run an extra bash binary? This version also has cleaner error handling and doesn't show internal errors about undefined variables or such:

root@9487ecb740f3:/rear/test# bash s.sh
22.04.4 LTS (Jammy Jellyfish)
0
ERROR: SCHLOMO is not set in /etc/os-release
1
s.sh: line 20: root:x:0:0:root:/root:/bin/bash: restricted: cannot specify `/' in command names
s.sh: line 21: daemon:x:1:1:daemon:/usr/sbin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 22: bin:x:2:2:bin:/bin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 23: sys:x:3:3:sys:/dev:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 24: sync:x:4:65534:sync:/bin:/bin/sync: restricted: cannot specify `/' in command names
s.sh: line 25: games:x:5:60:games:/usr/games:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 26: man:x:6:12:man:/var/cache/man:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 27: lp:x:7:7:lp:/var/spool/lpd:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 28: mail:x:8:8:mail:/var/mail:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 29: news:x:9:9:news:/var/spool/news:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 30: uucp:x:10:10:uucp:/var/spool/uucp:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 31: proxy:x:13:13:proxy:/bin:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 32: www-data:x:33:33:www-data:/var/www:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 33: backup:x:34:34:backup:/var/backups:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: line 34: list:x:38:38:Mailing: command not found
s.sh: line 35: irc:x:39:39:ircd:/run/ircd:/usr/sbin/nologin: restricted: cannot specify `/' in command names
s.sh: eval: line 36: syntax error near unexpected token `('
s.sh: eval: line 36: `gnats:x:41:41:Gnats Bug-Reporting System (admin):/var/lib/gnats:/usr/sbin/nologin'
ERROR: Bash errors while executing '/etc/passwd'
1
s.sh: line 27: /usr/libexec/dpkg/dpkg-db-backup: restricted: cannot specify `/' in command names
ERROR: Bash errors while executing '/etc/cron.daily/dpkg'
1
s.sh: line 20: date: command not found
bar in /tmp/tmp.5UswwxhrrT
0
ERROR: Arg 1 '/etc/nothing' is not a readable file or arg 2 'VERSION' is not valid variable name
root@9487ecb740f3:/rear/test# 

ATM there is only one scenario that I don't like which is that I didn't manage to abort reading the config file if there is anything wrong with it. The example with line 20: date: command not found illustrates that: The line with date in the "config file" is ignored but the 2nd line with FOO= is then read. Strictly speaking I'd prefer the code to abort on that with an error.

@schlomo
Copy link
Member

schlomo commented Apr 15, 2024

Since we talked about the example of reading /etc/os-release I read again https://www.freedesktop.org/software/systemd/man/latest/os-release.html and realised that we should also read /usr/lib/os-release if it doesn't exist... More complexity and probably not part of the function under discussion, but of the use case for which we bring it in.

Fixed is_trustworthy_for_root:
Only check that the owner name is 'root' because
the group does not matter when it has no write permissions.
Treat files with an ACL as untrustworthy to be on the safe side
because (at last currently) ACLs are not checked.
See #3203 (comment)
@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

My current is_trustworthy_for_root is meant
for files that are not ReaR files,
like some shell-syntax config files.

We use our Source() function only for ReaR files
(hopefully - as far as I assumed up to now).

We don't care about the ownership of the ReaR files
and currently I cannot tell if this is a problem.
It could be a problem when we e.g. Source() files
as ReaR scripts that could have been "injected"
by others (i.e. new scripts in writable directories)
or modified by others (i.e. existing scripts).
Perhaps we may better call is_trustworthy_for_root
for each file that we Source() to be on the safe side?
Hopefully at least usr/share/rear/lib/global-functions.sh
is safe so that others cannot modify is_trustworthy_for_root().
Up to now I did not at all think about that.
Perhaps too carelessly I blindly assumed that
in any case only root can install ReaR
(perhaps because only root can install RPMs)
so I assumed ReaR files are owned by root and all is safe.
But I never verified that others cannot modify ReaR files.

@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

Regarding "why calling 'source'"
and "why having another bash in between"
see my explanations above and in particular
see the history of all that in
#3165
and
#3171

Neither "calling 'source'"
nor "having another bash in between"
was my idea so I cannot properly answer questions
about all the reasoning behind.

I only recognized that things worked better
(i.e. less false failures from too simple implementations)
when using 'source' from within a separated bash
but I did not compare that with possible other ways
how one could get a variable value out of a file.

I would much appreciate if if @pcahyna and @lzaoral
could comment here to help to get things sorted out
so that we all better understand each other.

@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

@schlomo
regarding your suggested get_shell_file_config_variable():

I think your goal is different than the goal
behind the 'source' method, cf. my
#3203 (comment)
therein in particular what I think what
the generic advantage of the 'source' method is
and what
the generic disadvantage of the 'source' method is.

So it makes sense to also have your function in ReaR
for cases where we cannot 'source' a file.

Perhaps over time we learn that in practice we never
actually needed to 'source' a file to get a variable
value out of it and then we could "just drop"
that source_variable_from_file function
because it is only a ReaR internal function
so we are free to do with it what we want.

Perhaps in the end only is_trustworthy_for_root
and its consequences "survive" from here
which could be a rather good end result.

@jsmeix
Copy link
Member Author

jsmeix commented Apr 15, 2024

Regarding /etc/os-release versus /usr/lib/os-release:

At least on my openSUSE Leap 15.5 I have:

# ls -l /etc/os-release
lrwxrwxrwx 1 root root ... /etc/os-release -> ../usr/lib/os-release

so at least on openSUSE Leap 15.5
/etc/os-release exists

# rpm -qf /etc/os-release
openSUSE-release-15.5-lp155.286.1.x86_64

but I don't know if /etc/os-release exists
as file or as symlink to /usr/lib/os-release
on other Linux distributions.

By the way:
This link was the reason why I do

resolved_file="$( readlink -e "$filename" )" || return 1

in is_trustworthy_for_root() because otherwise
the link would be reported as untrustworthy because

# stat -c %A /etc/os-release
lrwxrwxrwx

@jsmeix
Copy link
Member Author

jsmeix commented Apr 18, 2024

FYI
where we call 'source' in our scripts:

I used the output of

# find usr/sbin/rear usr/share/rear/ -type f \
 | xargs egrep '^source |[^$_(]source ' \
 | egrep -v ': *#|resource|while read source'

and removed the false positives:

usr/sbin/rear:
if ! source $SHARE_DIR/conf/default.conf ; then
if ! source $SHARE_DIR/lib/_input-output-functions.sh ; then
    source $script || BugError "Failed to source $script"
    source $SHARE_DIR/lib/progresssubsystem.nosh || BugError "Failed to source $SHARE_DIR/lib/progresssubsystem.nosh"

usr/share/rear/restore/NBKDC/default/400_restore_backup.sh:
source $VAR_DIR/recovery/nbkdc_settings

usr/share/rear/layout/recreate/default/200_run_layout_code.sh:
        ( source $LAYOUT_CODE )

usr/share/rear/layout/prepare/default/200_recreate_hpraid.sh:
        ( source $LAYOUT_CODE )

usr/share/rear/layout/prepare/Linux-s390/400_run_dasd_format_code.sh:
    ( source $DASD_FORMAT_CODE )

usr/share/rear/layout/save/GNU/Linux/190_opaldisk_layout.sh:
    source "$(opal_device_attributes "$device" attributes)"

usr/share/rear/skel/default/bin/dhclient-script:
source /etc/scripts/dhcp-setup-functions.sh

usr/share/rear/skel/default/etc/profile:
        source "$script"

usr/share/rear/skel/default/etc/syslog-ng-v3.conf:
source src {

usr/share/rear/skel/default/etc/scripts/system-setup:
source /usr/share/rear/conf/default.conf || echo -e "\n'source /usr/share/rear/conf/default.conf' failed with exit code $?"
        source /etc/rear/$conf.conf || echo -e "\n'source /etc/rear/$conf.conf' failed with exit code $?"
        source $system_setup_script || echo -e "\n'source $system_setup_script' results exit code $?\n"
        if ! source $system_setup_script ; then

usr/share/rear/skel/default/etc/scripts/system-setup.d/58-start-dhclient.sh:
source /etc/scripts/dhcp-setup-functions.sh

usr/share/rear/skel/default/etc/scripts/unlock-opal-disks:
source /usr/share/rear/lib/opal-functions.sh
[[ -f /.OPAL_PBA_SETTINGS.sh ]] && source /.OPAL_PBA_SETTINGS.sh
[[ ${#OPAL_PBA_TKNPATH[@]} -gt 0 ]] && source /usr/share/rear/lib/authtoken-functions.sh
    source "/etc/scripts/system-setup.d/$system_setup_script"

usr/share/rear/skel/default/etc/syslog-ng.conf:
source src {

usr/share/rear/skel/NBKDC/etc/scripts/system-setup.d/90-start-nbkdc.sh:
source $VAR_DIR/recovery/nbkdc_settings

usr/share/rear/skel/SESAM/etc/scripts/system-setup.d/59-start-sesam-client.sh:
        source $SHARE_DIR/lib/sesam-functions.sh
        source $SESAM_VAR_DIR/var/ini/sesam2000.profile

usr/share/rear/prep/DUPLICITY/default/200_find_duply_profile.sh:
            source $CONF    # is a normal shell configuration file
    source $DUPLY_PROFILE_FILE

usr/share/rear/prep/SESAM/default/400_prep_sesam.sh:
        source $SHARE_DIR/lib/sesam-functions.sh

usr/share/rear/rescue/GNU/Linux/220_load_modules_from_initrd.sh:
        source /etc/sysconfig/kernel
            source /etc/dracut.conf
                source $s

usr/share/rear/lib/rear-shell.bashrc:
for script in $SHARE_DIR/lib/*functions.sh ; do source $script ; done
source $SHARE_DIR/lib/progresssubsystem.nosh

usr/share/rear/lib/drlm-functions.sh:
                source $DRLM_CFG
            source $DRLM_CFG

usr/share/rear/lib/opaladmin-workflow.sh:
        source "$(opal_device_attributes "$device" attributes)"

usr/share/rear/lib/sesam-functions.sh:
source $sesam2000ini_file

usr/share/rear/lib/opal-functions.sh:
    source "$(opal_device_attributes "$device" attributes)"
        source "$(opal_device_attributes "$device" attributes)"

usr/share/rear/lib/framework-functions.sh:
    source "$source_file"

usr/share/rear/lib/global-functions.sh:
        source $CONF    # is a normal shell configuration file

usr/share/rear/build/OPALPBA/Linux-i386/810_deduplicate_files.sh:
source "$deduplication_script"

usr/share/rear/build/USB/default/800_enforce_usb_output.sh:
local_conf_output=$( source $ROOTFS_DIR/etc/rear/local.conf ; echo $OUTPUT )

i.e. 43 places where we call 'source' in our scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss / RFC enhancement Adaptions and new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants