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

Support simple, derived whitelist constants? #1213

Closed
JacobEvelyn opened this Issue May 21, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@JacobEvelyn
Contributor

JacobEvelyn commented May 21, 2018

Background

Brakeman version: 4.3.0
Rails version: 5.1.6
Ruby version: 2.5.1

Issue

We've got some code that produces somewhat complex SQL queries, and Brakeman produces a SQL injection warning. For clarity, I'll demonstrate the issue with a (very contrived) command injection example though:

COMMANDS = { foo: "echo", bar: "cat" }
MORE_COMMANDS = { foo: "touch" }

def safe(arg)
  command = if Date.today.tuesday? # Some condition.
    COMMANDS[arg]
  else
    MORE_COMMANDS[arg]
  end

  `#{command} file1.txt`
end

We're trying to get Brakeman to stop warning us about this, and it's a bit tricky to use sanitization here given the SQL we're producing, so we were thinking of going for a whitelist approach, like so:

COMMANDS = { foo: "echo", bar: "cat" }
MORE_COMMANDS = { foo: "touch" }
WHITELIST = COMMANDS.values + MORE_COMMANDS.values

def safe(arg)
  command = if Date.today.tuesday?
    COMMANDS[arg]
  else
    MORE_COMMANDS[arg]
  end

  raise "STOP!" unless WHITELIST.include?(command)

  `#{command} file1.txt`
end

However, Brakeman doesn't seem to be able to understand whitelists unless they're literals rather than anything derived. It might be that it's just too difficult to support this, but I wonder if for some narrow defined cases (like expanding keys and values on constant Hash literals into array literals) it could be supported. I don't know what sort of maintenance burden that would be, however.

Thoughts?

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented May 22, 2018

You have me really curious about your application with all these command line calls 😆

Pulling values or keys from hashes should be doable without much trouble.

Edit: upon closer reading I see this is just an example...

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Jun 2, 2018

I fixed the top example...not sure if it addresses your specific issue though.

Repository owner locked and limited conversation to collaborators Jul 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.