Namespaced classes that are not fully qualified can cause difference in false positives/negatives

[doliveirakn]

Background

Brakeman version: 4.5.1
Rails version: 5.0.7.2
Ruby version: 2.4.3

Link to Rails application code:

Issue

What problem are you seeing?

If we reference a namespaced ActiveRecord module without it being fully qualified constant, Brakeman will fail to identify it as a model and may report false positives or negatives.

Code:

# app/models/document.rb
class Document < ApplicationRecord
  attr_accessor :owner
end

# app/models/namespace/task.rb
module Namespace
  class Task < ApplicationRecord
    attr_accessor :owner
  end
end

# app/controllers/documents_controller.rb
class DocumentsController < ApplicationController
  def index
    redirect_to Document.new(params.permit(:owner)).owner
  end

  def show
    redirect_to Document.find(params[:id])
  end
end

# app/controllers/namespace/tasks_controller.rb
module Namespace
  class TasksController < ApplicationController
    def index
      redirect_to Task.new(params.permit(:owner)).owner
    end

    def show
      redirect_to Task.find(params[:id])
    end
  end
end

Running brakeman on this will result in 2 security warnings

== Warnings ==

Confidence: High
Category: Redirect
Check: Redirect
Message: Possible unprotected redirect
Code: redirect_to(Document.new(params.permit(:owner)).owner)
File: app/controllers/documents_controller.rb
Line: 4

Confidence: Weak
Category: Redirect
Check: Redirect
Message: Possible unprotected redirect
Code: redirect_to(Task.find(params[:id]))
File: app/controllers/namespace/tasks_controller.rb
Line: 9

It appears that brakeman is able to determine that Document is an ActiveRecord model but it cannot determine that Task is a model. This causes the checks to differ in behaviour and may report false negatives (Namespace::TasksController#index should have the same warning as the document one), or false positives (Namespace::TasksController#show should not have this warning)

It was not clear to me how many checks would be affected by this. It is clear that the Redirect check is affected, but there may be more. This may also affect more types of files as opposed to just models too as I believe any class lookup (such as controllers or jobs) would have a similar issue

[presidentbeef]

Thank you for the clear examples and the example application!

I am looking at this now... I really don't want to try to replicate Ruby's name resolution, but I don't think it would be too difficult to at least store alternate names for each class.

[gavingmiller]

I've tracked this down with the code that @doliveirakn included for his example app to the problem occurring in:

brakeman/lib/brakeman/checks/base_check.rb

Lines 443 to 455 in a882d53

	def model_name? exp
	@models ||= @tracker.models.keys
	if exp.is_a? Symbol
	@models.include? exp
	elsif call? exp and exp.target.nil? and exp.method == :current_user
	true
	elsif sexp? exp
	@models.include? class_name(exp)
	else
	false
	end
	end

Specifically the failure occurs here where

@tracker.models.keys = [:BrakemanUnresolvedModel, :ApplicationRecord, :Document, :"Namespace::Task"]

And exp = s(:const, :Task) which suggests that the namespace in Namespace::TasksController isn't being picked up, which I'm guessing is what @presidentbeef meant by "I really don't want to try to replicate Ruby's name resolution".

I put together a poor man's fix against line 451:

    elsif sexp? exp
      @models.include? class_name(exp) or
        @models.any? { |m| m.to_s.include?(class_name(exp).to_s) }

This gets the desired test results and passes all specs, but I suspect this is largely because namespaced specs have not been written, likely this is not performant, and I can think of a few cases where this could report false positives.

Hopefully this is enough to kick off conversation on a fix, and if there is a different direction I should be taking, please let me know. Thanks!

[presidentbeef]

I spent some time on this previously: https://github.com/presidentbeef/brakeman/compare/class_names

There are a lot of changes because I tried to lift class names into an actual class instead of managing them as symbols.

However, in the end the result was probably similar to yours: fuzzier matching on class names. Unfortunately, it was too fuzzy. It seemed to only really work okay for models, not controllers. This led to too many weird results and false positives.

I haven't revisited it since, but I do believe there is still room for improvement.