Skip to content

Commit

Permalink
Multiple Injection Restriction Bypasses Fixed: Directory traversal, g…
Browse files Browse the repository at this point in the history
…lobbing, chaining commands via executables, nested Shell syntax, shell expansions (#11)

* Fixed all command injection vulnerabilites present in previous implementation by properly handling globbing, chained commands via other binaries, directory traversal and missing of banned binaries accessed in unconventional ways. Added checking for owner/group and uncommon path types that should not be in args. Added typeing

* Correctly handle $IFS/${IFS} in commands. check_banned_executable can use precomputed abs_path_strings for .endswith check

* Tests now check err.value.args[0].startswith(<Expected Exception Message>) not == since my execptions have slighly different message format but the check is still the exact same

* Convert to Pytest. Add shell expansion tests, nested shell syntax test and FuzzDB tests

* Correctly handle all shell expansions. Correctly handled deeply nested shell syntax with recursive_shlex_split. Compatible with Python3.8

* Convert to Pytest. Add shell expansion tests, nested shell syntax test and FuzzDB tests

* Handle all shell redirection operators

* handle arithmetic expansion of bracket paramters and nested expansions

* - Complete Shell param/brace/sequence expansion for all ALLOWED_SHELL_EXPANSION_OPERATORS.
- Handle arithmetic expansion of parameters using -+ and/or nesting and shell variables.
- Track env vars set/modified by expansion and use them in the final command.
- Use expanuser to handle ~ and ~user tilde expansion when resolving paths.
- Correctly hand alphanumeric sequences expansions
- Block process substitution via input redirection.
- More tests, refactoring and comments.

My _shell_expand implementation is still in progress but is neccessary since there are no other viable solutions in Python.
The best I have seen is https://github.com/kojiromike/parameter-expansion
but it cannot be used in a security context because they say:
"All the standard shell expansions are supported, including some level of nested expansion, as long as this is not too complex or ambiguous."
and ambigous cases are exactly what needs to be handled.

* Remove redundant rmtree, remove script* and add time to BANNED_COMMAND_CHAINING_EXECUTABLES

* - check() now uses Popen kwargs to determine the initial env state and if shell expansion is needed.
- _shell expand is only used when shell=True or the executable is a shell. (list of shells derived from https://www.in-ulm.de/~mascheck/various/shells/) but reading /etc/shells could be used to get a more accurate/concise list on a per-system basis.
- The executable Popen kwarg is now handled correctly and takes precedence over the shell kwarg in determining if shell expansion is needed. See subprocess.py 1593-1596: When an executable is given and shell is True, the shell executable is replaced with the given executable.
- the initial venv state is a copy of the env Popen kwarg to not modify the original Popen kwargs.
- _resolve_executable_path now takes venv into account and uses the PATH env variable to find the executable if env is set in the Popen kwargs since this is how the subprocess module behaves.
- test_popen_kwargs added to test the Popen kwargs and the initial env state. Other tests adjusted accordingly and refactored EXCEPTIONS outside class so they can be used as pytest params.

* Remove unused os.environ import left by mistake

* Add FuzzDB license.

* remove unnessary list conversion

---------

Co-authored-by: Lucas Faudman <[email protected]>
  • Loading branch information
LucasFaudman and Lucas Faudman authored Mar 11, 2024
1 parent 4f27888 commit 780b236
Show file tree
Hide file tree
Showing 6 changed files with 1,954 additions and 42 deletions.
Loading

0 comments on commit 780b236

Please sign in to comment.