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

Conserve memory when resolving paths, recursively handle symlinks, check methods now more verbose and granular. #18

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Commits on Feb 10, 2024

  1. Fixed all command injection vulnerabilites present in previous implem…

    …entation 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
    Lucas Faudman authored and Lucas Faudman committed Feb 10, 2024
    Configuration menu
    Copy the full SHA
    562851d View commit details
    Browse the repository at this point in the history

Commits on Feb 13, 2024

  1. Correctly handle $IFS/${IFS} in commands. check_banned_executable can…

    … use precomputed abs_path_strings for .endswith check
    Lucas Faudman authored and Lucas Faudman committed Feb 13, 2024
    Configuration menu
    Copy the full SHA
    0d9a0e2 View commit details
    Browse the repository at this point in the history

Commits on Feb 15, 2024

  1. Tests now check err.value.args[0].startswith(<Expected Exception Mess…

    …age>) not == since my execptions have slighly different message format but the check is still the exact same
    Lucas Faudman authored and Lucas Faudman committed Feb 15, 2024
    Configuration menu
    Copy the full SHA
    ffd0425 View commit details
    Browse the repository at this point in the history
  2. Convert to Pytest. Add shell expansion tests, nested shell syntax tes…

    …t and FuzzDB tests
    Lucas Faudman authored and Lucas Faudman committed Feb 15, 2024
    Configuration menu
    Copy the full SHA
    3d3dab4 View commit details
    Browse the repository at this point in the history
  3. Correctly handle all shell expansions. Correctly handled deeply neste…

    …d shell syntax with recursive_shlex_split. Compatible with Python3.8
    Lucas Faudman authored and Lucas Faudman committed Feb 15, 2024
    Configuration menu
    Copy the full SHA
    d447303 View commit details
    Browse the repository at this point in the history
  4. Convert to Pytest. Add shell expansion tests, nested shell syntax tes…

    …t and FuzzDB tests
    Lucas Faudman authored and Lucas Faudman committed Feb 15, 2024
    Configuration menu
    Copy the full SHA
    d5d9974 View commit details
    Browse the repository at this point in the history
  5. Handle all shell redirection operators

    Lucas Faudman authored and Lucas Faudman committed Feb 15, 2024
    Configuration menu
    Copy the full SHA
    73d9c3c View commit details
    Browse the repository at this point in the history

Commits on Feb 16, 2024

  1. handle arithmetic expansion of bracket paramters and nested expansions

    Lucas Faudman authored and Lucas Faudman committed Feb 16, 2024
    Configuration menu
    Copy the full SHA
    6a18137 View commit details
    Browse the repository at this point in the history

Commits on Feb 23, 2024

  1. - 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.
    Lucas Faudman authored and Lucas Faudman committed Feb 23, 2024
    Configuration menu
    Copy the full SHA
    d22c6fb View commit details
    Browse the repository at this point in the history

Commits on Feb 27, 2024

  1. Remove redundant rmtree, remove script* and add time to BANNED_COMMAN…

    …D_CHAINING_EXECUTABLES
    Lucas Faudman authored and Lucas Faudman committed Feb 27, 2024
    Configuration menu
    Copy the full SHA
    d078eb0 View commit details
    Browse the repository at this point in the history
  2. - check() now uses Popen kwargs to determine the initial env state an…

    …d 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.
    Lucas Faudman authored and Lucas Faudman committed Feb 27, 2024
    Configuration menu
    Copy the full SHA
    be243f3 View commit details
    Browse the repository at this point in the history
  3. Remove unused os.environ import left by mistake

    Lucas Faudman authored and Lucas Faudman committed Feb 27, 2024
    Configuration menu
    Copy the full SHA
    c735164 View commit details
    Browse the repository at this point in the history

Commits on Mar 5, 2024

  1. Add FuzzDB license.

    Lucas Faudman authored and Lucas Faudman committed Mar 5, 2024
    Configuration menu
    Copy the full SHA
    c774f1a View commit details
    Browse the repository at this point in the history
  2. remove unnessary list conversion

    Lucas Faudman authored and Lucas Faudman committed Mar 5, 2024
    Configuration menu
    Copy the full SHA
    ddb99ac View commit details
    Browse the repository at this point in the history

Commits on Mar 6, 2024

  1. - Optimized space complexity of command parsing and path resolution b…

    …y converting _resolve_path_in_parsed_command to a generator of Path objects that takes a single cmd_part and yields all the resolved paths for it.
    
    - Split check_* methods based on if they are checking the expanded_command, a single command part, a single path string, or single path.
    - First the command is expanded and expanded_command is checked. Then the if expanded_command passes checks it is used to create a generator of cmd_parts.
    - Checks are then preformed on each part and its paths as they are generated, so all the parts/paths do not need to be stored in memory at once.
    - Reduces the space complexity from O(n) to O(1) where n is the number of command parts or paths.
    - More loops over the BANNED_* are needed which does slightly increase the time complexity, but this trade off is worth it since:
        - This can actually reduce the overall check execution time by raising an exception sooner before fully resolving all the parts/paths.
        - The number of BANNED_* is relatively small and constant.
        - The number of command parts and more importantly paths could be very large and unpredictable.
    
    - Recursively handle symlinks to symlinks, and/or symlinks to directories which may contain symlinks.
    - _path_is_executable now will not return True for dirs since dirs can have the x bit but they are not executables.
    
    - max_resolved_paths, and rglob_dirs kwargs can be used to control this behavior.
    - SecurityException raised if the number of resolved paths exceeds max_resolved_paths.
    - updated test_resolve_paths_in_parsed_command and added test_max_resolved_paths to test this behavior.
    - Underlying check implmentation logic is unchanged.
    Lucas Faudman authored and Lucas Faudman committed Mar 6, 2024
    Configuration menu
    Copy the full SHA
    de99993 View commit details
    Browse the repository at this point in the history

Commits on Mar 7, 2024

  1. cleanup comments

    Lucas Faudman authored and Lucas Faudman committed Mar 7, 2024
    Configuration menu
    Copy the full SHA
    8c12b66 View commit details
    Browse the repository at this point in the history
  2. Improve readability. (Underlying check logic unchanged)

    Lucas Faudman authored and Lucas Faudman committed Mar 7, 2024
    Configuration menu
    Copy the full SHA
    a8921a6 View commit details
    Browse the repository at this point in the history
  3. Same check logic but more verbose readable code.

    Split BANNED_COMMAND_CHAINING_ARGUMENTS from BANNED_COMMAND_CHAINING_EXECUTABLES.
    Add -ok -okdir system( (for awk and similar) to BANNED_COMMAND_CHAINING_ARGUMENTS
    Refactor check_path_is_chaining_executable and check_path_is_shell to path checks.
    Lucas Faudman authored and Lucas Faudman committed Mar 7, 2024
    Configuration menu
    Copy the full SHA
    21b9ded View commit details
    Browse the repository at this point in the history

Commits on Mar 12, 2024

  1. - Generators used when resolving paths to avoid storing paths all in …

    …memory at once.
    
    - Checks performed on paths as they are generated to catch banned paths as early as possible.
    - _recursive_resolve_symlinks handles symlinks to symlinks and symlinks to directories which may contain symlinks
    - max_resolved_paths kwarg can be used to limit the number of paths resolved to prevent the case where a large directory is injected into a command causing long rglob run-times needed to accurately preform checks which could allow the injection to be used as a DDoS mechanism.
    - rglobdirs kwarg can be used to control the behavior of whether files in directories are considered when preforming checks. This is useful to reduce check run-time when a user knows the command they are using does not need to consider files in directories since it does not have recursive capabilities.
    Lucas Faudman authored and Lucas Faudman committed Mar 12, 2024
    Configuration menu
    Copy the full SHA
    743e313 View commit details
    Browse the repository at this point in the history

Commits on Mar 13, 2024

  1. Fix sonarcloud Intentionality & Consistency issues from last PR

    https://sonarcloud.io/project/issues?open=AY4rNwVcynvJrFFtuQvF&id=pixee_python-security
    - Remove or correct this useless self-assignment.
    - Use concise character class syntax '\w' instead of '[a-zA-Z0-9_]'.
    - Add replacement fields or use a normal string instead of an f-string.
    - Replace the unused local variable expanded_command with _.
    - Rename this local variable Popen_env to match the regular expression ^[_a-z][a-z0-9_]*$.
    Lucas Faudman authored and Lucas Faudman committed Mar 13, 2024
    Configuration menu
    Copy the full SHA
    939293e View commit details
    Browse the repository at this point in the history