search

mongoid / origin

A Ruby DSL for building MongoDB queries
http://mongoid.org/en/origin/index.html
MIT License
62 stars 29 forks source link

[DNM] Protect query methods from insecure parameters: #94

Closed durran closed 10 years ago

durran commented 10 years ago

Raises an exception if ActionController::Parameters are passed directly to a query method, or are a value passed to a query method.

This applies to where, and, or, and nor.

[ SECURITY-90 ]

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.22%) when pulling aa2ba3f32a99515adddb3341888498ed4b379749 on security-90 into 459bc61c4b7d5433cdc091bdb19bf39bd0d78a93 on master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.23%) when pulling baa33e998f8147e5e13b81d87eb3deeb4417dd02 on security-90 into 459bc61c4b7d5433cdc091bdb19bf39bd0d78a93 on master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.23%) when pulling baa33e998f8147e5e13b81d87eb3deeb4417dd02 on security-90 into 459bc61c4b7d5433cdc091bdb19bf39bd0d78a93 on master.

arthurnn commented 10 years ago

IMO this should not be part of origin. origin should be only the syntax generator. Having to require actionpack in here just doesnt seems right, specially that people might not be using origin with Mongoid, or Rails.

Just as an example, AR has this type of preventions, and AREL has nothing to do with such responsibility.

@durran @ConradIrwin thoughts?

ConradIrwin commented 10 years ago

I don't have a strong opinion on where it should go — the patch is much smaller for inside Origin (it doesn't need to know about ActionController at all, just the permitted method); which is probably good for robustness. We can certainly try making a tidier patch for mongoid though.

durran commented 10 years ago

@arthurnn ActionPack is not included in origin - that's just for the specs. If people are not using Rails it will still work, just won't have any security. The solution of using permitted? here is wrong, as with strong parameters it is intended for use with mass assignment. We don't want to conflict what can be set with what can be queried on.

arthurnn commented 10 years ago

@durran Agreed with the part that the implementation should not use permitted?, thats a good point about differentiate mass assignments and actual queries. However I am still not super convinced we should put this in origin and not on mongoid. Even we are not adding actionpack in here we are adding parameters concerns in a gem that should not care about those things at all... Not that it wont work, but this as a separation of concerns matter.

durran commented 10 years ago

@arthurnn If we are being pure about separation of concerns Mongoid's entire design is wrong. :) If it would go into Mongoid, then I'd prefer the implementation did not do any method_missing magic - it would probably need to just override the two methods that were changed here... But before I do anything else am still waiting on 10gen input on SECURITY-90 itself. We might not actually do anything for it since it's not totally a framework responsibility in the first place. This is merely as a point of discussion.

arthurnn commented 10 years ago

@durran Sounds good to me, i like this implementation. I am just not following the discussion, there, as I dont have access to the issues. Maybe we should cc @brandonblack and friends here too?

durran commented 10 years ago

The pull request is linked in the issue - I just don't expect anyone to be working on the weekend. :)

estolfo commented 10 years ago

hey guys, thanks for looking at this. I'm chatting with the security team today to get their input. We can discuss on the jira ticket.

arthurnn commented 10 years ago

@estolfo :weary: i dont have access to the JIRA ticket

estolfo commented 10 years ago

oh, right. I'll update this thread as well then.

ConradIrwin commented 10 years ago

If you guys are interested, the first draft of my write up of this is at https://gist.github.com/ConradIrwin/1705866276f2afe930e5 — I wanted to wait until this patch was accepted before publishing.

On Wed, Jan 15, 2014 at 7:52 AM, Emily notifications@github.com wrote:

oh, right. I'll update this thread as well then.

— Reply to this email directly or view it on GitHubhttps://github.com/mongoid/origin/pull/94#issuecomment-32373813 .

estolfo commented 10 years ago

cool, thanks @ConradIrwin. I'll take a look at this

durran commented 10 years ago

After several other conversations here with various people and thinking about all the gotchas, the unanimous opinion is not to do this and force the users to ensure their own app is secure when passing parameters directly. The main reasons:

  1. ActiveRecord does not do this, and Mongoid is trying to be compliant with AR in as many ways as possible for the principle of least surprise.
  2. Using ActionController::Parameters in the context of a read and not a write is not how that API was intended to be used, and cannot cover all use cases (won't validate more than one level of nesting deep). It also means this is a Rails-only feature that would not be available to people not using Rails which is a substantial number of users.
  3. There is no real good generic solution that would fit users in any environment.

Will close and make a not in the Mongoid 4 documentation about security and this situation there.

estolfo commented 10 years ago

ok, thanks so much for all your research/work @durran

ConradIrwin commented 10 years ago

I've put the working version of the patch into mongoid-rails, so you can just install that gem to get protection. (It also imports the forbidden attributes protection from the strong_parameter gem).

At the moment it works on mongoid 3 with the strong parameters gem. I'll work on getting a version that works with mongoid 4 out too.