From b48e5b33651420242db5c8a7f180b31c2076a085 Mon Sep 17 00:00:00 2001
From: Martijn Dekker <martijn@inlv.org>
Date: Thu, 4 Mar 2021 13:37:13 +0000
Subject: [PATCH] Fix arbitrary command execution vuln in array subscripts in
 arith

This commit fixes an arbitrary command execution vulnerability in
array subscripts used within the arithmetic subsystem.

One of the possible reproducers is:
	var='1$(echo INJECTION >&2)' ksh -c \
		'typeset -A a; ((a[$var]++)); typeset -p a'

Output before this commit:
	INJECTION
	typeset -A a=([1]=1)
The 'echo' command has been surreptitiously executed from an
external environment variable.

Output after this commit:
	typeset -A a=(['1$(echo INJECTION >&2)']=1)
The value is correctly used as an array subscript and nothing in it
is parsed or executed. This is as it should be, as ksh93 supports
arbitrary subscripts for associative arrays.

If we think about it logically, the C-style arithmetic subsystem
simply has no business messing around with shell expansions or
quoting at all, because those don't belong to it. Shell expansions
and quotes are properly resolved by the main shell language before
the arithmetic subsystem is even invoked. It is particularly
important to maintain that separation because the shell expansion
mechanism also executes command substitutions.

Yet, the arithmetic subsystem subjected array subscripts that
contain `$` (and only array subscripts -- how oddly specific) to
an additional level of expansion and quote resolution. For some
unfathomable reason, there are two lines of code doing specifically
this. The vulnerability is fixed by simply removing those.

Incredibly, variants of this vulnerability are shared by bash, mksh
and zsh. Instead of fixing it, it got listed in Bash Pitfalls!
http://mywiki.wooledge.org/BashPitfalls#y.3D.24.28.28_array.5B.24x.5D_.29.29

src/cmd/ksh93/sh/arith.c:
- scope(): Remove these two lines that implement the vulnerability.
			if(strchr(sub,'$'))
				sub = sh_mactrim(shp,sub,0);
- scope(), arith(): Remove the NV_SUBQUOTE flag from two
  nv_endsubscript() calls. That flag causes the array subscript to
  retain the current level of shell quoting. The shell quotes
  everything as in "double quotes" before invoking the arithmetic
  subsystem, and the bad sh_mactrim() call removed one level of
  quoting. Since we're no longer doing that, this flag should no
  longer be passed, or subscripts may get extra backslash escapes.

src/cmd/ksh93/include/name.h,
src/cmd/ksh93/sh/array.c:
- nv_endsubscript(): The NV_SUBQUOTE flag was only passed from
  arith.c. Since it is now unused, remove it.

src/cmd/ksh93/tests/arith.sh:
- Tweak some tests: fix typos, report wrong values.
- Add 21 tests. Most are based on reproducers contributed by
  @stephane-chazelas and @hyenias. They verify that this
  vulnerability is gone and that no quoting bugs were introduced.

Resolves: https://github.com/ksh93/ksh/issues/152
---
 NEWS                            |   5 ++
 src/cmd/ksh93/include/name.h    |   2 -
 src/cmd/ksh93/include/version.h |   2 +-
 src/cmd/ksh93/sh.1              |   2 +-
 src/cmd/ksh93/sh/arith.c        |   8 +--
 src/cmd/ksh93/sh/array.c        |   2 +-
 src/cmd/ksh93/tests/arith.sh    | 102 +++++++++++++++++++++++++-------
 7 files changed, 91 insertions(+), 32 deletions(-)

diff --git a/NEWS b/NEWS
index ffb4176d8..24d4b67f6 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,11 @@ For full details, see the git log at: https://github.com/ksh93/ksh
 
 Any uppercase BUG_* names are modernish shell bug IDs.
 
+2021-03-04:
+
+- Fixed an arbitrary command execution vulnerability that occurred when
+  parsing the subscripts of arrays within arithmetic commands and expansion.
+
 2021-03-01:
 
 - Fixed the retention of size attributes when 'readonly' or 'typeset -r'
diff --git a/src/cmd/ksh93/include/name.h b/src/cmd/ksh93/include/name.h
index f2fef4121..bd727dfb0 100644
--- a/src/cmd/ksh93/include/name.h
+++ b/src/cmd/ksh93/include/name.h
@@ -150,8 +150,6 @@ struct Ufunction
 #define	nv_funtree(n)	((n)->nvalue.rp->ptree)
 #define	funptr(n)	((n)->nvalue.bfp)
 
-#define NV_SUBQUOTE	(NV_ADD<<1)	/* used with nv_endsubscript */
-
 /* NAMNOD MACROS */
 /* ... for attributes */
 
diff --git a/src/cmd/ksh93/include/version.h b/src/cmd/ksh93/include/version.h
index 10a84130f..80397a48a 100644
--- a/src/cmd/ksh93/include/version.h
+++ b/src/cmd/ksh93/include/version.h
@@ -20,7 +20,7 @@
 
 #define SH_RELEASE_FORK	"93u+m"		/* only change if you develop a new ksh93 fork */
 #define SH_RELEASE_SVER	"1.0.0-alpha"	/* semantic version number: https://semver.org */
-#define SH_RELEASE_DATE	"2021-03-01"	/* must be in this format for $((.sh.version)) */
+#define SH_RELEASE_DATE	"2021-03-04"	/* must be in this format for $((.sh.version)) */
 #define SH_RELEASE_CPYR	"(c) 2020-2021 Contributors to ksh " SH_RELEASE_FORK
 
 /* Scripts sometimes field-split ${.sh.version}, so don't change amount of whitespace. */
diff --git a/src/cmd/ksh93/sh.1 b/src/cmd/ksh93/sh.1
index ce8e1c803..ec0e48973 100644
--- a/src/cmd/ksh93/sh.1
+++ b/src/cmd/ksh93/sh.1
@@ -1612,7 +1612,7 @@ The value is null when not processing a
 trap.
 .TP
 .B .sh.file
-The pathname of the file than contains the current command.
+The pathname of the file that contains the current command.
 .TP
 .B .sh.fun
 The name of the current function that is being executed.
diff --git a/src/cmd/ksh93/sh/arith.c b/src/cmd/ksh93/sh/arith.c
index 93de13cdd..cfdbf7471 100644
--- a/src/cmd/ksh93/sh/arith.c
+++ b/src/cmd/ksh93/sh/arith.c
@@ -161,8 +161,6 @@ static Namval_t *scope(register Namval_t *np,register struct lval *lvalue,int as
 				sfprintf(shp->strbuf,"%s%s%c",nv_name(np),sub,0);
 				sub = sfstruse(shp->strbuf);
 			}
-			if(strchr(sub,'$'))
-				sub = sh_mactrim(shp,sub,0);
 			*cp = flag;
 			if(c || hasdot)
 			{
@@ -171,9 +169,9 @@ static Namval_t *scope(register Namval_t *np,register struct lval *lvalue,int as
 			}
 #if SHOPT_FIXEDARRAY
 			ap = nv_arrayptr(np);
-			cp = nv_endsubscript(np,sub,NV_ADD|NV_SUBQUOTE|(ap&&ap->fixed?NV_FARRAY:0));
+			cp = nv_endsubscript(np,sub,NV_ADD|(ap&&ap->fixed?NV_FARRAY:0));
 #else
-			cp = nv_endsubscript(np,sub,NV_ADD|NV_SUBQUOTE);
+			cp = nv_endsubscript(np,sub,NV_ADD);
 #endif /* SHOPT_FIXEDARRAY */
 			if(*cp!='[')
 				break;
@@ -270,7 +268,7 @@ static Sfdouble_t arith(const char **ptr, struct lval *lvalue, int type, Sfdoubl
 				dot=NV_NOADD;
 				if((c = *++str) !='[')
 					continue;
-				str = nv_endsubscript((Namval_t*)0,cp=str,NV_SUBQUOTE)-1;
+				str = nv_endsubscript((Namval_t*)0,cp=str,0)-1;
 				if(sh_checkid(cp+1,(char*)0))
 					str -=2;
 			}
diff --git a/src/cmd/ksh93/sh/array.c b/src/cmd/ksh93/sh/array.c
index 777c140d4..4317bd00b 100644
--- a/src/cmd/ksh93/sh/array.c
+++ b/src/cmd/ksh93/sh/array.c
@@ -1480,7 +1480,7 @@ char *nv_endsubscript(Namval_t *np, register char *cp, int mode)
 	/* first find matching ']' */
 	while(count>0 && (c= *++cp))
 	{
-		if(c=='\\' && (!(mode&NV_SUBQUOTE) || (c=cp[1])=='[' || c==']' || c=='\\' || c=='*' || c=='@'))
+		if(c=='\\')
 		{
 			quoted=1;
 			cp++;
diff --git a/src/cmd/ksh93/tests/arith.sh b/src/cmd/ksh93/tests/arith.sh
index 9a0eae0ba..46656a728 100755
--- a/src/cmd/ksh93/tests/arith.sh
+++ b/src/cmd/ksh93/tests/arith.sh
@@ -544,18 +544,18 @@ i=0 j=1
 unset a x
 x=0
 ((a[++x]++))
-(( x==1)) || err_exit '((a[++x]++)) should only increment x once'
-(( a[1]==1))  || err_exit 'a[1] not incremented'
+((x==1)) || err_exit "((a[++x]++)) should only increment x once (expected '1', got '$x')"
+((a[1]==1)) || err_exit "a[1] not incremented (expected '1', got '${a[1]}')"
 unset a
 x=0
 ((a[x++]++))
-(( x==1)) || err_exit '((a[x++]++)) should only increment x once'
-(( a[0]==1))  || err_exit 'a[0] not incremented'
+((x==1)) || err_exit "((a[x++]++)) should only increment x once (expected '1', got '$x')"
+((a[0]==1)) || err_exit "a[0] not incremented (expected '1', got '${a[0]}')"
 unset a
 x=0
 ((a[x+=2]+=1))
-(( x==2)) || err_exit '((a[x+=2]++)) should result in x==2'
-(( a[2]==1))  || err_exit 'a[0] not 1'
+((x==2)) || err_exit "((a[x+=2]++)) should result in x==2 (expected '2', got '$x')"
+((a[2]==1)) || err_exit "a[0] not 1 (expected '1', got '${a[2]}')"
 
 unset a i
 typeset -a a
@@ -691,7 +691,9 @@ unset x
 let x=010
 [[ $x == 10 ]] || err_exit 'let treating 010 as octal'
 (set -o letoctal; let x=010; [[ $x == 8 ]]) || err_exit 'let not treating 010 as octal with letoctal on'
-(set -o posix 2>/dev/null; let x=010; [[ $x == 8 ]]) || err_exit 'let not treating 010 as octal with posix on'
+if [[ -o ?posix ]]
+then	(set -o posix; let x=010; [[ $x == 8 ]]) || err_exit 'let not treating 010 as octal with posix on'
+fi
 
 float z=0
 integer aa=2 a=1
@@ -699,37 +701,37 @@ typeset -A A
 A[a]=(typeset -A AA)
 A[a].AA[aa]=1
 (( z= A[a].AA[aa]++ ))
-(( z == 1 )) ||  err_exit "z should be 1 but is $z for associative array of
-associative array arithmetic"
-[[ ${A[a].AA[aa]} == 2 ]] || err_exit '${A[a].AA[aa]} should be 2 after ++ operation for associative array of associative array arithmetic'
+(( z == 1 )) ||  err_exit "z should be '1' but is '$z' for associative array of associative array arithmetic"
+[[ ${A[a].AA[aa]} == 2 ]] || err_exit "\${A[a].AA[aa]} should be '2' but is '${A[a].AA[aa]}'" \
+	'after ++ operation for associative array of associative array arithmetic'
 unset A[a]
 
 A[a]=(typeset -a AA)
 A[a].AA[aa]=1
 (( z += A[a].AA[aa++]++ ))
-(( z == 2 )) ||  err_exit "z should be 2 but is $z for associative array of
-index array arithmetic"
-(( aa == 3 )) || err_exit "subscript aa should be 3 but is $aa after ++"
-[[ ${A[a].AA[aa-1]} == 2 ]] || err_exit '${A[a].AA[aa]} should be 2 after ++ operation for ssociative array of index array arithmetic'
+(( z == 2 )) ||  err_exit "z should be '2' but is '$z' for associative array of index array arithmetic"
+(( aa == 3 )) || err_exit "subscript aa should be '3' but is '$aa' after ++"
+[[ ${A[a].AA[aa-1]} == 2 ]] || err_exit "\${A[a].AA[aa]} should be '2' but is '${A[a].AA[aa]}'" \
+	'after ++ operation for associative array of index array arithmetic'
 unset A
 
 typeset -a A
 A[a]=(typeset -A AA)
 A[a].AA[aa]=1
 (( z += A[a].AA[aa]++ ))
-(( z == 3 )) ||  err_exit "z should be 3 but is $z for index array of
-associative array arithmetic"
-[[ ${A[a].AA[aa]} == 2 ]] || err_exit '${A[a].AA[aa]} should be 2 after ++ operation for index array of associative array arithmetic'
+(( z == 3 )) ||  err_exit "z should be '3' but is '$z' for index array of associative array arithmetic"
+[[ ${A[a].AA[aa]} == 2 ]] || err_exit "\${A[a].AA[aa]} should be '2' but is '${A[a].AA[aa]}'" \
+	'after ++ operation for index array of associative array arithmetic'
 unset A[a]
 
 A[a]=(typeset -a AA)
 A[a].AA[aa]=1
 (( z += A[a++].AA[aa++]++ ))
-(( z == 4 )) ||  err_exit "z should be 4 but is $z for index array of
-index array arithmetic"
-[[ ${A[a-1].AA[aa-1]} == 2 ]] || err_exit '${A[a].AA[aa]} should be 2 after ++ operation for index array of index array arithmetic'
-(( aa == 4 )) || err_exit "subscript aa should be 4 but is $aa after ++"
-(( a == 2 )) || err_exit "subscript a should be 2 but is $a after ++"
+(( z == 4 )) ||  err_exit "z should be '4' but is '$z' for index array of index array arithmetic"
+[[ ${A[a-1].AA[aa-1]} == 2 ]] || err_exit "\${A[a].AA[aa]} should be '2' but is '${A[a].AA[aa]}'" \
+	'after ++ operation for index array of index array arithmetic'
+(( aa == 4 )) || err_exit "subscript aa should be '4' but is '$aa' after ++"
+(( a == 2 )) || err_exit "subscript a should be '2' but is '$a' after ++"
 unset A
 
 unset r x
@@ -774,5 +776,61 @@ v=$(printf $'%.28a\n' 64)
 # Redirections with ((...)) should not cause a syntax error
 (eval '((1)) >/dev/null') 2>/dev/null || err_exit 'redirections with ((...)) yield a syntax error'
 
+# ======
+# Arbitrary command execution vulnerability in array subscripts in arithmetic expressions
+# https://github.com/ksh93/ksh/issues/152
+
+exp='array_test_1: 1$(echo INJECTION >&2): arithmetic syntax error'
+got=$(var='1$(echo INJECTION >&2)' "$SHELL" -c 'typeset -a a; ((a[$var]++)); typeset -p a' array_test_1 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 1A: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(var='1$(echo INJECTION >&2)' "$SHELL" -c 'typeset -a a; ((a["$var"]++)); typeset -p a' array_test_1 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 1B: expected $(printf %q "$exp"), got $(printf %q "$got")"
+
+exp='typeset -A a=(['\''1$(echo INJECTION >&2)'\'']=1)'
+got=$(set +x; { var='1$(echo INJECTION >&2)'; typeset -A a; ((a[$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 2A: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='1$(echo INJECTION >&2)'; typeset -A a; ((a["$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 2B: expected $(printf %q "$exp"), got $(printf %q "$got")"
+
+exp='typeset -A a=(['\''$0`echo INJECTION >&2`'\'']=1)'
+got=$(set +x; { var='$0`echo INJECTION >&2`'; typeset -A a; ((a[$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 3A: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='$0`echo INJECTION >&2`'; typeset -A a; ((a["$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 3B: expected $(printf %q "$exp"), got $(printf %q "$got")"
+
+exp='typeset -A a=(['\''x\]+b\[$(echo INJECTION >&2)'\'']=1)'
+got=$(set +x; { var="x\]+b\[\$(echo INJECTION >&2)"; typeset -A a; ((a[$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 4A: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var="x\]+b\[\$(echo INJECTION >&2)"; typeset -A a; ((a["$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 4B: expected $(printf %q "$exp"), got $(printf %q "$got")"
+
+exp='typeset -A a=(['\''$var'\'']=1)'
+got=$(set +x; { var='$0$(echo INJECTION >&2)'; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5A: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='1`echo INJECTION >&2`'; typeset -A a; ((a["\$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5B: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var=''; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5C: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='\'; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5D: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var=']'; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5E: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='x]foo'; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5F: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var=''; typeset -A a; let 'a[$var]++'; typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5G: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='\'; typeset -A a; let 'a[$var]++'; typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5H: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var=']'; typeset -A a; let 'a[$var]++'; typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5I: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='x]foo'; typeset -A a; let 'a[$var]++'; typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5J: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var='1$(echo INJECTION >&2)'; typeset -A a; ((a["\$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5K: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var="x\]+b\[\$(echo INJECTION >&2)"; typeset -A a; ((a[\$var]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5L: expected $(printf %q "$exp"), got $(printf %q "$got")"
+got=$(set +x; { var="x\]+b\[\$(uname>&2)"; typeset -A a; ((a["\$var"]++)); typeset -p a; } 2>&1)
+[[ $got == "$exp" ]] || err_exit "Array subscript quoting test 5M: expected $(printf %q "$exp"), got $(printf %q "$got")"
+
 # ======
 exit $((Errors<125?Errors:125))