CVE-2015-2972: Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection') (CWE-89)
value: Multiple SQL injection vulnerabilities in Sysphonic Thetis before 2.3.0 allow remote attackers to execute arbitrary SQL commands via unspecified vectors.
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: 2015-07-19T15:59Z
Repository: https://github.com/sysphonic/thetis
Language: Ruby
Files changed: app/controllers/application_controller.rb, app/controllers/mail_filters_controller.rb, app/controllers/mail_folders_controller.rb, app/controllers/workflows_controller.rb, app/helpers/mail_filters_helper.rb, app/models/address.rb, app/models/comment.rb, app/models/email.rb, app/models/item.rb, app/models/toy.rb, app/models/user.rb, app/views/mail_filters/_mail_filter_form.html.erb
Commit date: 2015-07-07 10:28:35 +0900
Commit message: Reinforcement of security.
Signed-off-by: MORITA Shintaro <shin@sysphonic.com>

Diff: diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 1e45ce15..9b83a1af 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -73,22 +73,22 @@ class ApplicationController < ActionController::Base
 
  public
   #=== paginate_by_sql
   #
   def paginate_by_sql(model, sql, per_page, options={})
-    if options[:count]
+    if options[:count].blank?
+      total = model.count_by_sql_wrapping_select_query(sql)
+    else
       if options[:count].is_a?(Integer)
         total = options[:count]
-      else
-        total = model.count_by_sql(options[:count])
+      #else
+      #  total = model.count_by_sql(options[:count])
       end
-    else
-      total = model.count_by_sql_wrapping_select_query(sql)
     end
 
+    SqlHelper.validate_token([params['page']])
     object_pages = model.paginate_by_sql(sql, {:page => params['page'], :per_page => per_page})
-    #objects = model.find_by_sql_with_limit(sql, object_pages.current.to_sql[1], per_page)
     return [object_pages, object_pages, total]
   end
 
  protected
   #=== gate_process
@@ -97,11 +97,11 @@ class ApplicationController < ActionController::Base
   #
   def gate_process
 
     HistoryHelper.keep_last(request)
 
-    @login_user = User.find_by_id(session[:login_user_id])
+    @login_user = User.find(session[:login_user_id])
 
     begin
       if @login_user.nil? \
            or @login_user.time_zone.nil? or @login_user.time_zone.empty?
         unless THETIS_USER_TIMEZONE_DEFAULT.nil? or THETIS_USER_TIMEZONE_DEFAULT.empty?
@@ -116,16 +116,10 @@ class ApplicationController < ActionController::Base
   end
 end
 
 module ActiveRecord
   class Base
-    def self.find_by_sql_with_limit(sql, offset, limit)
-      sql = sanitize_sql(sql)
-      add_limit!(sql, {:limit => limit, :offset => offset})
-      find_by_sql(sql)
-    end
-
     def self.count_by_sql_wrapping_select_query(sql)
       sql = sanitize_sql(sql)
       count_by_sql("select count(*) from (#{sql}) as my_table")
     end
 
diff --git a/app/controllers/mail_filters_controller.rb b/app/controllers/mail_filters_controller.rb
index c1ee6aaa..76542940 100644
--- a/app/controllers/mail_filters_controller.rb
+++ b/app/controllers/mail_filters_controller.rb
@@ -106,11 +106,14 @@ class MailFiltersController < ApplicationController
   def show
     if params[:action] == 'show'
       Log.add_info(request, params.inspect)
     end
 
-    @mail_filter = MailFilter.find_by_id(params[:id])
+    begin
+      @mail_filter = MailFilter.find(params[:id])
+    rescue => evar
+    end
     if @mail_filter.nil?
       render(:text => 'ERROR:' + t('msg.already_deleted', :name => MailFilter.model_name.human))
       return
     else
       if @mail_filter.mail_account.user_id != @login_user.id
@@ -223,12 +226,12 @@ class MailFiltersController < ApplicationController
   #Does execute MailFilters of the specified MailAccount.
   #
   def do_execute
     Log.add_info(request, params.inspect)
 
-    mail_account = MailAccount.find_by_id(params[:mail_account_id])
-    mail_folder = MailFolder.find_by_id(params[:mail_folder_id])
+    mail_account = MailAccount.find(params[:mail_account_id])
+    mail_folder = MailFolder.find(params[:mail_folder_id])
 
     if mail_account.user_id != @login_user.id \
         or mail_folder.user_id != @login_user.id
       render(:text => t('msg.need_to_be_owner'))
       return
@@ -256,12 +259,13 @@ class MailFiltersController < ApplicationController
   #
   def get_order
     Log.add_info(request, params.inspect)
 
     mail_account_id = params[:mail_account_id]
+    SqlHelper.validate_token([mail_account_id])
 
-    @mail_account = MailAccount.find_by_id(mail_account_id)
+    @mail_account = MailAccount.find(mail_account_id)
 
     if @mail_account.user_id != @login_user.id
       flash[:notice] = t('msg.need_to_be_owner')
       redirect_to(:controller => 'desktop', :action => 'show')
       return
@@ -283,13 +287,15 @@ class MailFiltersController < ApplicationController
   #
   def update_order
     Log.add_info(request, params.inspect)
 
     mail_account_id = params[:mail_account_id]
-    order_ary = params[:mail_filters_order]
+    order_arr = params[:mail_filters_order]
+
+    SqlHelper.validate_token([mail_account_id])
 
-    @mail_account = MailAccount.find_by_id(mail_account_id)
+    @mail_account = MailAccount.find(mail_account_id)
 
     if @mail_account.user_id != @login_user.id
       render(:text => 'ERROR:' + t('msg.need_to_be_owner'))
       return
     end
@@ -299,12 +305,12 @@ class MailFiltersController < ApplicationController
 
     filters.sort! { |filter_a, filter_b|
       id_a = filter_a.id.to_s
       id_b = filter_b.id.to_s
 
-      idx_a = order_ary.index(id_a)
-      idx_b = order_ary.index(id_b)
+      idx_a = order_arr.index(id_a)
+      idx_b = order_arr.index(id_b)
 
       if idx_a.nil? or idx_b.nil?
         idx_a = filters.index(id_a)
         idx_b = filters.index(id_b)
       end
diff --git a/app/controllers/mail_folders_controller.rb b/app/controllers/mail_folders_controller.rb
index 0b355d22..dd67e47f 100644
--- a/app/controllers/mail_folders_controller.rb
+++ b/app/controllers/mail_folders_controller.rb
@@ -300,11 +300,11 @@ class MailFoldersController < ApplicationController
   def get_mail_attachments
     Log.add_info(request, params.inspect)
 
     email_id = params[:id]
 
-    email = Email.find_by_id(email_id)
+    email = Email.find(email_id)
     if email.nil? or email.user_id != @login_user.id
       render(:text => '')
       return
     end
 
@@ -325,18 +325,24 @@ class MailFoldersController < ApplicationController
   #
   def get_mail_attachment
     Log.add_info(request, params.inspect)
 
     attached_id = params[:id].to_i
-    mail_attach = MailAttachment.find_by_id(attached_id)
+    begin
+      mail_attach = MailAttachment.find(attached_id)
+    rescue => evar
+    end
 
     if mail_attach.nil?
       redirect_to(THETIS_RELATIVE_URL_ROOT + '/404.html')
       return
     end
 
-    email = Email.find_by_id(mail_attach.email_id)
+    begin
+      email = Email.find(mail_attach.email_id)
+    rescue => evar
+    end
     if email.nil? or email.user_id != @login_user.id
       render(:text => '')
       return
     end
 
@@ -436,11 +442,14 @@ class MailFoldersController < ApplicationController
 
       count = 0
       params[:check_mail].each do |email_id, value|
         next if value != '1'
 
-        email = Email.find_by_id(email_id)
+        begin
+          email = Email.find(email_id)
+        rescue => evar
+        end
         next if email.nil? or (email.user_id != @login_user.id)
 
         if trash_folder.nil? \
             or folder_id == trash_folder.id.to_s \
             or mail_folder.get_parents(false).include?(trash_folder.id.to_s)
@@ -471,11 +480,15 @@ class MailFoldersController < ApplicationController
   #
   def ajax_move_mails
     Log.add_info(request, params.inspect)
 
     folder_id = params[:thetisBoxSelKeeper].split(':').last
-    mail_folder = MailFolder.find_by_id(folder_id)
+    SqlHelper.validate_token([folder_id])
+    begin
+      mail_folder = MailFolder.find(folder_id)
+    rescue => evar
+    end
 
     if folder_id == '0' \
         or mail_folder.nil? \
         or mail_folder.user_id != @login_user.id
       flash[:notice] = 'ERROR:' + t('msg.cannot_save_in_folder')
@@ -539,19 +552,20 @@ class MailFoldersController < ApplicationController
   #Updates folders' order by Ajax.
   #
   def update_folders_order
     Log.add_info(request, params.inspect)
 
-    order_ary = params[:folders_order]
+    order_arr = params[:folders_order]
 
+    SqlHelper.validate_token([params[:id]])
     folders = MailFolder.get_childs(params[:id], false, false)
     # folders must be ordered by xorder ASC.
 
     folders.sort! { |id_a, id_b|
 
-      idx_a = order_ary.index(id_a)
-      idx_b = order_ary.index(id_b)
+      idx_a = order_arr.index(id_a)
+      idx_b = order_arr.index(id_b)
 
       if idx_a.nil? or idx_b.nil?
         idx_a = folders.index(id_a)
         idx_b = folders.index(id_b)
       end
diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb
index d2885a53..9d415ea0 100644
--- a/app/controllers/workflows_controller.rb
+++ b/app/controllers/workflows_controller.rb
@@ -101,10 +101,11 @@ class WorkflowsController < ApplicationController
   def move
     Log.add_info(request, params.inspect)
 
     unless params[:thetisBoxSelKeeper].nil?
       folder_id = params[:thetisBoxSelKeeper].split(':').last
+      SqlHelper.validate_token([folder_id])
 
       workflow = Workflow.find(params[:id])
 
       workflow.item.update_attribute(:folder_id, folder_id)
 
diff --git a/app/helpers/mail_filters_helper.rb b/app/helpers/mail_filters_helper.rb
index 56a9a9e1..62cb009e 100644
--- a/app/helpers/mail_filters_helper.rb
+++ b/app/helpers/mail_filters_helper.rb
@@ -411,12 +411,16 @@ module MailFiltersHelper
   end
 
   def self.execute_action_move(mail_filter, email, val)
 
     mail_folder_id = val
+    SqlHelper.validate_token([mail_folder_id])
 
-    mail_folder = MailFolder.find_by_id(mail_folder_id)
+    begin
+      mail_folder = MailFolder.find(mail_folder_id)
+    rescue => evar
+    end
     if !mail_folder.nil? and (mail_folder.user_id == email.user_id)
       email.update_attribute(:mail_folder_id, mail_folder_id)
     end
 
     return true
diff --git a/app/models/address.rb b/app/models/address.rb
index 20e91591..18c6b158 100644
--- a/app/models/address.rb
+++ b/app/models/address.rb
@@ -208,11 +208,12 @@ class Address < ActiveRecord::Base
   #return:: Address instance created from specified array.
   #
   def self.parse_csv_row(row, book, idxs, user)
 
     imp_id = (idxs[0].nil? or row[idxs[0]].nil?)?(nil):(row[idxs[0]].strip)
-    unless imp_id.nil? or imp_id.empty?
+    SqlHelper.validate_token([imp_id])
+    unless imp_id.blank?
       org_address = Address.find_by_id(imp_id)
     end
 
     if org_address.nil?
       address = Address.new
@@ -303,11 +304,15 @@ class Address < ActiveRecord::Base
     unless self.groups.nil? or self.groups.empty?
 
       if (/^|([0-9]+|)+$/ =~ self.groups) == 0
 
         self.get_groups_a.each do |group_id|
-          group = Group.find_by_id(group_id)
+          begin
+            group = Group.find(group_id)
+          rescue => evar
+            group = nil
+          end
           if group.nil?
             err_msgs << I18n.t('address.import.not_valid_groups') + ': '+group_id.to_s
             break
           end
         end
@@ -320,11 +325,15 @@ class Address < ActiveRecord::Base
     unless self.teams.nil? or self.teams.empty?
 
       if (/^|([0-9]+|)+$/ =~ self.teams) == 0
 
         self.get_teams_a.each do |team_id|
-          team = Team.find_by_id(team_id)
+          begin
+            team = Team.find(team_id)
+          rescue => evar
+            team = nil
+          end
           if team.nil?
             err_msgs << I18n.t('address.import.not_valid_teams') + ': '+team_id.to_s
             break
           end
         end
diff --git a/app/models/comment.rb b/app/models/comment.rb
index a5c8aad9..7cfdcdb3 100644
--- a/app/models/comment.rb
+++ b/app/models/comment.rb
@@ -102,12 +102,11 @@ class Comment < ActiveRecord::Base
     begin
       attachments = Attachment.find_by_sql(sql)
     rescue => evar
       Log.add_error(nil, evar)
     end
-    attachments = [] if attachments.nil?
-    return attachments
+    return (attachments || [])
   end
 
   #=== self.get_toys
   #
   #Gets Toys (desktop items) of specified User.
@@ -158,7 +157,6 @@ class Comment < ActiveRecord::Base
       end
       entries << feed_entry
     end
     return entries
   end
-
 end
diff --git a/app/models/email.rb b/app/models/email.rb
index 8d5f026e..0c0c1f65 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -51,11 +51,11 @@ class Email < ActiveRecord::Base
     email.recalc_size
 
 # FEATURE_MAIL_STRICT_CAPACITY >>>
     if (email.status != Email::STATUS_TEMPORARY) \
         and (email.size > org_size)
-      mail_account = MailAccount.find_by_id(email.mail_account_id)
+      mail_account = MailAccount.find(email.mail_account_id)
       max_size = mail_account.get_capacity_mb * 1024 * 1024
       con = "(id != #{email.id})" unless email.id.nil?
       cur_size = MailAccount.get_using_size(mail_account.id, con)
       if email.size > (max_size - cur_size)
         raise('ERROR:' + I18n.t('mail.msg.capacity_over'))
diff --git a/app/models/item.rb b/app/models/item.rb
index ca2f3b4e..038d73b6 100644
--- a/app/models/item.rb
+++ b/app/models/item.rb
@@ -66,11 +66,15 @@ class Item < ActiveRecord::Base
   def is_a_copy?(folder_obj_cache=nil)
 
     return false if self.source_id.nil?
 
     # Exclude those created from system templates.
-    src_item = Item.find_by_id(self.source_id)
+    begin
+      src_item = Item.find(self.source_id)
+    rescue => evar
+      src_item = nil
+    end
     if src_item.nil?
       return true
     else
       return !src_item.in_system_folder?(folder_obj_cache)
     end
diff --git a/app/models/toy.rb b/app/models/toy.rb
index a0bf852f..dcc5c548 100644
--- a/app/models/toy.rb
+++ b/app/models/toy.rb
@@ -123,61 +123,61 @@ class Toy < ActiveRecord::Base
   def self.get_for_user(user)
 
     return [] if user.nil?
 
     toys = Toy.where("user_id=#{user.id}").to_a
-    deleted_ary = []
+    deleted_arr = []
 
     return [] if toys.nil?
 
     toys.each do |toy|
       case toy.xtype
         when Toy::XTYPE_ITEM
           item = Item.find_by_id(toy.target_id)
           if item.nil?
-            deleted_ary << toy
+            deleted_arr << toy
             next
           end
           Toy.copy(toy, item)
 
         when Toy::XTYPE_COMMENT
           comment = Comment.find_by_id(toy.target_id)
           if comment.nil?
-            deleted_ary << toy
+            deleted_arr << toy
             next
           end
           Toy.copy(toy, comment)
 
         when Toy::XTYPE_WORKFLOW
           workflow = Workflow.find_by_id(toy.target_id)
           if workflow.nil?
-            deleted_ary << toy
+            deleted_arr << toy
             next
           end
           Toy.copy(toy, workflow)
 
         when Toy::XTYPE_SCHEDULE
           schedule = Schedule.find_by_id(toy.target_id)
           if schedule.nil?
-            deleted_ary << toy
+            deleted_arr << toy
             next
           end
           Toy.copy(toy, schedule)
 
         when Toy::XTYPE_FOLDER
           folder = Folder.find_by_id(toy.target_id)
           if folder.nil?
-            deleted_ary << toy
+            deleted_arr << toy
             next
           end
           Toy.copy(toy, folder)
       end
     end
 
-    deleted_ary.each do |toy|
-      toys.delete toy
-      Toy.destroy toy.id
+    deleted_arr.each do |toy|
+      toys.delete(toy)
+      Toy.destroy(toy.id)
     end
 
     return toys
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index bf4f46bc..f620221c 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1129,11 +1129,15 @@ class User < ActiveRecord::Base
     unless self.groups.nil? or self.groups.empty?
 
       if (/^|([0-9]+|)+$/ =~ self.groups) == 0
 
         self.get_groups_a.each do |group_id|
-          group = Group.find_by_id(group_id)
+          begin
+            group = Group.find(group_id)
+          rescue => evar
+            group = nil
+          end
           if group.nil?
             err_msgs << I18n.t('user.import.not_valid_groups') + ': '+group_id.to_s
             break
           end
         end
diff --git a/app/views/mail_filters/_mail_filter_form.html.erb b/app/views/mail_filters/_mail_filter_form.html.erb
index 3af47ef1..184d6446 100644
--- a/app/views/mail_filters/_mail_filter_form.html.erb
+++ b/app/views/mail_filters/_mail_filter_form.html.erb
@@ -4,11 +4,12 @@ params[:mail_filter] ||= {}
 if params[:mail_account_id].blank?
   mail_account_id = @mail_filter.mail_account_id unless @mail_filter.nil?
 else
   mail_account_id = params[:mail_account_id]
 end
-mail_account = MailAccount.find_by_id(mail_account_id)
+SqlHelper.validate_token([mail_account_id])
+mail_account = MailAccount.find(mail_account_id)
 %>
 
 <%= render(:partial => 'common/error', :locals => {:obj => @mail_filter}) %>
 
 <table style="width:100%; margin-top:5px;" cellspacing="0" cellpadding="0">

Vulnerable code:     if options[:count]
      else
        total = model.count_by_sql(options[:count])
    else
      total = model.count_by_sql_wrapping_select_query(sql)
    #objects = model.find_by_sql_with_limit(sql, object_pages.current.to_sql[1], per_page)
    @login_user = User.find_by_id(session[:login_user_id])
    def self.find_by_sql_with_limit(sql, offset, limit)
      sql = sanitize_sql(sql)
      add_limit!(sql, {:limit => limit, :offset => offset})
      find_by_sql(sql)
    end

    @mail_filter = MailFilter.find_by_id(params[:id])
    mail_account = MailAccount.find_by_id(params[:mail_account_id])
    mail_folder = MailFolder.find_by_id(params[:mail_folder_id])
    @mail_account = MailAccount.find_by_id(mail_account_id)
    order_ary = params[:mail_filters_order]
    @mail_account = MailAccount.find_by_id(mail_account_id)
      idx_a = order_ary.index(id_a)
      idx_b = order_ary.index(id_b)
    email = Email.find_by_id(email_id)
    mail_attach = MailAttachment.find_by_id(attached_id)
    email = Email.find_by_id(mail_attach.email_id)
        email = Email.find_by_id(email_id)
    mail_folder = MailFolder.find_by_id(folder_id)
    order_ary = params[:folders_order]
      idx_a = order_ary.index(id_a)
      idx_b = order_ary.index(id_b)
    mail_folder = MailFolder.find_by_id(mail_folder_id)
    unless imp_id.nil? or imp_id.empty?
          group = Group.find_by_id(group_id)
          team = Team.find_by_id(team_id)
    attachments = [] if attachments.nil?
    return attachments

      mail_account = MailAccount.find_by_id(email.mail_account_id)
    src_item = Item.find_by_id(self.source_id)
    deleted_ary = []
            deleted_ary << toy
            deleted_ary << toy
            deleted_ary << toy
            deleted_ary << toy
            deleted_ary << toy
    deleted_ary.each do |toy|
      toys.delete toy
      Toy.destroy toy.id
          group = Group.find_by_id(group_id)
mail_account = MailAccount.find_by_id(mail_account_id)
Fixed code:     if options[:count].blank?
      total = model.count_by_sql_wrapping_select_query(sql)
    else
      #else
      #  total = model.count_by_sql(options[:count])
    SqlHelper.validate_token([params['page']])
    @login_user = User.find(session[:login_user_id])
    begin
      @mail_filter = MailFilter.find(params[:id])
    rescue => evar
    end
    mail_account = MailAccount.find(params[:mail_account_id])
    mail_folder = MailFolder.find(params[:mail_folder_id])
    SqlHelper.validate_token([mail_account_id])
    @mail_account = MailAccount.find(mail_account_id)
    order_arr = params[:mail_filters_order]

    SqlHelper.validate_token([mail_account_id])
    @mail_account = MailAccount.find(mail_account_id)
      idx_a = order_arr.index(id_a)
      idx_b = order_arr.index(id_b)
    email = Email.find(email_id)
    begin
      mail_attach = MailAttachment.find(attached_id)
    rescue => evar
    end
    begin
      email = Email.find(mail_attach.email_id)
    rescue => evar
    end
        begin
          email = Email.find(email_id)
        rescue => evar
        end
    SqlHelper.validate_token([folder_id])
    begin
      mail_folder = MailFolder.find(folder_id)
    rescue => evar
    end
    order_arr = params[:folders_order]
    SqlHelper.validate_token([params[:id]])
      idx_a = order_arr.index(id_a)
      idx_b = order_arr.index(id_b)
      SqlHelper.validate_token([folder_id])
    SqlHelper.validate_token([mail_folder_id])
    begin
      mail_folder = MailFolder.find(mail_folder_id)
    rescue => evar
    end
    SqlHelper.validate_token([imp_id])
    unless imp_id.blank?
          begin
            group = Group.find(group_id)
          rescue => evar
            group = nil
          end
          begin
            team = Team.find(team_id)
          rescue => evar
            team = nil
          end
    return (attachments || [])
      mail_account = MailAccount.find(email.mail_account_id)
    begin
      src_item = Item.find(self.source_id)
    rescue => evar
      src_item = nil
    end
    deleted_arr = []
            deleted_arr << toy
            deleted_arr << toy
            deleted_arr << toy
            deleted_arr << toy
            deleted_arr << toy
    deleted_arr.each do |toy|
      toys.delete(toy)
      Toy.destroy(toy.id)
          begin
            group = Group.find(group_id)
          rescue => evar
            group = nil
          end
SqlHelper.validate_token([mail_account_id])
mail_account = MailAccount.find(mail_account_id)