Rails ActriveRecordとSQLインジェクションと実際のアプリケーション

先日はActive RecordのSQLインジェクションパターンを紹介しました。今回は脆弱なコードを見つける事を試みようと思います。脆弱とは言っても攻撃可能であることは意味しません。コーディングとして脆弱であるという意味です。実際に攻撃可能であるかどうかまでは確認していません。

サンプルプログラム

実例ということでRailsアプリケーションが必要なので検索して、OpenProjectと呼ばれるRailsアプリケーションを利用しています。OpenProjectはOffice Projectのようなプロジェクト管理用のオープンソースWebアプリケーションです。GitHubでソースコードが公開されています。今回はそのコードを利用しています。

https://github.com/opf/openproject

コードの確認

今回はActiveRecordの識別子の使い方がSQLインジェクションに脆弱になるコードの部分に焦点を絞り、ざっと簡単に確認しました。ActiveRecord関連以外にも気になる部分が幾つか見つかりましたが、省略しています。またソースコード全てを確認した訳ではありません。

app/models/news.rb

!user.nil? && user.allowed_to?(:view_news, project)
end
# returns latest news for projects visible by user
def self.latest(user = User.current, count = 5)
  find(:all, :limit => count, :conditions => Project.allowed_to_condition(user, :view_news), :include => [ :author, :project ], :order => "#{News.table_name}.created_on DESC")
end
def self.latest_for(user, options = {})
  limit = options.fetch(:count) { 5 }

:order => "#{News.table_name}.created_on DESC"

の部分はテーブル名となるNewsのtable_nameを直接文字列埋め込みを行っています。table_nameは普通は正しいテーブル名のハズなので、このような使い方でも大丈夫です。しかし、こういう書き方が一般的であると、誤って外部入力を埋め込んでしまうプログラマが居てもおかしくありません。

app/controllers/projects_controller.rb

cond = @project.project_condition(Setting.display_subprojects_work_packages?)
  @open_issues_by_type = WorkPackage.visible.count(:group => :type,
                                          :include => [:project, :status, :type],
                                          :conditions => ["(#{cond}) AND #{Status.table_name}.is_closed=?", false])
  @total_issues_by_type = WorkPackage.visible.count(:group => :type,
                                          :include => [:project, :status, :type],
                                          :conditions => cond)
  if User.current.allowed_to?(:view_time_entries, @project)

Status.table_nameが直接埋め込みで利用されています。この部分ではcondも直接埋め込まれています。

app/controllers/time_entries/reports_controller.rb

    sql << " WHERE"
    sql << " (%s) AND" % sql_condition
    sql << " (spent_on BETWEEN '%s' AND '%s')" % [ActiveRecord::Base.connection.quoted_date(@from), ActiveRecord::Base.connection.quoted_date(@to)]
    sql << " GROUP BY #{sql_group_by}, tyear, tmonth, tweek, spent_on"
    @hours = ActiveRecord::Base.connection.select_all(sql)
    @hours.each do |row|
      case @columns
      when 'year'
        row['year'] = row['tyear']

    @hours = ActiveRecord::Base.connection.select_all(sql)

の部分のsqlの文字列は貼り付けたコードから分かるようにSQLのWHERE条件です。GROUP BYの条件にsql_group_byを埋め込んでいます。sql_group_byの中身がどうなっているか、確認しないと安全であるかどうかは分かりません。

以下、識別子埋め込みを行っている部分のみを貼り付けます。

app/models/project.rb

end
# Deletes all project's members
def delete_all_members
  me, mr = Member.table_name, MemberRole.table_name
  connection.delete("DELETE FROM #{mr} WHERE #{mr}.member_id IN (SELECT #{me}.id FROM #{me} WHERE #{me}.project_id = #{id})")
  Member.delete_all(['project_id = ?', id])
end
def destroy_all_work_packages
  self.work_packages.each do |wp|

app/models/repository.rb

    true
  end
  def clear_changesets
    cs, ch, ci = Changeset.table_name, Change.table_name, "#{table_name_prefix}changesets_work_packages#{table_name_suffix}"
    connection.delete("DELETE FROM #{ch} WHERE #{ch}.changeset_id IN (SELECT #{cs}.id FROM #{cs} WHERE #{cs}.repository_id = #{id})")
    connection.delete("DELETE FROM #{ci} WHERE #{ci}.changeset_id IN (SELECT #{cs}.id FROM #{cs} WHERE #{cs}.repository_id = #{id})")
    connection.delete("DELETE FROM #{cs} WHERE #{cs}.repository_id = #{id}")
  end
end

app/models/user.rb

  has_many :memberships, :class_name => 'Member', :foreign_key => 'user_id', :include => [ :project, :roles ], :conditions => "#{Project.table_name}.status=#{Project::STATUS_ACTIVE}", :order => "#{Project.table_name}.name"
  has_many :projects, :through => :memberships
  # Active non-anonymous users scope
  scope :not_builtin,
        :conditions => "#{User.table_name}.status <> #{STATUSES[:builtin]}"
  # Users blocked via brute force prevention
  # use lambda here, so time is evaluated on each query
  scope :blocked, lambda { create_blocked_scope(true) }
  scope :not_blocked, lambda { create_blocked_scope(false) }

app/models/work_package.rb

    ActiveRecord::Base.connection.select_all("select    s.id as status_id,
                                                s.is_closed as closed,
                                                i.project_id as project_id,
                                                count(i.id) as total
                                              from
                                                #{WorkPackage.table_name} i, #{Status.table_name} s
                                              where
                                                i.status_id=s.id
                                                and i.project_id IN (#{project.descendants.active.collect{|p| p.id}.join(',')})
                                              group by s.id, s.is_closed, i.project_id") if project.descendants.active.any?
  end

まとめ

Active Recordオブジェクトのtable_nameは正規のテーブル名を保存しているはずなので、普通はセキュリティ上の問題にはなりません。しかし、しっかりコード検査をした訳ではありませんが、ざっと斜め読みしただけで、目的の変数による識別子埋め込み、問題となるかも？と思える箇所も見つけることができました。

このアプリの場合、結構気軽にSQL文に変数を直接埋め込んでいるようなので、詳しく調べるとSQLインジェクションに脆弱な部分も見つかるかも？と思えるコードでした。他にもマスアサイメントは大丈夫なのか？ビューのパスが大丈夫なのか？と思える部分もありました。

気軽に変数を埋め込んでいるコードになっているので、もし初心者このようなコードを参考にアプリケーションを作った場合、危険なコードになってしまう可能性が高いと思います。

堅牢なSQLデータベース操作を行うには、HTMLのビューのセキュリティ対策などと同じく、デフォルトで必要なエスケープ・バリデーションを無条件で適用します。無条件でセキュリティ対策を適用することにより、勘違いなどによるセキュリティホール（攻撃可能な脆弱性）を生むリスクを低減できます。

エスケープ処理を省略したい場合、明らかに安全であることを明示するため「XXXは安全なテーブル名。変数埋め込み」となどとコメントを書いておくのも良いと思います。