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

Conversation

Swivelgames
Copy link
Contributor

@Swivelgames Swivelgames commented Apr 5, 2023

Implements #52

Introduces --strict to prevent access to variables that weren't defined in sourced files

This only works with --source. It's a no-op if it's set without --source.

Tested using fidian/multishell-small:

# run-script-againt-shells bash-3 ./run-tests
bash-3.1.23: pass
bash-3.2.57: pass

# run-script-againt-shells bash-4 ./run-tests
bash-4.0.44: pass
bash-4.1.17: pass
bash-4.2.53: pass
bash-4.3.48: pass
bash-4.4.23: pass

# run-script-againt-shells bash-5 ./run-tests
bash-5.0.18: pass
bash-5.1.8: pass
Minor addition

In addition to these changes, I also introduced a tests/*.actual in addition to the tests/*.diff for debugging tests.

@Swivelgames
Copy link
Contributor Author

Re-ran the tests after the latest updates:

# run-script-againt-shells bash-3 ./run-tests
bash-3.1.23: pass
bash-3.2.57: pass

# run-script-againt-shells bash-4 ./run-tests
bash-4.0.44: pass
bash-4.1.17: pass
bash-4.2.53: pass
bash-4.3.48: pass
bash-4.4.23: pass

# run-script-againt-shells bash-5 ./run-tests
bash-5.0.18: pass
bash-5.1.8: pass

mo
@@ -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

mo
[[ $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


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"

mo
# 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants