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:41:55 +0800
Commit message: Refactor activity_user to remove possible SQL injection points.

Diff: diff --git a/app/controllers/home_controller.rb b/app/controllers/home_controller.rb
index 491fd4dc2..0306f02b5 100644
--- a/app/controllers/home_controller.rb
+++ b/app/controllers/home_controller.rb
@@ -120,24 +120,26 @@ class HomeController < ApplicationController
       event
     end
   end
 
   #----------------------------------------------------------------------------
+  # TODO: this is ugly, ugly code. It's being security patched now but urgently
+  # needs refactoring to use user id instead. Permuations based on name or email
+  # yield incorrect results.
   def activity_user
     user = current_user.pref[:activity_user]
     if user && user != "all_users"
       user = if user =~ /@/ # email
           User.where(:email => user).first
         else # first_name middle_name last_name any_name
           name_query = if user.include?(" ")
             user.name_permutations.map{ |first, last|
-              "(upper(first_name) LIKE upper('%#{first}%') AND upper(last_name) LIKE upper('%#{last}%'))"
-            }.join(" OR ")
+              User.where(:first_name => first, :last_name => last)
+            }.map(&:to_a).flatten.first
           else
-            "upper(first_name) LIKE upper('%#{user}%') OR upper(last_name) LIKE upper('%#{user}%')"
+            [User.where(:first_name => user), User.where(:last_name => user)].map(&:to_a).flatten.first
           end
-          User.where(name_query).first
         end
     end
     user.is_a?(User) ? user.id : nil
   end
 
diff --git a/spec/controllers/home_controller_spec.rb b/spec/controllers/home_controller_spec.rb
index ed1725d2f..0095333be 100644
--- a/spec/controllers/home_controller_spec.rb
+++ b/spec/controllers/home_controller_spec.rb
@@ -169,18 +169,20 @@ describe HomeController do
     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])
+      User.should_receive(:where).with(:first_name => 'Billy').and_return([@user])
+      User.should_receive(:where).with(:last_name => '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])
+      User.should_receive(:where).with(:first_name => 'Billy', :last_name => "Elliot").and_return([@user])
+      User.should_receive(:where).with(:first_name => 'Elliot', :last_name => "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')

Vulnerable code:               "(upper(first_name) LIKE upper('%#{first}%') AND upper(last_name) LIKE upper('%#{last}%'))"
            }.join(" OR ")
            "upper(first_name) LIKE upper('%#{user}%') OR upper(last_name) LIKE upper('%#{user}%')"
          User.where(name_query).first
      User.should_receive(:where).with("upper(first_name) LIKE upper('%Billy%') OR upper(last_name) LIKE upper('%Billy%')").and_return([@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])
Fixed code:   # TODO: this is ugly, ugly code. It's being security patched now but urgently
  # needs refactoring to use user id instead. Permuations based on name or email
  # yield incorrect results.
              User.where(:first_name => first, :last_name => last)
            }.map(&:to_a).flatten.first
            [User.where(:first_name => user), User.where(:last_name => user)].map(&:to_a).flatten.first
      User.should_receive(:where).with(:first_name => 'Billy').and_return([@user])
      User.should_receive(:where).with(:last_name => 'Billy').and_return([@user])
      User.should_receive(:where).with(:first_name => 'Billy', :last_name => "Elliot").and_return([@user])
      User.should_receive(:where).with(:first_name => 'Elliot', :last_name => "Billy").and_return([@user])