Brakeman を欺く - Kashiwa.rb #4

Transcript

1. Brakeman を欺く 2024.10.21 Kahiwa.rb #4 生活発表会 Koji NAKAMURA (@kozy4324)
2. Koji NAKAMURA • 𝕏: @kozy4324 • GitHub: @kozy4324 • Classi株式会社所属 • Kashiwa.rb主催 自己紹介
3. 近況報告 この1ヶ月何してた？ • Kaigi on Rails 2024 の登壇準備 • （それ以外の記憶があんまりないな...）
4. KoR パーカー も購入したよ 発送が間に合ってないけどな！
7. 今日の話 • Kaigi on Rails の登壇で Brakeman について喋ろうと思った内容は時間の都合で 大幅カットした • 今日ここで供養したい
8. https://brakemanscanner.org/
9. Brakeman とは？ • Ruby on Rails Static Analysis Security Tool • Rails 7.2 からはデフォルトで含まれる ◦ rails new すると Gemfile に含まれる • コードをスキャンして脆弱性疑いのある実装をレポートしてくれる • SQL インジェクション脆弱性以外にも多くの脆弱性をチェックしてくれる ◦ e.g.) XSS, コマンドインジェクション , オープンリダイレクト , etc…
10. Brakeman で SQL インジェクションを検知する class UsersController < ApplicationController def index @exists = User.exists?(params[:id]) end end app/controllers/users_controller.rb
11. == Brakeman Report == Application Path: /Users/kozy4324/work/my-rails-app Rails Version: 7.1.4 Brakeman Version: 6.2.1 Scan Date: 2024-10-20 11:18:41 +0900 Duration: 0.497845 seconds Checks Run: SQL == Overview == Controllers: 2 Models: 3 Templates: 3 Errors: 0 Security Warnings: 1 == Warning Types == SQL Injection: 1 == Warnings == Confidence: High Category: SQL Injection Check: SQL Message: Possible SQL injection Code: User.exists?(params[:id]) File: app/controllers/users_controller.rb Line: 3 $ brakeman -t SQL の出力結果
12. 本題: Brakeman を欺く？ • ドキュメントより ◦ > Only the developers of an application can understand if certain values are dangerous or not. By default, Brakeman is extremely suspicious. This can lead to many “false positives.” • SQL インジェクションを検知するロジックをリバースエンジニアリング • どういった実装をすると誤検知になるか（＝欺けるか）を確認する
13. Brakeman のチェックする実装を確認する (1) $ rdbg -e "b Brakeman::FileParser#parse_ruby" -e "open vscode" -c -- brakeman --no-threads -t SQL brakeman を rdbg で起動する def parse_ruby input, path if path.is_a? Brakeman ::FilePath path = path.relative end Brakeman .debug "Parsing #{ path}" if @use_prism begin parse_with_prism input, path rescue => e Brakeman .debug "Prism failed to parse #{ path}: #{e}" parse_with_ruby_parser input, path end else parse_with_ruby_parser input, path end end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/file_parser.rb#L82
14. Brakeman のチェックする実装を確認する (2) def parse_with_ruby_parser input, path begin RubyParser .new.parse input, path, @timeout rescue Racc::ParseError => e raise e.exception(e.message + "\nCould not parse #{ path}") rescue Timeout::Error => e raise Exception .new("Parsing #{ path} took too long (> #{ @timeout } seconds). Try increasing the limit with --parser-timeout" ) rescue => e raise e.exception(e.message + "\nWhile processing #{ path}") end end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/file_parser.rb#L108
15. def run_check # Avoid reporting `user_input` on silly values when generating warning. # Note that we retroactively find `user_input` inside the "dangerous" value. @safe_input_attributes .merge IGNORE_METHODS_IN_SQL @sql_targets = [:average, :calculate , :count, :count_by_sql , :delete_all , :destroy_all , :find_by_sql , :maximum, :minimum, :pluck, :sum, :update_all ] @sql_targets .concat [:from, :group, :having, :joins, :lock, :order, :reorder, :where] if tracker.options[:rails3] @sql_targets .concat [:find_by, :find_by!, :find_or_create_by , :find_or_create_by! , :find_or_initialize_by , :not] if tracker.options[:rails4] if tracker.options[:rails6] @sql_targets .concat [:delete_by , :destroy_by , :rewhere, :reselect] @sql_targets .delete :delete_all @sql_targets .delete :destroy_all end if version_between?( "6.1.0", "9.9.9") @sql_targets .delete :order @sql_targets .delete :reorder @sql_targets .delete :pluck end Brakeman のチェックする実装を確認する (3) https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L21 $ rdbg -e "b Brakeman::CheckSQL#run_check" -e "open vscode" -c -- brakeman --no-threads -t SQL 改めて brakeman を rdbg で起動する
16. Brakeman のチェックする実装を確認する (4) @expected_targets = active_record_models. keys + [:connection, :"ActiveRecord::Base" , :Arel] Brakeman.debug "Finding possible SQL calls on models" calls = tracker.find_call(:methods => @sql_targets , :nested => true) narrow_targets = [:exists?, :select] calls.concat tracker.find_call(:targets => active_record_models. keys, :methods => narrow_targets, :chained => true) Brakeman.debug "Finding possible SQL calls with no target" calls.concat tracker.find_call(:target => nil, :methods => @sql_targets ) Brakeman.debug "Finding possible SQL calls using constantized()" calls.concat tracker.find_call(:methods => @sql_targets ).select { |result| constantize_call? result } https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L56
17. dangerous_value = case method when :find check_find_arguments call. second_arg when :exists? check_exists call. first_arg when :delete_all , :destroy_all check_find_arguments call. first_arg when :named_scope , :scope check_scope_arguments call when :find_by_sql , :count_by_sql check_by_sql_arguments call. first_arg when :calculate check_find_arguments call. third_arg when :last, :first, :all check_find_arguments call. first_arg when :average , :count, :maximum , :minimum , :sum if call.length > 5 unsafe_sql?(call. first_arg) or check_find_arguments(call. last_arg) else check_find_arguments call. last_arg end when :where, :rewhere , :having, :find_by , :find_by! , :find_or_create_by , :find_or_create_by! , :find_or_initialize_by ,:not, :delete_by , :destroy_by check_query_arguments call. arglist Brakeman のチェックする実装を確認する (5) https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L179
18. if dangerous_value add_result result input = include_user_input? dangerous_value if input confidence = :high user_input = input else confidence = :medium user_input = dangerous_value end if result[ :call].target and result[ :chain] and not @expected_targets .include? result[ :chain].first confidence = case confidence when :high :medium when :medium :weak else confidence end end Brakeman のチェックする実装を確認する (6) https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L222
19. ここまでのまとめ • ruby_parser gem で AST（抽象構文木）を作ってそれをチェックしている • 特定のクエリメソッド呼び出し有無をチェック ◦ 特定のメソッドはレシーバを見ているが、基本的にレシーバ関係なく収集 • クエリメソッドの呼び出し別に dangerous_value の有無をチェック • dangerous_value がある場合 ◦ dangerous_value にユーザーインプットが含まれていると confidence が High 、含まれていなけれ ば Medium ◦ メソッドチェーンで、かつ先頭がターゲット（モデルクラス、 :connection、Arel）でない場合は confidence のレベルを一つ下げる ( High → Medium, Medium → weak )
20. Brakeman のチェックする実装を確認する (7) #Checks the given expression for unsafe SQL values. If an unsafe value is #found, returns that value (may be the given _exp_ or a subexpression). # #Otherwise, returns false/nil. def unsafe_sql? exp, ignore_hash = false return unless sexp?(exp) dangerous_value = find_dangerous_value exp, ignore_hash safe_value?(dangerous_value) ? false : dangerous_value end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L447
21. Brakeman のチェックする実装を確認する (8) #Check _exp_ for dangerous values. Used by unsafe_sql? def find_dangerous_value exp, ignore_hash case exp.node_type when :lit, :str, :const, :colon2, :true, :false, :nil nil : (中略) : when :call unless IGNORE_METHODS_IN_SQL .include? exp.method if has_immediate_user_input? exp exp elsif TO_STRING_METHODS .include? exp.method find_dangerous_value exp. target, ignore_hash else check_call exp end end : end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L458
22. Brakeman のチェックする実装を確認する (9) def safe_value? exp return true unless sexp? exp case exp.node_type when :str, :lit, :const, :colon2, :nil, :true, :false true when :call if exp.method == :to_s or exp.method == :to_sym safe_value? exp. target else ignore_call? exp end when :if safe_value? exp. then_clause and safe_value? exp. else_clause when :block, :rlist safe_value? exp. last when :or safe_value? exp. lhs and safe_value? exp. rhs when :dstr not unsafe_string_interp? exp else false end end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L601
23. Brakeman のチェックする実装を確認する (10) def ignore_call? exp return unless call? exp ignore_methods_in_sql. include? exp.method or quote_call? exp or arel? exp or exp. method.to_s.end_with? "_id" or number_target? exp or date_target? exp or locale_call? exp end https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L626 IGNORE_METHODS_IN_SQL = Set[:id, :merge_conditions , :table_name , :quoted_table_name , :quoted_primary_key , :to_i, :to_f, :sanitize_sql , :sanitize_sql_array , :sanitize_sql_for_assignment , :sanitize_sql_for_conditions , :sanitize_sql_hash , :sanitize_sql_hash_for_assignment , :sanitize_sql_hash_for_conditions , :to_sql, :sanitize , :primary_key , :table_name_prefix , :table_name_suffix , :where_values_hash , :foreign_key , :uuid, :escape, :escape_string ] https://github.com/presidentbeef/brakeman/blob/v6.2.2/lib/brakeman/checks/check_sql.rb#L589
24. だいたいわかった
25. 期待する Brakeman の出力（脆弱性の問題がある） class UsersController < ApplicationController def index @users = User.where("name = '#{params[:cond]}'") end end app/controllers/users_controller.rb == Warnings == Confidence: High Category: SQL Injection Check: SQL Message: Possible SQL injection Code: User.where("name = '#{params[:cond]}'") File: app/controllers/users_controller.rb Line: 3 $ brakeman -t SQL の出力結果
26. Brakeman を欺く - 式展開する値をメソッド経由にする class UsersController < ApplicationController def index @users = User.where("name = '#{cond}'") end def cond params[:cond] end end app/controllers/users_controller.rb == Warnings == Confidence: Medium Category: SQL Injection Check: SQL Message: Possible SQL injection Code: User.where("name = '#{cond}'") File: app/controllers/users_controller.rb Line: 3 $ brakeman -t SQL の出力結果 Confidence が Medium になる
27. Brakeman を欺く - 引数をメソッド経由にする class UsersController < ApplicationController def index @users = User.where(sql) end def sql "name = '#{params[:cond]}'" end end app/controllers/users_controller.rb == Warnings == No warnings found $ brakeman -t SQL の出力結果
28. 期待する Brakeman の出力（脆弱性の問題はない） class UsersController < ApplicationController def index @val = params["sort_by_#{params[:attr]}"] end end app/controllers/users_controller.rb == Warnings == No warnings found $ brakeman -t SQL の出力結果
29. == Warnings == Category: SQL Injection Check: SQL Message: Possible SQL injection Code: find_by("sort_by_#{params[:attr]}") File: app/controllers/users_controller.rb Line: 3 Brakeman を欺く - クエリメソッドと I/F を被らせる class UsersController < ApplicationController def index @val = find_by "sort_by_#{params[:attr]}" end def find_by key params[key] end end app/controllers/users_controller.rb $ brakeman -t SQL の出力結果
30. まとめ • Brakeman の精度 ◦ 怪しい実装はレポートするスタンスをとっている ◦ 偽陽性が多く出る分に関しては仕方がないのではないだろうか ◦ 検知が漏れるよりも100倍マシ（だと思う） • 素朴に普通に Rails の実装をしよう ◦ あまりにもトリッキーなことをすると Brakeman も脆弱性を検知できないぞ！ • AST（抽象構文木）の取り扱う実装を眺めてみて ◦ ruby_parser gem の前提知識が足りないので深掘りしたくなった ◦ ただし時代は Prism パーサーでは？ ◦ Brakeman も 6.2.1 で Prism サーバーに切り替えるオプションが入っている ◦ Brakeman の今後の Prism パーサー対応も追いかけていこう
31. EOF