[gentoo-commits] proj/gentoo-functions:master commit in: /

["Sam James" <sam@gentoo.org>]

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