Working around CVE-2012-5664

This commit is contained in:
Roland Venesz 2013-01-03 13:16:30 +01:00
parent 3f4ee12025
commit d5123e0eb3
3 changed files with 36 additions and 16 deletions

View File

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

View File

@ -20,7 +20,7 @@ describe CanCan::ControllerResource do
end end
it "should not load resource into an instance variable if already set" do 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) @controller.instance_variable_set(:@project, :some_project)
resource = CanCan::ControllerResource.new(@controller) resource = CanCan::ControllerResource.new(@controller)
resource.load_resource resource.load_resource
@ -148,7 +148,7 @@ describe CanCan::ControllerResource do
end end
it "should perform authorization using controller action and loaded model" do 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) @controller.instance_variable_set(:@project, :some_project)
stub(@controller).authorize!(:show, :some_project) { raise CanCan::AccessDenied } stub(@controller).authorize!(:show, :some_project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller) resource = CanCan::ControllerResource.new(@controller)
@ -156,14 +156,14 @@ describe CanCan::ControllerResource do
end end
it "should perform authorization using controller action and non loaded model" do 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 } stub(@controller).authorize!(:show, Project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller) resource = CanCan::ControllerResource.new(@controller)
lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied) lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
end end
it "should call load_resource and authorize_resource for load_and_authorize_resource" do 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) resource = CanCan::ControllerResource.new(@controller)
mock(resource).load_resource mock(resource).load_resource
mock(resource).authorize_resource mock(resource).authorize_resource
@ -171,7 +171,7 @@ describe CanCan::ControllerResource do
end end
it "should not build a single resource when on custom collection action even with id" do 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 = CanCan::ControllerResource.new(@controller, :collection => [:sort, :list])
resource.load_resource resource.load_resource
@controller.instance_variable_get(:@project).should be_nil @controller.instance_variable_get(:@project).should be_nil
@ -187,7 +187,7 @@ describe CanCan::ControllerResource do
end end
it "should build a resource when on custom new action even when params[:id] exists" do 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 } stub(Project).new { :some_project }
resource = CanCan::ControllerResource.new(@controller, :new => :build) resource = CanCan::ControllerResource.new(@controller, :new => :build)
resource.load_resource resource.load_resource
@ -238,30 +238,30 @@ describe CanCan::ControllerResource do
end end
it "should load resource through the association of another parent resource using instance variable" do 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 category = Object.new
@controller.instance_variable_set(:@category, category) @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 = CanCan::ControllerResource.new(@controller, :through => :category)
resource.load_resource resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project @controller.instance_variable_get(:@project).should == :some_project
end end
it "should load resource through the custom association name" do 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 category = Object.new
@controller.instance_variable_set(:@category, category) @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 = CanCan::ControllerResource.new(@controller, :through => :category, :through_association => :custom_projects)
resource.load_resource resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project @controller.instance_variable_get(:@project).should == :some_project
end end
it "should load resource through the association of another parent resource using method" do 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 category = Object.new
stub(@controller).category { category } 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 = CanCan::ControllerResource.new(@controller, :through => :category)
resource.load_resource resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project @controller.instance_variable_get(:@project).should == :some_project
@ -298,10 +298,10 @@ describe CanCan::ControllerResource do
end end
it "should load through first matching if multiple are given" do 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 category = Object.new
@controller.instance_variable_set(:@category, category) @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 = CanCan::ControllerResource.new(@controller, :through => [:category, :user])
resource.load_resource resource.load_resource
@controller.instance_variable_get(:@project).should == :some_project @controller.instance_variable_get(:@project).should == :some_project
@ -367,7 +367,7 @@ describe CanCan::ControllerResource do
end end
it "should authorize based on resource name if class is false" do 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 } stub(@controller).authorize!(:show, :project) { raise CanCan::AccessDenied }
resource = CanCan::ControllerResource.new(@controller, :class => false) resource = CanCan::ControllerResource.new(@controller, :class => false)
lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied) lambda { resource.authorize_resource }.should raise_error(CanCan::AccessDenied)
@ -390,6 +390,14 @@ describe CanCan::ControllerResource do
@controller.instance_variable_get(:@project).should == project @controller.instance_variable_get(:@project).should == project
end 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 it "should load resource using custom find_by attribute" do
project = Project.create!(:name => "foo") project = Project.create!(:name => "foo")
@params.merge!(:action => "show", :id => "foo") @params.merge!(:action => "show", :id => "foo")

View File

@ -20,6 +20,18 @@ RSpec.configure do |config|
config.extend WithModel if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record" config.extend WithModel if ENV["MODEL_ADAPTER"].nil? || ENV["MODEL_ADAPTER"] == "active_record"
end 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 class Ability
include CanCan::Ability include CanCan::Ability