New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn unless a rails 4 app protect[s]_from_forgery with :exception #648

Merged
merged 3 commits into from Apr 30, 2015

Conversation

Projects
None yet
2 participants
@oreoshake
Contributor

oreoshake commented Apr 14, 2015

Because this has bitten me twice before and the effect is basically catastrophic, let's warn people when they don't use protect_from_forgery with: :exception. This is now the default for newly generated rails 4 applications and so it should be considered the standard.

Show outdated Hide outdated lib/brakeman/checks/check_forgery_setting.rb
@@ -52,6 +52,17 @@ def run_check
:confidence => CONFIDENCE[:high],
:gem_info => gemfile_or_environment,
:link_path => "https://groups.google.com/d/topic/rubyonrails-security/LZWjzCPgNmU/discussion"
elsif version_between? "4.0.0", "100.0.0" and forgery_opts = app_controller[:options][:protect_from_forgery]
unless forgery_opts.is_a?(Array) and sexp?(forgery_opts.first) and hash_access(forgery_opts.first.first_arg, :with).value == :exception

This comment has been minimized.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

It's possible hash_access(forgery_opts.first.first_arg, :with) will return nil.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

It's possible hash_access(forgery_opts.first.first_arg, :with) will return nil.

This comment has been minimized.

@oreoshake

oreoshake Apr 16, 2015

Contributor

fixed in 12ed187

@oreoshake

oreoshake Apr 16, 2015

Contributor

fixed in 12ed187

Show outdated Hide outdated lib/brakeman/checks/check_forgery_setting.rb
:message => "protect_from_forgery should be configured with 'with: :exception'",
:confidence => CONFIDENCE[:med],
:file => app_controller[:files].first,
:link_path => "blog post link?"

This comment has been minimized.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Or update http://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/ and link to external blog posts from there. Then just remove :link_path.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Or update http://brakemanscanner.org/docs/warning_types/cross-site_request_forgery/ and link to external blog posts from there. Then just remove :link_path.

This comment has been minimized.

Show outdated Hide outdated lib/brakeman/checks/check_forgery_setting.rb
elsif version_between? "4.0.0", "100.0.0" and forgery_opts = app_controller[:options][:protect_from_forgery]
unless forgery_opts.is_a?(Array) and sexp?(forgery_opts.first) and hash_access(forgery_opts.first.first_arg, :with).value == :exception
warn :controller => :ApplicationController,

This comment has been minimized.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Pass in forgery_opts.first as :code? Then we'd also get the line number.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Pass in forgery_opts.first as :code? Then we'd also get the line number.

This comment has been minimized.

@oreoshake

oreoshake Apr 16, 2015

Contributor

done 12ed187. sometimes the value is a boolean so there's some code to handle that.

@oreoshake

oreoshake Apr 16, 2015

Contributor

done 12ed187. sometimes the value is a boolean so there's some code to handle that.

@@ -87,6 +87,7 @@ module Brakeman::WarningCodes
:CVE_2011_2932 => 83,
:cross_site_scripting_inline => 84,
:CVE_2014_7829 => 85,
:csrf_not_protected_by_raising_exception => 86,

This comment has been minimized.

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Thanks for adding this!

@presidentbeef

presidentbeef Apr 14, 2015

Owner

Thanks for adding this!

presidentbeef added a commit that referenced this pull request Apr 30, 2015

Merge pull request #648 from oreoshake/check-for-csrf-protection-by-e…
…xception

Warn unless a rails 4 app protect[s]_from_forgery with :exception

@presidentbeef presidentbeef merged commit 141936e into presidentbeef:master Apr 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Repository owner locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.