Command injection false positive

[jasonperrone]

Background

Brakeman version: 6.1.2 Rails version: 7.1.3.3 Ruby version: 3.3.1

False Positive

Full warning from Brakeman: Confidence: High Category: Command Injection Check: Execute Message: Possible command injection Code: Open3.capture2("zgrep", "^\[#{RequestLog.find_by_request_id(params[:r]).request_id[0, 10]}", ("#{Rails.root.join("log")}/#{Rails.env}.log-#{(RequestLog.find_by_request_id(params[:r]).created_at + i.days).strftime("%Y%m%d")}.gz" or "#{Rails.root.join("log")}/#{Rails.env}.log-#{(RequestLog.find_by_request_id(params[:r]).created_at + i.days).strftime("%Y%m%d")}")) File: app/controllers/admin/admin_controller.rb Line: 370

Relevant code:

search_request_id = params[:r]
if search_request_id
  rl = RequestLog.find_by_request_id(search_request_id)
  request_id = rl.request_id[0,10]
  log = path to a log file
  result, status = Open3.capture2('zgrep',"^\\[#{request_id}",log)
end

Why might this be a false positive? I can't see how Brakeman is drawing a line from an ActiveRecord Object to a request parameter. The field I am passing to Open3.capture2 is a new variable that is a substring of an attribute from an ActiveRecord model that was found by using a request parameter. In other words, request_id is the substring of RequestLog.request_id. It really has nothing to do with the request parameter "r". In order to do command injection, not only would they have to pass down a command injected string, but that same string would have had to have been found in a record in my database in a table that is only ever updated by middleware, nothing from the UI. They would have had to hack my database to put their command in a RequestLog record, and then pass that same command down as a request param to this particular controller action. Plus they would need to know I am only looking at the first 10 characters of this string. Seems like a reach, no?

[presidentbeef]

Hi @jasonperrone, thanks for reporting this.

It's really just warning that there's string-building with database values being used for an argument to the command. There's no way for Brakeman to know what a RequestLog is and whether or not an attacker is able to manipulate one.

[jasonperrone]

Understood, thanks @presidentbeef