Merge pull request #800 from wopata/master

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

lib/cancan/controller_resource.rb

@@ -129,7 +129,7 @@ module CanCan
         @params[@options[:id_param]]
       else
         @params[parent? ? :"#{name}_id" : :id]
-      end
+      end.to_s
     end
 
     def member_action?

spec/cancan/controller_resource_spec.rb

@@ -20,7 +20,7 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
   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 @@ describe CanCan::ControllerResource do
     @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")

spec/spec_helper.rb

@@ -20,6 +20,18 @@ RSpec.configure do |config|
   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