Security Fixes #1926
Security Fixes #1926
Conversation
|
I just broke up my monster commit into three that each have their own general theme, so it's easier to tell what's going on. TODO: add tests that fail if arbitrary params are symbolized |
|
I tried the below method to add tests: describe Admin::PostsController, :type => :controller do
render_views
it "should not leak on index page" do
get :index, 'really_long_malicious_key' => nil
Symbol.all_symbols.map(&:to_s).should_not include('really_long_malicious_key'), 'oh noes!'
end
endBut it kept failing even when switching to this feature branch... The assumption being that any params passed to |
| @template.url_for params.merge(@param_name => (page <= 1 ? nil : page)) | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
@gregbell, @macfanatic, thoughts?
A lot of our symbol leakage is directly caused by url_for... I don't understand why it needs to symbolize keys just to build a URL.
|
@daxter any update on the status of this PR? Overall I'm concerned with the whitelisting of the params. It means that it is impossible to add additional features without modifying the code. For example, if I add some custom filtering to my application that runs on a different param, the pagination work work anymore. Now with that being said, if there is a security or DOS attack vulnerability I am all for plugging the wholes. But it seems like the issue is more with |
|
That seems to be the case. I haven't tried bugging the Rails team as of yet; it's astounding that |
|
I'm closing the issue in favor of fixing or submitting the issue to Rails, instead of worrying about this in AA since it's not AA specific. @daxter - Will you post with the Rails team? |
|
I will open a ticket with the Rails team, but this shouldn't be closed as the underlying problem hasn't been resolved. |
|
Haha, looks like we're just using include Rails.application.routes.url_helpers
url_for action: 'index', controller: 'employees', host: 'foo.bar', params: {'eee' => 3}
# => "http://foo.bar/employees?eee=3"
Symbol.all_symbols.map(&:to_s).include? 'eee'
# => falseI'm going to re-make this PR so that user parameters are namespaced properly under the |
|
Is that in a particular area of the app, or scattered throughout you think? |
|
The issue exists anywhere we build links that include the params hash. So it's specific to the view layer. Don't actually have the tests passing yet, so we'll see. |
| view.request.stub!(:query_parameters).and_return({:controller => 'admin/posts', :action => 'index', :page => '1'}) | ||
| view.controller.params = {:controller => 'admin/posts', :action => 'index'} | ||
| view.request.stub!(:query_parameters).and_return :page => '1' | ||
| view.request.stub!(:path_parameters).and_return :controller => 'admin/posts', :action => 'index' | ||
| view |
There was a problem hiding this comment.
According to this guide, we were populating the parameters above incorrectly.
9.1.1 path_parameters, query_parameters, and request_parameters
Rails collects all of the parameters sent along with the request in the params hash, whether they are sent as part of the query string or the post body. The request object has three accessors that give you access to these parameters depending on where they came from. The query_parameters hash contains parameters that were sent as part of the query string while the request_parameters hash contains parameters sent as part of the post body. The path_parameters hash contains parameters that were recognized by the routing as being part of the path leading to this particular controller and action.
|
Hmm.. So apparently Rails before 3.2 doesn't support the |
|
After thinking about it for a while, it seems that the best option would be to utilize # in your initializer
config.safe_params.push :foobar
# Then we'd automatically preserve those parameters in our URLs across web requests
def default_url_options
params.slice *safe_params # with safe_params being that config option above.
end |
|
The benefit of this approach is that The downside is that those parameters could potentially never leave your query string, even when they're no longer applicable. So for example, |
|
Okay here we go. You could set the default url options to only be included on specific page actions. So you might want the pagination stuff to persist on links on the index page, but if you're linking to a show page, that doesn't make any sense. # so you could set it like this
config.safe_params[:index].push :foo # accepting :index, :show, or :all
# with an implementation like this
safe_params.slice(:all, params[:action]).values.flatten # the keys that should be included |
|
Comments @gregbell? |
|
Of the two options I suggested above, I'm going with the first one. YAGNI and all that. This approach seems to solve the problem of symbol leakage without removing any functionality from Active Admin. |
|
Okay, everything should be working now 🎉 |
| @@ -72,6 +72,7 @@ def mock_action_view(assigns = {}) | |||
| ActionView::Base.send :include, ActionView::Helpers | |||
| ActionView::Base.send :include, ActiveAdmin::ViewHelpers | |||
| ActionView::Base.send :include, Rails.application.routes.url_helpers | |||
| ActionView::Base.send :include, Module.new{ def safe_params; {}; end } | |||
There was a problem hiding this comment.
^^ dirty hack ^^
|
MetaSearch tries too hard to be dynamic. That's a problem because all of these symbolize the string given: respond_to?
defined?
sendThe only real solution is to check if there's a DB attribute (or predefined search method) for each I imagine Ransack has the very same problem. |
|
Looking at this again, I almost want to remove all the |
| class MetaSearch::Builder | ||
| def assign_attributes(opts) | ||
| opts.each_pair do |k, v| | ||
| send "#{k}=", v if base.column_names.include? k[/(.+)_.+\z/, 1] |
There was a problem hiding this comment.
This actually won't work in all situations.
:name_contains # works
:name_starts_with # breaks
|
Nevermind. I just did a local merge of this PR and the |
|
So the only question left is whether we want to remove the |
|
FYI @gregbell and @macfanatic I'm setting this to the 0.7.0 milestone. My plan is to merge #1979, then update this PR appropriately and merge it in. |
|
After rebasing this on master, without the fixes: |
|
In case I ever want it (and as an experiment to see if GitHub will hold onto the commits for a long time), this branch was at 8781916 before I rebased & updated it. |
url_for normally symbolizes the keys passed to it. This is a problem for Active Admin, since we don't want to limit extensibility by whitelisting which query parameters are persisted on the index page. We get around this problem by passing the query parameters back into new URL creation, namespaced under the `:params` key. This way they're persisted, but arbitrary symbols aren't created in memory.
|
Finally, a simple solution to this problem. |
Until now we've been passing the entire params hash to
url_for, which callssymbolize_keyson the hash passed in. The result is that an attacker can DOS your website by passing arbitrary strings as parameters. An example being:Symbols never leave memory (until app restart), so this attack vector is quite effective against Ruby apps.
The good news is, only registered users are able to exploit this vulnerability.
I think this whole adventure was pretty cool. ✨ If you want to try it out yourself, here's some tools.