commit: b654697f3d39014e4600ec5f314a7e94a0637c26 Author: Kerin Millar <kfm <AT> plushkava <DOT> net> AuthorDate: Sun Feb 12 09:14:13 2023 +0000 Commit: Sam James <sam <AT> gentoo <DOT> org> CommitDate: Sun Feb 12 18:12:21 2023 +0000 URL: https://gitweb.gentoo.org/proj/gentoo-functions.git/commit/?id=b654697f
Prevent code injection and naming conflicts in yesno() Currently, the yesno() function works by first trying to match the first argument against a series of patterns that would reasonably indicate a truthful or falseful value. If it fails to do so, it proceeds to treat the argument as if it were a name reference. However, it does so without bothering to check whether its value could be a legal variable name in the first place, which is extremely dangerous! Below is a trivial example of a code injection. $ . ./functions.sh $ untrusted_input='_; echo "you just got pwned"' $ yesno "$untrusted_input" you just got pwned Frankly, I think the name dereferencing is a monumental anti-feature and would urge that it be removed. However, it is beyond my purview to make such a decision. Instead, add a check to ensure that the name appears to be a valid one before calling upon eval. Here is the same test, as conducted against the new implementation. $ yesno "$untrusted_input" $ echo $? 1 $ EINFO_VERBOSE=1 $ yesno "$untrusted_input" * Invalid argument given to yesno (expected a boolean-like or a legal name) Another issue with allowing for name dereferencing is that, by its very nature, it is anethema to the use of the (non-standard) local builtin. Here is a concrete example of the prior implementation producing a bogus warning and returning the wrong exit status value entirely. $ value=yes $ yesno value * $value is not set properly $ echo $? 1 Compare and contrast to the new implementation, which works correctly. $ yesno value $ echo $? 0 This has been accomplished by eradicating the use of local. Now only the first positional parameter and _ are ever assigned to, with the latter variable being used a loop iterator. This is rather unconventional but is, nevertheless, safe. Although _ is a special variable, its value will not normally be affected by the shell until the enclosing compound command has concluded. Further, any conforming implementation of sh(1) is able to use a for loop to assign to it. Signed-off-by: Kerin Millar <kfm <AT> plushkava.net> Signed-off-by: Sam James <sam <AT> gentoo.org> functions.sh | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/functions.sh b/functions.sh index 6ad22f3..7882113 100644 --- a/functions.sh +++ b/functions.sh @@ -47,20 +47,31 @@ eoutdent() # yesno() { - [ -z "$1" ] && return 1 - - case "$1" in - [Yy][Ee][Ss]|[Tt][Rr][Uu][Ee]|[Oo][Nn]|1) return 0;; - [Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0) return 1;; - esac - - local value= - eval "value=\$${1}" - case "$value" in - [Yy][Ee][Ss]|[Tt][Rr][Uu][Ee]|[Oo][Nn]|1) return 0;; - [Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0) return 1;; - *) vewarn "\$$1 is not set properly"; return 1;; - esac + for _ in 1 2; do + case $1 in + [Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0) + return 1 + ;; + [Yy][Ee][Ss]|[Tt][Rr][Uu][Ee]|[Oo][Nn]|1) + return 0 + esac + if [ "$_" -gt 1 ]; then + ! break + else + # Using eval can be very dangerous. Check whether the + # value is a legitimate variable name before proceeding + # to treat it as one. + ( + LC_ALL=C + case $1 in + ''|_|[[:digit:]]*|*[!_[:alnum:]]*) exit 1 + esac + ) || ! break + # Treat the value as a nameref then try again. + eval "set -- \"\$$1\"" + fi + done || vewarn "Invalid argument given to yesno (expected a boolean-like or a legal name)" + return 1 } #