Catch raw hrefs as bad, warn unless wrapped in a "safe" method #45
Conversation
Add concept of a safe-ening method to mark hrefs as safe
Feature:
Warn when using unsafe hrefs. This is a very specific case that as of now produces a ton of noise. This came out of an xss vuln where the value was escaped but still vulnerable.
link_to 'asdf', h(@scary)
where
@scary = 'javascript:alert(1)'
or
@scary = 'data: # http://palpapers.plynt.com/issues/2010Oct/bypass-xss-filters/
This branch accomplishes slightly intelligent warnings by adding a new command line option to declare methods that make a string URL safe (unless there is already a standard one out there). e.g.:
$ brakeman . --url-safe-methods ensure_valid_protocol!
link_to 'asdf', ensure_valid_protocol!(@scary, :javascript)
|
Bah, found another class of FPs. Will fix. |
| :submit_tag, :text_area, :text_field, | ||
| :text_field_tag, :url_encode, :url_for, | ||
| :will_paginate] ).merge tracker.options[:safe_methods] | ||
| @ignore_methods = IGNORE_METHODS.merge tracker.options[:safe_methods] |
There was a problem hiding this comment.
Merge modifies the original set, so this isn't going to work out right.
|
I'm happy with this for now. |
Catch raw hrefs as bad, warn unless wrapped in a "safe" method
| call = result[:call] = result[:call].dup | ||
| @matched = false | ||
| url_arg = process call[3][2] | ||
| return if sexp?(url_arg) && url_arg.node_type == :string_interp && !url_arg[1].chomp.empty? |
There was a problem hiding this comment.
The idea is that if you are doing string interpolation, you know what you are doing?
There was a problem hiding this comment.
So "http://safe.com/#{param}" is safe but "#{bad}/stuff" is not. Safe?
On Feb 22, 2012 4:50 PM, "Justin" <
reply@reply.github.com>
wrote:
- @models = tracker.models.keys
- @inspect_arguments = tracker.options[:check_arguments]
- methods = tracker.find_call :target => false, :method => :link_to
- methods.each do |call|
process_result call- end
- end
- def process_result result
- #Have to make a copy of this, otherwise it will be changed to
- #an ignored method call by the code above.
- call = result[:call] = result[:call].dup
- @matched = false
- url_arg = process call[3][2]
- return if sexp?(url_arg) && url_arg.node_type == :string_interp &&
!url_arg[1].chomp.empty?The idea is that if you are doing string interpolation, you know what you
are doing?
Reply to this email directly or view it on GitHub:
https://github.com/presidentbeef/brakeman/pull/45/files#r478556
|
can
though |
|
this could not identify the engine path helper and shows it as error
|
|
This check is flagging the following code as a finding: link_to("Show", current_user.bars.find(params[:id]))However, this wouldn't allow a user to specify their own protocol and so isn't an actual vulnerability. As a result, I can't really use |
|
@presidentbeef Yep, that did it. Thanks! |
Add concept of a safe-ening method to mark hrefs as safe
Feature:
Warn when using unsafe hrefs. This is a very specific case that as of now produces a ton of noise. This came out of an xss vuln where the value was escaped but still vulnerable.
where
or
This branch accomplishes slightly intelligent warnings by adding a new command line option to declare methods that make a string URL safe (unless there is already a standard one out there). e.g.: