Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Working around a SQL Injection Vulnerability in Ruby on Rails (CVE-2012-5664) #800

Merged
merged 2 commits into from

4 participants

@steerio

Quoting Aaron Patterson from the Rails Security mailing list:

There is a SQL injection vulnerability in Active Record in ALL versions. This vulnerability has been assigned the CVE identifier CVE-2012-5664.

Versions Affected: All.
Not affected: NONE.
Fixed Versions: 3.2.10, 3.1.9, 3.0.18

Impact

Due to the way dynamic finders in Active Record extract options from method parameters, a method parameter can mistakenly be used as a scope. Carefully crafted requests can use the scope to inject arbitrary SQL.

All users running an affected release should either upgrade or use one of the work arounds immediately.

Impacted code passes user provided data to a dynamic finder like this:

Post.find_by_id(params[:id])

Releases

The 3.2.10, 3.1.9 & 3.0.18 releases are available at the normal locations.

Workarounds

The issue can be mitigated by explicitly converting the parameter to an expected value. For example, change this:

Post.find_by_id(params[:id])

to this:

Post.find_by_id(params[:id].to_s)

My rationale for the workaround was that it does not hurt to have it there, and it might save several users who can't upgrade their Rails versions (for incompatibility reasons for example), but can easily upgrade Cancan.

@steerio

An update on this issue explaining the true level of threat this poses. It's apparently not as bad as it might have seemed at first.

@byroot

Anyway only magic finders are affected, not AR#find

So I do not want to be rude but your commits are useless.

@Chetane

There are cases where magic finders are used, I think those are the ones steerio wants to protect against.

        if @options[:find_by]
          if resource_base.respond_to? "find_by_#{@options[:find_by]}!"
            resource_base.send("find_by_#{@options[:find_by]}!", id_param)
          else
            resource_base.send(@options[:find_by], id_param)
          end
        else
@byroot

Oh ! You're right, my bad ... /o\

The changes in specs misleaded me: wopata@d5123e0#L1R264

@ryanb ryanb merged commit 1cb33bd into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  lib/cancan/controller_resource.rb
@@ -129,7 +129,7 @@ def id_param
@params[@options[:id_param]]
else
@params[parent? ? :"#{name}_id" : :id]
- end
+ end.to_s
end
def member_action?
View
37 spec/cancan/controller_resource_spec.rb
@@ -20,7 +20,7 @@
end
it "should not load resource into an instance variable if already set" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
@controller.instance_variable_set(:@project, :some_project)
resource = CanCan::ControllerResource.new(@controller)
resource.load_resource
@@ -148,7 +148,7 @@ class Project < ::Project; end
end
it "should perform authorization using controller action and loaded model" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
@controller.instance_variable_set(:@project, :some_project)
stub(@controller).authorize!(:show, :some_project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller)
@@ -156,14 +156,14 @@ class Project < ::Project; end
end
it "should perform authorization using controller action and non loaded model" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
stub(@controller).authorize!(:show, Project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller)
lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
end
it "should call load_resource and authorize_resource for load_and_authorize_resource" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
resource = CanCan::ControllerResource.new(@controller)
mock(resource).load_resource
mock(resource).authorize_resource
@@ -171,7 +171,7 @@ class Project < ::Project; end
end
it "should not build a single resource when on custom collection action even with id" do
- @params.merge!(:action => "sort", :id => 123)
+ @params.merge!(:action => "sort", :id => "123")
resource = CanCan::ControllerResource.new(@controller, :collection => [:sort, :list])
resource.load_resource
@controller.instance_variable_get(:@project).should be_nil
@@ -187,7 +187,7 @@ class Project < ::Project; end
end
it "should build a resource when on custom new action even when params[:id] exists" do
- @params.merge!(:action => "build", :id => 123)
+ @params.merge!(:action => "build", :id => "123")
stub(Project).new { :some_project }
resource = CanCan::ControllerResource.new(@controller, :new => :build)
resource.load_resource
@@ -238,30 +238,30 @@ class Section
end
it "should load resource through the association of another parent resource using instance variable" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
category = Object.new
@controller.instance_variable_set(:@category, category)
- stub(category).projects.stub!.find(123) { :some_project }
+ stub(category).projects.stub!.find("123") { :some_project }
resource = CanCan::ControllerResource.new(@controller, :through => :category)
resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project
end
it "should load resource through the custom association name" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
category = Object.new
@controller.instance_variable_set(:@category, category)
- stub(category).custom_projects.stub!.find(123) { :some_project }
+ stub(category).custom_projects.stub!.find("123") { :some_project }
resource = CanCan::ControllerResource.new(@controller, :through => :category, :through_association => :custom_projects)
resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project
end
it "should load resource through the association of another parent resource using method" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
category = Object.new
stub(@controller).category { category }
- stub(category).projects.stub!.find(123) { :some_project }
+ stub(category).projects.stub!.find("123") { :some_project }
resource = CanCan::ControllerResource.new(@controller, :through => :category)
resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project
@@ -298,10 +298,10 @@ class Section
end
it "should load through first matching if multiple are given" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
category = Object.new
@controller.instance_variable_set(:@category, category)
- stub(category).projects.stub!.find(123) { :some_project }
+ stub(category).projects.stub!.find("123") { :some_project }
resource = CanCan::ControllerResource.new(@controller, :through => [:category, :user])
resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project
@@ -367,7 +367,7 @@ class Section
end
it "should authorize based on resource name if class is false" do
- @params.merge!(:action => "show", :id => 123)
+ @params.merge!(:action => "show", :id => "123")
stub(@controller).authorize!(:show, :project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller, :class => false)
lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
@@ -390,6 +390,13 @@ class Section
@controller.instance_variable_get(:@project).should == project
end
+ # CVE-2012-5664
+ it "should always convert id param to string" do
+ @params.merge!(:action => "show", :the_project => { :malicious => "I am" })
+ resource = CanCan::ControllerResource.new(@controller, :id_param => :the_project)
+ resource.send(:id_param).class.should == String
+ end
+
it "should load resource using custom find_by attribute" do
project = Project.create!(:name => "foo")
@params.merge!(:action => "show", :id => "foo")
View
12 spec/spec_helper.rb
@@ -20,6 +20,18 @@
config.extend WithModel if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
end
+# Working around CVE-2012-5664 requires us to convert all ID params
+# to strings. Let's switch to using string IDs in tests, otherwise
+# SuperModel and/or RR will fail (as strings are not fixnums).
+
+module SuperModel
+ class Base
+ def generate_id
+ object_id.to_s
+ end
+ end
+end
+
class Ability
include CanCan::Ability
Something went wrong with that request. Please try again.