Eval for filenames, evil?

Question

I have the following script for creating projects:

clear
version="0.0.2"
echo "Project creator"
echo "---------------"
# ask user to specify a directory (could start with spelled-out home directory)
directoryName=""
# Input must meet the following criteria: 
# 1.) No spaces at all in the directory path
# 2.) some characters must be entered
while [[ ${#directoryName} -eq 0 || $directoryName == *" "* ]]
        do
                echo -e "\nEnter the full directory of the project (no spaces):"
                read directoryName
        done
echo "You entered: $directoryName"
# uate directoryName so that the rest of the code can understand it
# create directory if it does not exist
eval mkdir "$directoryName"
# if successful
if [ $? -eq 0 ]
then
        # call executable in /home/miwarren/CPPFileCreator to create a main.cpp file to put in the folder
        $HOME/CPPFileCreator/a.out $directoryName
        # copy a run.sh file into the folder, too...
        eval cp $HOME/CPPFileCreator/run.sh $directoryName/run.sh
        # you might want to put a fileCreator.sh in the folder as well
        eval cp $HOME/fileCreator.sh $directoryName/fileCreator.sh
fi

I make sure to strip out any spaces (assumed that every unsafe string has at least a space in it, because injection-style attack). I want users to not have to spell out path parts when there exist variables for it (for example, $HOME for the home directory).

Am I still good to use eval here, and if not, what to do instead?

What could possibly go right with eval? Try this: directoryName=foo;ls, then eval mkdir "$directoryName". Then consider the effects of replacing ls with a script that does something like rm -rf /. :) — Satō Katsura, Sep 03 '16 at 05:39
mkdir -p "$directoryName"? for d in "$directoryName"; do mkdir -p "$d"; done? — Satō Katsura, Sep 03 '16 at 05:41
If you took the directory name as a parameter instead of trying to read it from standard input then the user could rely on all their shell expansions to easily specify the directory. — CB Bailey, Sep 03 '16 at 09:37
Why do you care whether the directory name contains spaces? If you quoted command parameters correctly and removed all the eval, it shouldn't matter to this script. — CB Bailey, Sep 03 '16 at 09:38

John1024 · Accepted Answer · 2016-09-03T08:05:55.210

"I want users to not have to spell out path parts when there exist variables for it (for example, $HOME for the home directory)."

That can be done without eval:

$ s='$HOME/.config'
$ s="${s//\$HOME/$HOME}"
$ echo "$s"
/home/john1024/.config

This has some limitations. For one, if both HOMES and HOME are names of variables that you want to substitute, then, to avoid false matches, HOMES must be substituted before HOME.

Applying substitutions for all exported variables

Using bash:

while IFS== read -r name val
do
   s="${s//\$$name/$val}"
done < <(printenv)

For example:

$ export A=alpha; export B=beta
$ s='$HOME/$A/$B'
$ while IFS== read -r name val; do s="${s//\$$name/$val}"; done < <(printenv)
$ echo "$s"
/home/john1024/alpha/beta

Because this approach doesn't sort the variable names by length, it has the variable overlap issue mentioned above.

We can fix that by sorting the variable names according to length:

while IFS== read -r n name val
do
   s="${s//\$$name/$val}"
done < <(printenv | awk '/^[^ \t]/{key=$0; sub(/=.*/,"",key); printf "%s=%s\n",length(key),$0}' | sort -rnt=)

If the user enters a variable name that does not exist but the initial characters match some shorter name, the shorter name will be substituted. If this matters, we can avoid it by requiring the user to use brace-notation for this variables with this code:

while IFS== read -r n name val
do
   s="${s//\$\{$name\}/$val}"
done < <(printenv | awk '/^[^ \t]/{key=$0; sub(/=.*/,"",key); printf "%s=%s\n",length(key),$0}' | sort -rnt=)

As an example:

$ s='${HOME}/${A}/${B}'
$ while IFS== read -r n name val; do s="${s//\$\{$name\}/$val}"; done < <(printenv | awk '/^[^ \t]/{key=$0; sub(/=.*/,"",key); printf "%s=%s\n",length(key),$0}' | sort -rnt=)
$ echo "$s"
/home/john1024/alpha/beta

Can this be extended to any variable name they might think of ? — Mike Warren, Sep 03 '16 at 06:59
@MikeWarren I could be wrong but I think you would need to write a new parameter expansion for each variable. s="${s//\$HOME/$HOME}" is a substitution parameter expansion. So anytime the variable a contains the substring $HOME then replace it with the expanded value of $HOME. — the_velour_fog, Sep 03 '16 at 07:06
I suspect that is why John1024 may have used the variable name s. So that when it gets used in the parameter expansion i.e. ${s//PATTERN_TO_FIND/REPLACE_STRING} it causes you to say "hey that looks like a common form of a substitution expression" when you see it in amongst the rest of the script — the_velour_fog, Sep 03 '16 at 07:10
@MikeWarren Yes, I just added code to do the substitution this way for all the shell variables that printenv shows (which is all the exported variables). — John1024, Sep 03 '16 at 07:34

score 3 · Answer 2 · edited Apr 13 '17 at 12:36

Using eval here makes no sense whatsoever. Before you decide how to fix your script, think carefully about your requirements and write them down. Reading your question, it's clear that you don't have a clear understanding of what you want to do, and you aren't going to solve this problem until you figure out what you expect from a solution.

assumed that every unsafe string has at least a space in it

No, not even close. Your script allows the user to execute arbitrary code, since it allows the user to type any string not containing spaces and newlines. The user could use tabs and semicolons instead. Even without any whitespace, the user could type command substitutions:

`touch /tmp/this_is_executed`somefile
$(touch /tmp/this_is_executed)somefile

Is this a problem? It depends on what you're trying to do. Is the input coming from the user who runs your script from a normal account? If so, there is no security problem, but it doesn't make sense for the script to read input from standard input: it should take a command line argument instead, and then the command line shell would do the variable expansions that you want. If your script runs with elevated privileges or takes input over the network, then eval is most definitely evil, and you should not use it until you understand the implications. eval takes a string as argument (if it has multiple arguments, they're joined together with spaces in between) and executes it as shell code. If you didn't want to execute shell code, eval is the wrong tool.

If the script is taking unprivileged input, then it's the wrong place to perform variable expansion. Do that in the interface that feeds input to your script, running with the privileges of the user (e.g. use Javascript if the input is coming from a web browser). In your script, you'll need to check that the specified file names are in a directory that the user is supposed to have access to. Be careful about .. and symbolic links that might allow escaping a directory tree, and about leading - that can be parsed as options instead of file names.

Eval for filenames, evil?

2 Answers2

Applying substitutions for all exported variables