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

Catch shell injection from -c shell commands #1396

Merged
merged 2 commits into from Sep 12, 2019

Conversation

@JacobEvelyn
Copy link
Contributor

JacobEvelyn commented Sep 11, 2019

Closes #1207

Closes #1207
Copy link
Owner

presidentbeef left a comment

Thanks @JacobEvelyn! Just a few minor comments so far.

def dash_c_shell_command?(first_arg, args)
first_arg.size <= 2 &&
KNOWN_SHELL_COMMANDS.include?(first_arg.value) &&
args[2]&.size <= 2 &&

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Sep 12, 2019

Owner

Same here - string? args[2] will do what you want.

# when the first two arguments are something like "bash -c" because then
# the third argument is effectively the command being run and might be
# a malicious executable if it comes (partially or fully) from user input.
if dash_c_shell_command?(first_arg, args)

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Sep 12, 2019

Owner

You could pass in call.second_arg instead of all the arguments.

This comment has been minimized.

Copy link
@JacobEvelyn

JacobEvelyn Sep 12, 2019

Author Contributor

Good catch! I forgot to fix that during a refactor.

@@ -21,6 +21,10 @@ class Brakeman::CheckExecute < Brakeman::BaseCheck
SHELL_ESCAPE_MODULE_METHODS = Set[:escape, :join, :shellescape, :shelljoin]
SHELL_ESCAPE_MIXIN_METHODS = Set[:shellescape, :shelljoin]

# These are common shells that are known to allow the execution of commands
# via a -c flag. See dash_c_shell_command? for more info.
KNOWN_SHELL_COMMANDS = Set.new(["sh", "bash", "ksh", "csh", "tcsh", "zsh"])

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Sep 12, 2019

Owner

You can use Set[...] here.

This comment has been minimized.

Copy link
@JacobEvelyn

JacobEvelyn Sep 12, 2019

Author Contributor

No idea you could do this; thanks!

# @return [Boolean] true iff the command given by `first_arg`, `args` is of
# invoking a new shell process via `<shell_command> -c` (like `bash -c`)
def dash_c_shell_command?(first_arg, args)
first_arg.size <= 2 &&

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Sep 12, 2019

Owner

I think what you want here is string? first_arg

@JacobEvelyn

This comment has been minimized.

Copy link
Contributor Author

JacobEvelyn commented Sep 12, 2019

Thanks for the comments, @presidentbeef ! Just pushed up a new commit to fix those issues.

@presidentbeef presidentbeef merged commit cdfcf46 into presidentbeef:master Sep 12, 2019
9 checks passed
9 checks passed
ci/circleci: default Your tests passed on CircleCI!
Details
ci/circleci: test-2-3 Your tests passed on CircleCI!
Details
ci/circleci: test-2-4 Your tests passed on CircleCI!
Details
ci/circleci: test-2-5 Your tests passed on CircleCI!
Details
ci/circleci: test-2-6 Your tests passed on CircleCI!
Details
ci/circleci: upload-coverage Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/diff-coverage 100% (98% threshold)
Details
codeclimate/total-coverage 95% (0.0% change)
Details
@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Sep 12, 2019

Awesome, thanks!

Repository owner locked and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.