Is using `eval` to define functions with text from a file evil?

Question

This is a follow-up question to eval cat inside a function.

I'm using eval to mimic import functionality from other languages (such as JavaScript). This is something I wanted to do on my local machine for a while since I've built up an overwhelming collection of script files.

The reasoning is that I now have a very large number of individual functions in separate files, and I don't want to constantly read files with source again and again every time I want to call the functionality.

It's mostly just for fun, but I don't want to shoot my foot off should I ever use it in a less casual context:

import_as() { 
  import_name="$1"
  import_fnname="${2:-"$import_name"}"

  if test -f "$1"; then
    echo "File '$1' doesn't exist."
  fi

  case "$2" in 
    *[!-a-zA-Z0-9_]* ) echo "BAD";; 
    *) eval "$2"'() { '"$(< $1.sh)"'; }' ;; 
  esac
}

Here's an example of it in use:

add.sh

#!/bin/sh
echo "$(($1 + $2))"

sub.sh

#!/bin/sh
echo "$(($1 - $2))"

example_import.sh

import_as "add" "math_add"
import_as "sub"

math_add 2 5       # Returns "7"
math_subtract 5 1  # Returns "4"

My question is whether or not this use case of eval is vulnerable to exploits after the checks I've performed, or if there is something exploitable in this script?

PS: I understand eval is considered evil, and I don't want to hear that as an answer. I want specific reasoning for this use case if you believe that in this use case of eval there could be exploits.

@CharlesDuffy I was considering a variable to hold the current `LC_CTYPE` and resetting at the end, but I thought I'd exclude it to keep the code small. Side effects are another matter! Moving the answer now :) — Nick Bull, Sep 17 '18 at 15:53
BTW, I'd also consider `[[:alpha:]]` rather than `[A-Za-z]` -- behavior still can be locale-dependent, but at least it's a bit more reliably so (compared to the wide swings in behavior you get between AaBbCc...Zz and ABC...Zabc...z locales otherwise). — Charles Duffy, Sep 17 '18 at 15:57
BTW, I can think of *one* reason to avoid this pattern: The `eval`ed functions won't know which files they came from, so using `PS4=':$BASH_SOURCE:$LINENO+'; set -x` to log execution with file and line for each command would be considerably less informative. — Charles Duffy, Sep 17 '18 at 16:01

Charles Duffy · Answer 1 · 2018-09-17T15:44:07.320

1

If you trust your arguments to import_as -- both $1 and $2, and the content of the file referenced by $1 -- the above is safe. eval is evil insofar as it permits data to be evaluated as code; if you're hardcoding those contents, they're part of the (trusted) code, not (untrusted) data.

edited Sep 17 '18 at 15:44

answered Sep 17 '18 at 15:41

Charles Duffy

280,126
43
390
441

Just for kicks, what if I don't think that the arguments `$1` and `$2` are safe, but I trust file contents as only local files could be read? i.e., can I write anything that could blow my foot off by accident and then invoke it (so not malicious intent, such as a malicious file)? :) – Nick Bull Sep 17 '18 at 15:43
Do you mean arguments *to the function from the file*, or arguments *to `import_as`*? It's the latter that are risky. – Charles Duffy Sep 17 '18 at 15:43
I mean the arguments provided to `import_as`, i.e., the function name and the file name, given that they're both checked as above in the function – Nick Bull Sep 17 '18 at 15:44
That's a locale-specific check, so it's hard to make a firm statement without reviewing every possible character collation order. If you set `LC_CTYPE=C`, so we know exactly what is in `A-Z` and `a-z` ranges, *then* I'd call it safe. – Charles Duffy Sep 17 '18 at 15:46
Haha, oh my good call! How about the first argument, the file contents reference? I presumed that would be the easiest to exploit. – Nick Bull Sep 17 '18 at 15:47
You trust all local files on your machine? (I mean, web services can often be coerced to write local files, and if they're using `/tmp` or `/uploads` or something...) – Charles Duffy Sep 17 '18 at 15:48
yes, this would not be used for a public server at all. Perhaps if I was to consider its use in a public environment I'd contain the file reference to a single folder and use `case "$1" in *[!/]* ) ... ;; *) echo "Accept" ;; esac` – Nick Bull Sep 17 '18 at 15:51
*nod* -- only importing code from a specific, trusted directory (where you know and control the write permissions) is definitely a place I would go with this. – Charles Duffy Sep 17 '18 at 15:53
1

...as soon as you write code that assumes that nobody but you can ever write *anywhere* on a box, you're opening yourself up to privilege escalations from attackers who've only gotten access to an unprivileged account like `nobody` but are trying to work their way up. – Charles Duffy Sep 17 '18 at 15:53

Nick Bull · Accepted Answer · 2018-09-17T16:14:43.947

0

After reviewing Charles Duffy's fantastic answer and commentary, this code has been evaluated to be safer:

import_as() { 
  # Charles Duffy's suggestions, including side effect reduction.
  current_lc_ctype="$LC_CTYPE" 
  LC_CTYPE=C

  import_name="$1"
  import_fnname="${2:-"$import_name"}"

  case "$import_name" in
    *[!/]*) echo "BAD - local directory only."; return ;;
    *) echo "Okay" ;;
  esac

  if test -f "$import_name"; then
    echo "File '$import_name' doesn't exist."
  fi

  case "$2" in 
    *[!-a-zA-Z0-9_]*) echo "BAD"; return;; 
    *) eval "$2"'() { '"$(< $1.sh)"'; }' ;; 
  esac

  # Change LC_CTYPE back to prior to function call
  LC_CTYPE="$current_lc_ctype"
}

edited Sep 17 '18 at 16:14

answered Sep 17 '18 at 15:56

Nick Bull

9,518
6
36
58

1

Consider declaring your `local`s to avoid modifying global variables without intent. Setting `local LC_CTYPE=C`, for that matter, may succeed in reverting the change without needing to explicitly set aside the old value. – Charles Duffy Sep 17 '18 at 15:58
@CharlesDuffy Excellent consideration! I'll consider the portability trade off, but I am quite sure I agree for my use case :) Thanks for all your help, has been an absolute lesson on security considerations – Nick Bull Sep 17 '18 at 16:01
1

Even though `local` is an extension, it's explicitly supported in dash and ash -- perhaps the most widely used otherwise-POSIX-baseline shells. Trying to ensure that your functions `unset` newly-introduced variables otherwise gets messy. – Charles Duffy Sep 17 '18 at 16:03
Thanks again Charles, you're an invaluable source of shell knowledge :) – Nick Bull Sep 17 '18 at 16:09
BTW, if you're aiming for POSIX compliance, you'll want to change `$(< $1.sh)` to `$(cat -- "$1.sh")`. – Charles Duffy Sep 17 '18 at 17:00

Is using `eval` to define functions with text from a file evil?

2 Answers2