From d5123e0eb3017de67a9e6afa45c14a5ef096f2a8 Mon Sep 17 00:00:00 2001 From: Roland Venesz Date: Thu, 3 Jan 2013 13:16:30 +0100 Subject: [PATCH 1/2] Working around CVE-2012-5664 --- lib/cancan/controller_resource.rb | 2 +- spec/cancan/controller_resource_spec.rb | 38 +++++++++++++++---------- spec/spec_helper.rb | 12 ++++++++ 3 files changed, 36 insertions(+), 16 deletions(-) diff --git a/lib/cancan/controller_resource.rb b/lib/cancan/controller_resource.rb index 8ca8a16..20def9d 100644 --- a/lib/cancan/controller_resource.rb +++ b/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? diff --git a/spec/cancan/controller_resource_spec.rb b/spec/cancan/controller_resource_spec.rb index 2fa05d9..f5273fe 100644 --- a/spec/cancan/controller_resource_spec.rb +++ b/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,14 @@ describe CanCan::ControllerResource do @controller.instance_variable_get(:@project).should == project end + # CVE-2012-5664 + it "should always convert id param to string" do + project = Project.create! + @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") diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index e022933..97169cc 100644 --- a/spec/spec_helper.rb +++ b/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 From d3a8929111b27edbe35924a771e2ff6ffd71cd44 Mon Sep 17 00:00:00 2001 From: Roland Venesz Date: Thu, 3 Jan 2013 13:26:24 +0100 Subject: [PATCH 2/2] Creating a Project here is unnecessary --- spec/cancan/controller_resource_spec.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/cancan/controller_resource_spec.rb b/spec/cancan/controller_resource_spec.rb index f5273fe..513263e 100644 --- a/spec/cancan/controller_resource_spec.rb +++ b/spec/cancan/controller_resource_spec.rb @@ -392,7 +392,6 @@ describe CanCan::ControllerResource do # CVE-2012-5664 it "should always convert id param to string" do - project = Project.create! @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