Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented strict mode to prevent insecure access to external variables #64

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
diagnostic.partial
diagnostic.test
tests/*.diff
tests/*.actual
spec/
spec-runner/
node_modules/
Expand Down
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,18 @@ There are more scripts available in the [demos directory](demo/) that could help
There are additional features that the program supports. Try using `mo --help` to see what is available.


Security
-----------

Because `mo` uses environment variables that could be unsafe in the event that `mo` is invoked programmatically with user-input, `mo` provides `--strict` mode when used with `--source=`. `mo` will attempt to track variables that are created by each `--source=`'ed environment file, and will disallow access to external variables.

To use this, `--strict` must be used with `--source=`:

```bash
./mo --strict --source=.env my-template.mustache
```


Concessions
-----------

Expand Down
55 changes: 54 additions & 1 deletion mo
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,15 @@
# treated as an empty value for the purposes of conditionals.
# MO_ORIGINAL_COMMAND - Used to find the `mo` program in order to generate a
# help message.
# MO_STRICT_MODE - Used to limit access to variables outside of sourced files
#
# Returns nothing.

MO_HAS_SOURCED=false
MO_STRICT_SWITCH=false
MO_STRICT_MODE=false
MO_SOURCED_VARS=()

mo() (
# This function executes in a subshell so IFS is reset.
# Namespace this variable so we don't conflict with desired values.
Expand All @@ -119,6 +126,10 @@ mo() (
exit 0
;;

--strict)
MO_STRICT_SWITCH=true
;;

--allow-function-arguments)
# shellcheck disable=SC2030
MO_ALLOW_FUNCTION_ARGUMENTS=true
Expand Down Expand Up @@ -147,8 +158,21 @@ mo() (
fi

if [[ -f "$f2source" ]]; then
MO_HAS_SOURCED=true

local BEFORE_VARS AFTER_VARS ALL_VARS
BEFORE_VARS=(`cat <(set -o posix ; set) | cut -d'=' -f1`)

# shellcheck disable=SC1090
. "$f2source"
AFTER_VARS=(`cat <(set -o posix ; set) | cut -d'=' -f1`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like this type of approach, but would prefer to have a more clean environment. Also, mo was made to avoid calling other tools like sort, uniq, cut, and tr. What do you think of something like this? The meat of this (the huge eval) could be pulled out into a function making it easier for devs to understand. Then simply call it from here to list the variables that the script exports.

while read -r line; do
    if [[ ! " ${MO_SOURCED_VARS[@]} " =~ " $line " ]]; then
        MO_SOURCED_VARS+=("$line")
    fi
done < <(
    # Need to pass the name of the file into this but can't use a variable
    eval "
    # Remove all variables that are not read only
    while read -r line; do
        line=\${line#declare * }
        line=\${line%%=*}
        unset \"\$line\" &> /dev/null
    done < <(declare -p)
    unset line

    # Load variables
    . \"$f2source\"

    # Write out all readonly variables
    while read -r line; do
        line=\${line#declare * }
        line=\${line%%=*}
        unset \"\$line\" &> /dev/null
        [[ ! -v \"\$line\" ]] && [[ \"\$line\" != \"BASH_ARGV\" ]] && echo \"\$line\"
    done < <(declare -p)
    "
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Honestly, I meant to revisit this afterwards to remove those external tools. I'm glad you pointed this out. I actually avoided many solutions with grep and others, but I guess I never got around to cleaning the proof-of-concept up before pushing this up haha.

I'll play with this and see what I can do!


ALL_VARS=( ${BEFORE_VARS[@]} ${AFTER_VARS[@]} )
MO_SOURCED_VARS=( \
${MO_SOURCED_VARS[@]} \
$(echo "${ALL_VARS[@]}" | tr ' ' '\n' | sort | uniq -u)\
)
unset BEFORE_VARS AFTER_VARS ALL_VARS
else
echo "No such file: $f2source" >&2
exit 1
Expand All @@ -169,8 +193,17 @@ mo() (
done
fi

# Allow turning off Strict Mode
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems to be the opposite of what you want. Perhaps change this to

Suggested change
# Allow turning off Strict Mode
# Restrict Strict Mode to only operate with sourced data

if [[ $MO_HAS_SOURCED == true ]] && [[ $MO_STRICT_SWITCH == true ]]; then
MO_STRICT_MODE=true
fi

moGetContent moContent "${files[@]}" || return 1
moParse "$moContent" "" true

for v in ${MO_SOURCED_VARS[@]}; do
unset -v $v > /dev/null 2>&1
done
)


Expand Down Expand Up @@ -908,6 +941,7 @@ moShow() {
fi
fi
else
moTestLegal "${moNameParts[0]}" || return 1
# Further subindexes are disallowed
eval "echo -n \"\${${moNameParts[0]}[${moNameParts[1]%%.*}]}\""
fi
Expand Down Expand Up @@ -989,6 +1023,18 @@ moStandaloneDenied() {
}


# Internal: Determines if a variable name is outside of the scope of the template
#
# Returns 0 if variable is safe, Returns 1 if variable is illegal
moTestLegal() {
[[ $MO_STRICT_MODE != true ]] && return 0
[[ " ${MO_SOURCED_VARS[*]} " =~ " ${1%.*} " ]] && return 0

echo "Illegal variable access $1" >&2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a minor change

Suggested change
echo "Illegal variable access $1" >&2
echo "Illegal access to variable: $1" >&2

return 1
}


# Internal: Determines if the named thing is a function or if it is a
# non-empty environment variable. When MO_FALSE_IS_EMPTY is set to a
# non-empty value, then "false" is also treated is an empty value.
Expand All @@ -1004,6 +1050,9 @@ moStandaloneDenied() {
# Returns 0 if the name is not empty, 1 otherwise. When MO_FALSE_IS_EMPTY
# is set, this returns 1 if the name is "false".
moTest() {
# Don't allow access to outside names
moTestLegal "$1" || return 1

# Test for functions
moIsFunction "$1" && return 0

Expand All @@ -1030,7 +1079,11 @@ moTest() {
#
# Returns true (0) if the variable is set, 1 if the variable is unset.
moTestVarSet() {
[[ "${!1-a}" == "${!1-b}" ]]
# Don't allow access to outside names
moTestLegal "$1" || return 1

[[ "${!1-a}" == "${!1-b}" ]] && return 0
return 1
}


Expand Down
4 changes: 4 additions & 0 deletions run-tests
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ for TEST in tests/*.expected; do
. "${BASE}.env"
echo "Do not read this input" | mo "${BASE}.template"
fi
) | (
cat > "${BASE}.actual";
cat "${BASE}.actual"
) | diff -U5 - "${TEST}" > "${BASE}.diff"

statusCode=$?
Expand All @@ -34,6 +37,7 @@ for TEST in tests/*.expected; do
echo "ok"
PASS=$(( PASS + 1 ))
rm "${BASE}.diff"
rm "${BASE}.actual"
fi
done

Expand Down
1 change: 1 addition & 0 deletions tests/source.expected
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
Purposely Unsafe for Backwards Compatibility
value
* 1
* 2
Expand Down
1 change: 1 addition & 0 deletions tests/source.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

cd "${0%/*}" || exit 1
cat <<EOF | ../mo --source=source.vars
{{#BASH}}Purposely Unsafe for Backwards Compatibility{{/BASH}}
{{VAR}}
{{#ARR}}
* {{.}}
Expand Down
2 changes: 2 additions & 0 deletions tests/strict-source-multiple-1.vars
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export A=from1
export B=from1
3 changes: 3 additions & 0 deletions tests/strict-source-multiple-2.vars
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export B=from2
export C=from2
declare -a -x D=(from2)
5 changes: 5 additions & 0 deletions tests/strict-source-multiple.expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
A: from1
B: from2
C: from2
D: from2
D: from2
32 changes: 32 additions & 0 deletions tests/strict-source-multiple.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/usr/bin/env bash

cd "${0%/*}" || exit 1

_CTRL_Z_=$'\cZ'

{
IFS=$'\n'"${_CTRL_Z_}" read -r -d "${_CTRL_Z_}" STDERR;
IFS=$'\n'"${_CTRL_Z_}" read -r -d "${_CTRL_Z_}" STDOUT;
} <<EOF
$((printf "${_CTRL_Z_}%s${_CTRL_Z_}%d${_CTRL_Z_}" "$(\
cat <<EOMO | ../mo --strict --source=strict-source-multiple-1.vars --source=strict-source-multiple-2.vars
{{#BASH_VERSINFO}}Illegal{{/BASH_VERSINFO}}
{{BASH_VERSINFO.0}}
A: {{A}}
B: {{B}}
C: {{C}}
D: {{D.0}}
D: {{#D}}{{.}}{{/D}}
EOMO
)" "${?}" 1>&2) 2>&1)
EOF

echo "${STDOUT[@]}"

expectedErr="Illegal variable access BASH_VERSINFO"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
expectedErr="Illegal variable access BASH_VERSINFO"
expectedErr="Illegal access to variable: BASH_VERSINFO"

if [[ ! "$STDERR" =~ "$expectedErr" ]]; then
echo "STDERR should have contained an illegal variable access message:"
echo "$STDERR"
fi

exit 0