From efd6938e3095f04d4d3477a10a285847076fdc7c Mon Sep 17 00:00:00 2001 From: Jason Frey Date: Fri, 2 Aug 2024 11:04:12 -0400 Subject: [PATCH] Upgrade brakeman to v6 --- Gemfile | 2 +- config/brakeman.ignore | 32 +++++++-- lib/extensions/brakeman_fingerprint_patch.rb | 70 ++++++++++++++++++++ lib/tasks/test_security_helper.rb | 5 ++ 4 files changed, 103 insertions(+), 6 deletions(-) create mode 100644 lib/extensions/brakeman_fingerprint_patch.rb diff --git a/Gemfile b/Gemfile index de511f79f44..d594eccd432 100644 --- a/Gemfile +++ b/Gemfile @@ -303,7 +303,7 @@ group :development do end group :test do - gem "brakeman", "~>5.4", :require => false + gem "brakeman", "~>6.2", :require => false gem "bundler-audit", :require => false gem "capybara", "~>2.5.0", :require => false gem "db-query-matchers", "~>0.11.0" diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 3f10fbec000..502902b6c94 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -1,20 +1,42 @@ { "ignored_warnings": [ + { + "warning_type": "Cross-Site Request Forgery", + "warning_code": 86, + "fingerprint": "6301c055d2b1a4bc467bcd405b0ba295893f71df183eae355cd1a8b6c0ed0588", + "check_name": "ForgerySetting", + "message": "`protect_from_forgery` should be configured with `with: :exception`", + "file": "(engine:manageiq-ui-classic) app/controllers/application_controller.rb", + "line": 13, + "link": "https://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/", + "code": "protect_from_forgery(:secret => SecureRandom.hex(64), :except => ([:authenticate, :external_authenticate, :kerberos_authenticate, :saml_login, :initiate_saml_login, :oidc_login, :initiate_oidc_login, :csp_report]), :with => :reset_session)", + "render_path": null, + "location": { + "type": "controller", + "controller": "ApplicationController" + }, + "user_input": null, + "confidence": "Medium", + "cwe_id": [ + 352 + ], + "note": "This was intentionally changed from :exception to :reset_session in ManageIQ/manageiq-ui-classic#4901" + }, { "warning_type": "Command Injection", "warning_code": 14, - "fingerprint": "1d1f63511c528d6d28a292a8b3ff2a91b5e0c4104112fb06631a00ca89f03e2f", + "fingerprint": "9a58ac820e59b1edb4530e27646edc1f328915a7a356d987397659b48c52239e", "check_name": "Execute", "message": "Possible command injection", "file": "lib/ansible/runner.rb", - "line": 409, + "line": 422, "link": "https://brakemanscanner.org/docs/warning_types/command_injection/", "code": "`python#{version} -c 'import site; print(\":\".join(site.getsitepackages()))'`", "render_path": null, "location": { "type": "method", "class": "Ansible::Runner", - "method": "ansible_python_paths_raw" + "method": "s(:self).ansible_python_paths_raw" }, "user_input": "version", "confidence": "Medium", @@ -24,6 +46,6 @@ "note": "This method is safe because it verifies that the version is in the form #.#." } ], - "updated": "2023-03-20 10:30:55 -0400", - "brakeman_version": "5.4.1" + "updated": "2024-09-11 16:34:41 -0400", + "brakeman_version": "6.2.1" } diff --git a/lib/extensions/brakeman_fingerprint_patch.rb b/lib/extensions/brakeman_fingerprint_patch.rb new file mode 100644 index 00000000000..dab14a39a33 --- /dev/null +++ b/lib/extensions/brakeman_fingerprint_patch.rb @@ -0,0 +1,70 @@ +# Brakeman fingerprints account for the file location as part of the fingerprint +# digest. The fingerprint uses the file's relative path, but assumes that all +# files being scanned will be under the app_tree root directory, providing a +# consistent file path regardless of the system it is run on. +# +# For engines, however this is not always the case. Engines can come from gems +# and gems can be installed basically anywhere on the system depending on how +# ruby is installed and configured, and even depending on which Ruby version +# manager is used. Additionally, in CI gems are typically installed in a vendor +# directory, and locally gems can be configured as git-based or path-based. +# Because of this, the file path and the fingerprint can vary widely between +# local dev and CI, causing problems when trying to ignore issues. For example, +# +# git-based engine locally | ../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb +# git-based engine on CI | vendor/bundle/ruby/3.0.0/bundler/gems/manageiq-ui-classic-df1d9535ef51/app/controllers/application_controller.rb +# path-based engine locally | ../manageiq-ui-classic/app/controllers/application_controller.rb +# version-based engine locally | ../../.gem/ruby/3.1.5/bundler/gems/manageiq-ui-classic-0.1.0/app/controllers/application_controller.rb +# +# This patch introduces a way to "remove" the leading path for files that reside +# in engines. This removal provides a consistent file path for the fingerprint +# method. For example, all of the above will appear like +# +# (engine:manageiq-ui-classic) app/controllers/application_controller.rb +# +# NOTE: This patch only modifies what is necessary to make the +# test:security:brakeman test suite work, namely fingerprint and the json +# reporter (which leverages to_hash). It does not modify things such as the +# the text reporter (CLI output) nor the interactive ignore. +module BrakemanFingerprintPatch + def self.rails_engine_paths + @rails_engine_paths ||= ::Rails::Engine.subclasses.map { |e| e.root.to_s << "/" } + end + + # Removes any leading engine paths if the file is an engine, and replaces with + # `(engine:)` + # + # NOTE: Ideally this code should use the in_engine_paths? method (that is + # patched in brakeman_excludes_patch.rb), however Brakeman::Warning doesn't + # have a reference to the app_tree where that method is defined, as warnings + # are standalone objects. + def file_string + engine_path = BrakemanFingerprintPatch.rails_engine_paths.detect { |p| self.file.absolute.start_with?(p) } + if engine_path + engine_name = File.basename(engine_path).sub(/-\h+$/, "").sub(/-(?:\d+\.)+\d+$/, "") + engine_relative = self.file.absolute.sub(engine_path, "") + "(engine:#{engine_name}) #{engine_relative}" + else + self.file.relative + end + end + + # This method is copied from brakeman (https://github.com/presidentbeef/brakeman/blob/e4f49f64d263f8001bac62eec182ad417152776d/lib/brakeman/warning.rb#L250-L257) + # in order to modify the file_string component of the digest to account for engine support. + def fingerprint + loc = self.location + location_string = loc && loc.sort_by { |k, v| k.to_s }.inspect + warning_code_string = sprintf("%03d", @warning_code) + code_string = @code.inspect + + Digest::SHA2.new(256).update("#{warning_code_string}#{code_string}#{location_string}#{file_string}#{self.confidence}").to_s + end + + # This method overrides the .to_hash method from brakeman (https://github.com/presidentbeef/brakeman/blob/e4f49f64d263f8001bac62eec182ad417152776d/lib/brakeman/warning.rb#L288-L310) + # in order to modify the file value to account for engine support. + def to_hash(absolute_paths: true) + super.tap do |h| + h[:file] = (absolute_paths ? self.file.absolute : file_string) + end + end +end diff --git a/lib/tasks/test_security_helper.rb b/lib/tasks/test_security_helper.rb index 30a6e4db615..aca8d9ad20a 100644 --- a/lib/tasks/test_security_helper.rb +++ b/lib/tasks/test_security_helper.rb @@ -12,6 +12,11 @@ def self.brakeman(format: "human") require Rails.root.join('lib/extensions/brakeman_excludes_patch') Brakeman::AppTree.prepend(BrakemanExcludesPatch) + # Brakeman's fingerprint check does not work properly with engines + require "brakeman/warning" + require Rails.root.join('lib/extensions/brakeman_fingerprint_patch') + Brakeman::Warning.prepend(BrakemanFingerprintPatch) + app_path = Rails.root.to_s engine_paths = Vmdb::Plugins.paths.except(ManageIQ::Schema::Engine).values