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
base: master
Are you sure you want to change the base?
Conversation
In lib/global-functions.sh added new function get_var_from_file() see #3171
@lzaoral
Why does it matter what non-zero return code Or in other words:
? |
@jsmeix I suppose it does not. I just did that to be consistent with surrounding functions which return either |
Dropped the final '|| return 1' from the get_var_from_file() implementation, see #3203 (comment)
@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"
There was a problem hiding this 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.
There was a problem hiding this 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?
@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 |
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. |
That's the intent of the function, as documented in the comment:
If you feel that the function name promises more than the function is able to do, I agree with renaming it. |
Yes, I think that would be beneficial |
I prefer names that tell what a function actually is about. Regarding |
Renamed get_var_from_file into get_shell_file_config_variable to tell what that function actually is about, see #3203 (comment)
Via |
@jsmeix may I suggest an alternative implementation that doesn't call the 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 code s.sh
example run s.sh
WDYT? |
@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 rear/usr/share/rear/prep/GALAXY11/default/400_prep_galaxy.sh Lines 5 to 23 in 09579ae
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! |
@schlomo |
Since |
Renamed get_shell_file_config_variable into source_variable_from_file to make it explicit that the file is sourced.
Via Some "forensics" why we use 'source' at all: Using 'source' was introduced via
where 'grep' was replaced by '.' Because 'source' fails for readonly variables, see Then during
see also
(End of "forensics" why we use 'source'.) From there we further developed For me the generic advantage of the 'source' method is Of course the generic disadvantage of the 'source' method is I think when we restrict the sourcing shell I think we either use 'source' as it normally works But even then we would need additional security things
or whatever kind of bash code that could trigger This all looks rather hopeless to get it secure. |
An offhanded generic idea how to avoid problems My idea is to "simply" not use untrustworthy files. Actually my idea is that a file is untrustworthy Or in other words: For ReaR this means: To check if only 'root' could have written a file So I suggest another new helper function:
This helper function can then be called where needed
|
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)
My curent
but on the other hand it is likely too careless |
@jsmeix I like your idea to reduce to attack surface by checking for trustworthy files (and you could use I'm wondering if we really need that as we actually don't have that check in the So maybe your initial thought was correct and we don't need to care so much about that? |
@jsmeix I was thinking about further along the lines of "preventing command execution" and came up with this approach: Setting the 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
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 |
Since we talked about the example of reading |
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)
My current is_trustworthy_for_root is meant We use our Source() function only for ReaR files We don't care about the ownership of the ReaR files |
Regarding "why calling 'source'" Neither "calling 'source'" I only recognized that things worked better I would much appreciate if if @pcahyna and @lzaoral |
@schlomo I think your goal is different than the goal So it makes sense to also have your function in ReaR Perhaps over time we learn that in practice we never Perhaps in the end only is_trustworthy_for_root |
Regarding /etc/os-release versus /usr/lib/os-release: At least on my openSUSE Leap 15.5 I have:
so at least on openSUSE Leap 15.5
but I don't know if /etc/os-release exists By the way:
in is_trustworthy_for_root() because otherwise
|
FYI I used the output of
and removed the false positives:
i.e. 43 places where we call 'source' in our scripts. |
Type: Enhancement
Impact: Normal
Reference to related issue (URL):
#3171
Currently mostly tested only on command line, see
#3171
and
#3203 (comment)
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