False positive: Command Injection

[jasonperrone]

trafficstars

Background

Brakeman version: 4.7.1 Rails version: 5.1.6 Ruby version: 2.4.2

False Positive

Full warning from Brakeman: Confidence: High Category: Command Injection Check: Execute Message: Possible command injection

Relevant code:

result, status = Open3.capture2('zgrep',"^\\[#{request_id}",log)

Why might this be a false positive?

I believe this is a false positive, even though request_id above is from params[:r][0,10] (r is a string and we take the first 10 characters of it). I think Brakeman should recognize that the system call has been tokenized and the first argument (zgrep) is the actual system command. The second argument, the request_id, should be whatever it wants. How could it cause command injection when the command is solely "zgrep"? I'm open minded to being corrected about this.

[presidentbeef]

Hi Jason,

I agree in this case it is a false positive. (Although, there are a number of seemingly benign commands which take options that allow for arbitrary code execution, so be careful...)

To be completely honest, the API for Open3.capture2 and related functions, most of which boil down to Kernel#spawn, is so complicated I simply punted on it.

I mean, look at this:

Open3.capture3([env,] cmd... [, opts])

The first argument might be a command...or it might be an environment hash! The second argument might be a command...or it might be an array of command+options...or it might just be an option...or it might be an option hash!

Anyway, I suppose we could check to see if the first argument is a string literal and go from there.

[jasonperrone]

That sounds like that would cut down on a lot of the warnings. Thanks, Justin.