CVE-2013-7225: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: Multiple SQL injection vulnerabilities in app/controllers/home_controller.rb in Fat Free CRM before 0.12.1 allow remote authenticated users to execute arbitrary SQL commands via (1) the homepage timeline feature or (2) the activity feature.
CWE description: The product constructs all or part of an SQL command using externally-influenced input from an upstream component, but it does not neutralize or incorrectly neutralizes special elements that could modify the intended SQL command when it is sent to a downstream component. Without sufficient removal or quoting of SQL syntax in user-controllable inputs, the generated SQL query can cause those inputs to be interpreted as SQL instead of ordinary user data.
Published: 2014-01-02T14:59Z
Repository: https://github.com/fatfreecrm/fat_free_crm
Language: Ruby
Files changed: app/controllers/home_controller.rb, spec/controllers/home_controller_spec.rb
Commit date: 2013-12-27 15:06:58 +0800
Commit message: Fixed sql injection in timeline method.

Diff: diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
index 1192dadf8..491fd4dc2 100644
--- a/app/controllers/home_controller.rb
+++ b/app/controllers/home_controller.rb
@@ -55,18 +55,23 @@ class HomeController < ApplicationController
   end
 
   # GET /home/timeline                                                     AJAX
   #----------------------------------------------------------------------------
   def timeline
-    unless params[:type].empty?
-      model = params[:type].camelize.constantize
-      item = model.find(params[:id])
-      item.update_attribute(:state, params[:state])
-    else
-      comments, emails = params[:id].split("+")
-      Comment.update_all("state = '#{params[:state]}'", "id IN (#{comments})") unless comments.blank?
-      Email.update_all("state = '#{params[:state]}'", "id IN (#{emails})") unless emails.blank?
+    state = params[:state].to_s
+    if %w(Collapsed Expanded).include?(state)
+      if (model_type = params[:type].to_s).present?
+        if %w(comment email).include?(model_type)
+          model = model_type.camelize.constantize
+          item = model.find(params[:id])
+          item.update_attribute(:state, state)
+        end
+      else
+        comments, emails = params[:id].split("+")
+        Comment.where(:id => comments.split(',')).update_all(:state => state) unless comments.blank?
+        Email.where(:id => emails.split(',')).update_all(:state => state) unless emails.blank?
+      end
     end
 
     render :nothing => true
   end
 
diff --git a/spec/controllers/home_controller_spec.rb b/spec/controllers/home_controller_spec.rb
index 52e97b049..ed1725d2f 100644
--- a/spec/controllers/home_controller_spec.rb
+++ b/spec/controllers/home_controller_spec.rb
@@ -40,17 +40,17 @@ describe HomeController do
 
       get :index
       assigns[:my_tasks].should == [task_1, task_2, task_3, task_4]
     end
 
-		it "should not display completed tasks" do
-			task_1 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Your first task", :bucket => "due_asap", :assigned_to => current_user.id)
-			task_2 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Completed task", :bucket => "due_asap", :completed_at => 1.days.ago, :completed_by => current_user.id, :assigned_to => current_user.id)
+    it "should not display completed tasks" do
+      task_1 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Your first task", :bucket => "due_asap", :assigned_to => current_user.id)
+      task_2 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Completed task", :bucket => "due_asap", :completed_at => 1.days.ago, :completed_by => current_user.id, :assigned_to => current_user.id)
 
-			get :index
-			assigns[:my_tasks].should == [task_1]
-		end
+      get :index
+      assigns[:my_tasks].should == [task_1]
+    end
 
     it "should get a list of my opportunities ordered by closes_on" do
       opportunity_1 = FactoryGirl.create(:opportunity, :name => "Your first opportunity", :closes_on => 15.days.from_now, :assigned_to => current_user.id, :stage => 'proposal')
       opportunity_2 = FactoryGirl.create(:opportunity, :name => "Another opportunity for you", :closes_on => 10.days.from_now, :assigned_to => current_user.id, :stage => 'proposal')
       opportunity_3 = FactoryGirl.create(:opportunity, :name => "Third Opportunity", :closes_on => 5.days.from_now, :assigned_to => current_user.id, :stage => 'proposal')
@@ -151,44 +151,93 @@ describe HomeController do
 
       xhr :get, :toggle, :id => "hello"
       session[:hello].should == true
     end
   end
-  
+
   describe "activity_user" do
-  
+
     before(:each) do
       @user = double(User, :id => 1, :is_a? => true)
       @cur_user = double(User)
     end
-  
+
     it "should find a user by email" do
       @cur_user.stub(:pref).and_return(:activity_user => 'billy@example.com')
       controller.instance_variable_set(:@current_user, @cur_user)
       User.should_receive(:where).with(:email => 'billy@example.com').and_return([@user])
       controller.send(:activity_user).should == 1
     end
-    
+
     it "should find a user by first name or last name" do
       @cur_user.stub(:pref).and_return(:activity_user => 'Billy')
       controller.instance_variable_set(:@current_user, @cur_user)
       User.should_receive(:where).with("upper(first_name) LIKE upper('%Billy%') OR upper(last_name) LIKE upper('%Billy%')").and_return([@user])
       controller.send(:activity_user).should == 1
     end
-    
+
     it "should find a user by first name and last name" do
       @cur_user.stub(:pref).and_return(:activity_user => 'Billy Elliot')
       controller.instance_variable_set(:@current_user, @cur_user)
       User.should_receive(:where).with("(upper(first_name) LIKE upper('%Billy%') AND upper(last_name) LIKE upper('%Elliot%')) OR (upper(first_name) LIKE upper('%Elliot%') AND upper(last_name) LIKE upper('%Billy%'))").and_return([@user])
       controller.send(:activity_user).should == 1
     end
-    
+
     it "should return nil when 'all_users' is specified" do
       @cur_user.stub(:pref).and_return(:activity_user => 'all_users')
       controller.instance_variable_set(:@current_user, @cur_user)
       User.should_not_receive(:where)
       controller.send(:activity_user).should == nil
     end
-    
+
+  end
+
+  describe "timeline" do
+
+    before(:each) do
+      require_user
+    end
+
+    it "should collapse all comments and emails on a specific contact" do
+      comment = double(Comment)
+      Comment.should_receive(:find).with("1").and_return(comment)
+      comment.should_receive(:update_attribute).with(:state, 'Collapsed')
+      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Collapsed"
+    end
+
+    it "should expand all comments and emails on a specific contact" do
+      comment = double(Comment)
+      Comment.should_receive(:find).with("1").and_return(comment)
+      comment.should_receive(:update_attribute).with(:state, 'Expanded')
+      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Expanded"
+    end
+
+    it "should not do anything when state neither Expanded nor Collapsed" do
+      comment = double(Comment)
+      Comment.should_not_receive(:find).with("1")
+      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Explode"
+    end
+
+    it "should collapse all comments and emails on Contact" do
+      where_stub = double
+      where_stub.should_receive(:update_all).with(:state => "Collapsed")
+      Comment.should_receive(:where).and_return(where_stub)
+      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Collapsed"
+    end
+
+    it "should not allow an arbitary state (sanitizes input)" do
+      where_stub = double
+      where_stub.should_receive(:update_all).with(:state => "Expanded")
+      Comment.should_receive(:where).and_return(where_stub)
+      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Expanded"
+    end
+
+    it "should not update an arbitary model (sanitizes input)" do
+      where_stub = double
+      where_stub.should_receive(:update_all).with(:state => "Expanded")
+      Comment.should_receive(:where).and_return(where_stub)
+      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Expanded"
+    end
+
   end
 
 end

Vulnerable code:     unless params[:type].empty?
      model = params[:type].camelize.constantize
      item = model.find(params[:id])
      item.update_attribute(:state, params[:state])
    else
      comments, emails = params[:id].split("+")
      Comment.update_all("state = '#{params[:state]}'", "id IN (#{comments})") unless comments.blank?
      Email.update_all("state = '#{params[:state]}'", "id IN (#{emails})") unless emails.blank?
		it "should not display completed tasks" do
			task_1 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Your first task", :bucket => "due_asap", :assigned_to => current_user.id)
			task_2 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Completed task", :bucket => "due_asap", :completed_at => 1.days.ago, :completed_by => current_user.id, :assigned_to => current_user.id)
			get :index
			assigns[:my_tasks].should == [task_1]
		end
  
  
  
    
    
    
    
Fixed code:     state = params[:state].to_s
    if %w(Collapsed Expanded).include?(state)
      if (model_type = params[:type].to_s).present?
        if %w(comment email).include?(model_type)
          model = model_type.camelize.constantize
          item = model.find(params[:id])
          item.update_attribute(:state, state)
        end
      else
        comments, emails = params[:id].split("+")
        Comment.where(:id => comments.split(',')).update_all(:state => state) unless comments.blank?
        Email.where(:id => emails.split(',')).update_all(:state => state) unless emails.blank?
      end
    it "should not display completed tasks" do
      task_1 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Your first task", :bucket => "due_asap", :assigned_to => current_user.id)
      task_2 = FactoryGirl.create(:task, :user_id => current_user.id, :name => "Completed task", :bucket => "due_asap", :completed_at => 1.days.ago, :completed_by => current_user.id, :assigned_to => current_user.id)
      get :index
      assigns[:my_tasks].should == [task_1]
    end







  end

  describe "timeline" do

    before(:each) do
      require_user
    end

    it "should collapse all comments and emails on a specific contact" do
      comment = double(Comment)
      Comment.should_receive(:find).with("1").and_return(comment)
      comment.should_receive(:update_attribute).with(:state, 'Collapsed')
      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Collapsed"
    end

    it "should expand all comments and emails on a specific contact" do
      comment = double(Comment)
      Comment.should_receive(:find).with("1").and_return(comment)
      comment.should_receive(:update_attribute).with(:state, 'Expanded')
      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Expanded"
    end

    it "should not do anything when state neither Expanded nor Collapsed" do
      comment = double(Comment)
      Comment.should_not_receive(:find).with("1")
      xhr :get, :timeline, :type => "comment", :id => "1", :state => "Explode"
    end

    it "should collapse all comments and emails on Contact" do
      where_stub = double
      where_stub.should_receive(:update_all).with(:state => "Collapsed")
      Comment.should_receive(:where).and_return(where_stub)
      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Collapsed"
    end

    it "should not allow an arbitary state (sanitizes input)" do
      where_stub = double
      where_stub.should_receive(:update_all).with(:state => "Expanded")
      Comment.should_receive(:where).and_return(where_stub)
      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Expanded"
    end

    it "should not update an arbitary model (sanitizes input)" do
      where_stub = double
      where_stub.should_receive(:update_all).with(:state => "Expanded")
      Comment.should_receive(:where).and_return(where_stub)
      xhr :get, :timeline, :id => "1,2,3,4+", :state => "Expanded"
    end