Search code examples
ruby-on-railscode-injectionbrakeman

How to deal with the Command Injection in rails app reported by Brakeman


I have the following code on some library on my project, which is executed on a Sideqik Worker:

def self.generate_pdf(report)
  file_name = report['r_file'].gsub('.ric', '')
  path = "#{Rails.root}/report_files"
  java_cmd = "./fileprint_linux.sh"
  if %w(development test).include?(Rails.env)
      command = "cd #{path}; sh #{java_cmd} silent #{report.r_file.path}"
  else
    temp = Tempfile.new("#{file_name}.tmp")
    File.open(temp.path, 'wb') { |f| f.write(open(report.r_file.url).read) }
    command = "cd #{path}; sh #{java_cmd} silent #{temp.path}"
  end

  stdin, stdout, stderr = Open3.popen3(command.shellescape)

  if stderr.read.blank?
    .......
  end
end

And when I run Brakeman (3.2.1) on the project I get the following security warning:

Possible command injection near line 21: Open3.popen3(("cd #{"#{Rails.root}/report_files"}; sh #{"./fileprint_linux.sh"} silent #{report.r_file.path}" or "cd #{"#{Rails.root}/report_files"}; sh #{"./fileprint_linux.sh"} silent #{Tempfile.new("#{report["r_file"].gsub(".ric", "")}.tmp").path}"))

And it highligths this part, which I guess causes the warning:

report['r_file'].gsub('.ric', '')

The warning also links to this page for more information about the warning but I didn't find a way of dealing with it: http://brakemanscanner.org/docs/warning_types/command_injection/

I've tried to find a solution to this looking at other post and pages but with no luck, hence this post.

How should I deal with this situation to fix this potential vulnerability reported by Brakeman?

Thanks in advance!


Solution

  • All credit given to @Gumbo that suggested the use of shellescape on each parameter, the way to fix the warning explained above is to use shellescape (Shellwords::shellescape) on each argument:

    "cd #{path.shellescape}; sh #{java_cmd.shellescape} silent #{report.r_file.path.shellescape}" 
    

    And then when calling the popen3 command, we pass each parameter separately using the *%W operator to easily convert the command string into an array:

    stdin, stdout, stderr = Open3.popen3(*%W(command))
    

    (using %w instead of *%W also works in this case)

    The combination of both changes solves the Brakeman warning mention before. Using just one of them didn't work for me.