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

False Positive: command injection when escaped upstream. #1159

Closed
envygeeks opened this Issue Feb 14, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@envygeeks

envygeeks commented Feb 14, 2018

Background

Brakeman version: ~> 4.1
Rails version: ~> 5.1
Ruby version: ~> 2.5

Link to Rails application code: No

Issue

When a variable is escaped upstream to a method, Brakeman claims that there could be command injection, even though it's escaped upstream in the initializer.

False Positive

Confidence: Medium
Category: Command Injection
Check: Execute
Message: Possible command injection
Code: `dig +short -x #{Shellwords.shellescape(ip)} @#{@mdns} -p #{@port}`
File: lib/blackhole/enumerators/mdns.rb
Line: 44

Relevant code:

class NotReadyForRelease
  def initialize(one, two)
    @one = Shellwords.shellescape(one)
    @two = Shellwords.shellescape(two)
  end

  def run(ip)
    ip = Shellwords.shellescape(ip)
    `dig +short -x #{ip} @#{@one} -p #{@two}`
  end
end

Why might this be a false positive?

Because it's escaped upstream of the method.

@envygeeks

This comment has been minimized.

envygeeks commented Feb 14, 2018

It can also be triggered with

ip, mdns, port = [ip,mdns,port].map(&Shellwords.method(:shellescape))
@envygeeks

This comment has been minimized.

envygeeks commented Feb 14, 2018

Actually, no matter what I do it just flat out claims there could be injection, no matter the type of escaping I do, no matter what I do, it claims there could be injection even though everything is escaped. That's not so nice.

@presidentbeef

This comment has been minimized.

Owner

presidentbeef commented Feb 15, 2018

Hi Jordon,

I hope you don't feel like Brakeman is being "mean" for pointing this out! 😢

I agree Brakeman should avoid warning when Shellwords.shellescape and related methods are used.

@envygeeks

This comment has been minimized.

envygeeks commented Feb 19, 2018

I don't think it was being mean, just confused more than anything, I thought I was doing something wrong at first, and started digging through Ruby's source to see if there was a particular reason (as in was Shellwords not safe.)

Thanks for the quick fix!

Repository owner locked and limited conversation to collaborators May 9, 2018

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