How to display the command and avoid code injection

Question

I'm playing with wget, and I basically do a script that download a page, grep some information on this page, and then wget another page depending on these informations. So basically, my code looks like

defautltCommand="wget -v"
formValue=$(cat myfileA.html | get_field_value )
command="${defaultCommand} --post-data=\"myfield=${formValue}\""
echo "Command 3: ${command}"
echo "${command}" | bash

However, this solution has several problems, the main one is that if the html file has some evil values (like " google.com; <evil command>; ls "), then it could do code injection. And because my script should be run as root as a NetworkManager script... I'd like to be sure that it's not possible to inject code. However, I like the idea to write the command that will be used, it's quite useful to debug when an error occurs.

Do you have a nice way to be sure that I can't get any code injection? Or maybe you have a still better way to proceed? The first idea I got was to replace all quote like this: ... | sed 's/"/\\/g"', but I'm not sure that I capture all the possible ways to inject code.

Thank you!

It looks to me like you are directly injecting code to bash. — Jeff Schaller, Nov 17 '17 at 17:22
@A.B I can't, just because the value of the argument are taken from the file, and I can't list all of them, because they looks like "A5h4f121SDEfdsZPfkshf457dsFJqsd"... — tobiasBora, Nov 17 '17 at 18:48
@JeffSchaller Well I'm injecting a code that I want into bash (basically a wget command), but I would like to avoid a bad user to inject also bad requests, like "rm -rf /"... — tobiasBora, Nov 17 '17 at 18:50

score 5 · Answer 1 · edited Nov 18 '17 at 02:16

Piping code into a shell is almost always wrong. Here, you're already running code in a shell, there's absolutely no reason to run another shell.

Furthermore, don't build commands by putting their parts into a string. A command is a list of strings; if you try to stuff it into a string, you lose the distinction between spaces that separate arguments and spaces inside arguments, and you'll run into additional issues when you try to split the string to run it, some of which have security implications. But you don't need that here anyway. You're overcomplicating things.

form_value=$(cat myfileA.html | get_field_value )
wget -v --post-data="$form_value"

If you want the wget -v part to be variable, but under the control of your script, and you know that the parts won't contain any whitespace or any of \[*?, then you can put that part into a variable.

wget='wget'
if [ -n "$verbose" ]; then
  wget="$wget -v"
fi
form_value=$(cat myfileA.html | get_field_value )
$wget --post-data="$form_value"

If your script runs under ksh or bash or zsh, but not if it runs under plain sh, you can put the command in an array.

#!/bin/ksh
…
download=(wget -v --post-data="$form_value")
…
"${download[@]}"

When echoing the content, beware of control characters that may cause your terminal to perform actions or may misrepresent what is really printed.

echo "$wget --post-data=$form_value" | tr -c '[:print:]' '?'

I like your explanation, but the "echo" thing makes me duplicate my code I guess no? — tobiasBora, Nov 18 '17 at 17:38

score 3 · Accepted Answer · answered Nov 18 '17 at 09:00

If you want to log a command, you can use the standard xtrace mechanism:

log_cmd() {
  local - # assuming ash-based shells or bash-4.4+
          # see set -o localoptions in zsh or use
          # function log_cmd { syntax in AT&T ksh
          # or replace {...;} with (...) POSIXly (but
          # spawns a subshell)

  typeset PS4="$1"; shift
  set -o xtrace

  "$@"
}

# here assuming a shell with arrays like ksh93, zsh, bash, yash
defautltCommand=(wget -v)
formValue=$(<myfileA.html get_field_value)
command=("${defaultCommand[@]}" "--post-data=myfield=$formValue")
log_cmd 'Command 3: ' "${command[@]}"

That way, you're never invoking a shell's interpreter on arbitrary data.

Sasha Che · Answer 3 · 2017-11-18T07:33:48.857

If your argument looks like "A5h4f121SDEfdsZPfkshf457dsFJqsd" you could remove all non-alphanumerics using sed.

Something like this:

#!/bin/bash

# sample function just not to think how to process your form
get_field_value () {
   echo "ls -la; ps -ef;\"find . -iname test.txt\";  \`date +%Y%m%d\`"
}

# out of the bad command will be:
get_field_value
# >> ls -la; ps -ef;"find . -iname test.txt";  `date +%Y%m%d`

# your script begins here

#sample url
url="www.google.com"

# suggesting you have your file near your script
formValue=$(cat myfileA.html | get_field_value )

# remove all non-alphanumerics using sed
# formValue=$(echo "$formValue" | sed -e "s/[^a-zA-Z0-9]//g")
formValue=$(echo "$formValue" | sed -e "s/[^[:alnum:]]//g")
echo "Command 3: wget -v --post-data=\"$formValue\""
# >> Command 3: wget -v --post-data="lslapseffindinametesttxtdateYmd"


# run your command
wget -v --post-data="myfield=${formValue}" "$url"

in this case all non-alphanumerics will be removed and the code will not be able to hurt

How to display the command and avoid code injection

3 Answers3