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
}
#
