CVE-2013-2506: Permissions, Privileges, and Access Controls (CWE-264)
value: app/models/spree/user.rb in spree_auth_devise in Spree 1.1.x before 1.1.6, 1.2.x, and 1.3.x does not perform mass assignment safely when updating a user, which allows remote authenticated users to assign arbitrary roles to themselves.
CWE description: Weaknesses in this category are related to the management of permissions, privileges, and other security features that are used to perform access control.
Published: 2013-03-08T18:55Z
Repository: https://github.com/spree/spree_auth_devise
Language: Ruby
Files changed: app/controllers/spree/admin/users_controller.rb, app/models/spree/user.rb, spec/controllers/spree/users_controller_spec.rb
Commit date: 2013-02-25 16:08:56 -0500
Commit message: Remove Mass Assignment of Role IDs

Diff: diff --git a/app/controllers/spree/admin/users_controller.rb b/app/controllers/spree/admin/users_controller.rb
index e154842..49ed87d 100644
--- a/app/controllers/spree/admin/users_controller.rb
+++ b/app/controllers/spree/admin/users_controller.rb
@@ -14,10 +14,51 @@ module Spree
           format.html
           format.json { render :json => json_data }
         end
       end
 
+      def create
+        if params[:user]
+          roles = params[:user].delete("spree_role_ids")
+        end
+
+        @user = Spree::User.new(params[:user])
+        if @user.save
+
+          if roles
+            @user.spree_roles = roles.reject(&:blank?).collect{|r| Spree::Role.find(r)}
+          end
+
+          flash.now[:notice] = t(:created_successfully)
+          render :edit
+        else
+          render :new
+        end
+      end
+
+      def update
+        if params[:user]
+          roles = params[:user].delete("spree_role_ids")
+        end
+
+        if @user.update_attributes(params[:user])
+          if roles
+            @user.spree_roles = roles.reject(&:blank?).collect{|r| Spree::Role.find(r)}
+          end
+
+          if params[:user][:password].present?
+            # this logic needed b/c devise wants to log us out after password changes
+            user = Spree::User.reset_password_by_token(params[:user])
+            sign_in(@user, :event => :authentication, :bypass => !Spree::Auth::Config[:signout_after_password_change])
+          end
+          flash.now[:notice] = t(:account_updated)
+          render :edit
+        else
+          render :edit
+        end
+      end
+
       def generate_api_key
         if @user.generate_spree_api_key!
           flash.notice = t('key_generated', :scope => 'spree.api')
         end
         redirect_to edit_admin_user_path(@user)
diff --git a/app/models/spree/user.rb b/app/models/spree/user.rb
index b03e1a8..461d53e 100644
--- a/app/models/spree/user.rb
+++ b/app/models/spree/user.rb
@@ -12,11 +12,11 @@ module Spree
     before_save :check_admin
     before_validation :set_login
     before_destroy :check_completed_orders
 
     # Setup accessible (or protected) attributes for your model
-    attr_accessible :email, :password, :password_confirmation, :remember_me, :persistence_token, :login, :spree_role_ids
+    attr_accessible :email, :password, :password_confirmation, :remember_me, :persistence_token, :login
 
     users_table_name = User.table_name
     roles_table_name = Role.table_name
 
     scope :admin, lambda { includes(:spree_roles).where("#{roles_table_name}.name" => "admin") }
diff --git a/spec/controllers/spree/users_controller_spec.rb b/spec/controllers/spree/users_controller_spec.rb
index 1bb644d..d3cc345 100644
--- a/spec/controllers/spree/users_controller_spec.rb
+++ b/spec/controllers/spree/users_controller_spec.rb
@@ -1,10 +1,11 @@
 require 'spec_helper'
 
 describe Spree::UsersController do
   let(:admin_user) { create(:user) }
   let(:user) { create(:user) }
+  let(:role) { create(:role) }
 
   before do
     controller.stub(:spree_current_user => user)
   end
 
@@ -21,7 +22,11 @@ describe Spree::UsersController do
         spree_put :update, { :user => { :email => 'mynew@email-address.com' } }
         assigns[:user].email.should == 'mynew@email-address.com'
         response.should redirect_to(spree.account_url(:only_path => true))
       end
     end
+
+    it 'should not update roles' do
+      expect { spree_put :update, { :user => { :spree_role_ids => [role.id] } }}.to raise_exception(ActiveModel::MassAssignmentSecurity::Error)
+    end
   end
 end

Vulnerable code:     attr_accessible :email, :password, :password_confirmation, :remember_me, :persistence_token, :login, :spree_role_ids
Fixed code:       def create
        if params[:user]
          roles = params[:user].delete("spree_role_ids")
        end

        @user = Spree::User.new(params[:user])
        if @user.save

          if roles
            @user.spree_roles = roles.reject(&:blank?).collect{|r| Spree::Role.find(r)}
          end

          flash.now[:notice] = t(:created_successfully)
          render :edit
        else
          render :new
        end
      end

      def update
        if params[:user]
          roles = params[:user].delete("spree_role_ids")
        end

        if @user.update_attributes(params[:user])
          if roles
            @user.spree_roles = roles.reject(&:blank?).collect{|r| Spree::Role.find(r)}
          end

          if params[:user][:password].present?
            # this logic needed b/c devise wants to log us out after password changes
            user = Spree::User.reset_password_by_token(params[:user])
            sign_in(@user, :event => :authentication, :bypass => !Spree::Auth::Config[:signout_after_password_change])
          end
          flash.now[:notice] = t(:account_updated)
          render :edit
        else
          render :edit
        end
      end

    attr_accessible :email, :password, :password_confirmation, :remember_me, :persistence_token, :login
  let(:role) { create(:role) }

    it 'should not update roles' do
      expect { spree_put :update, { :user => { :spree_role_ids => [role.id] } }}.to raise_exception(ActiveModel::MassAssignmentSecurity::Error)
    end