[Concept] Always require permit before fetching #13215

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
Contributor

imanel commented Dec 6, 2013

This is just idea and I need to hear opinion about it before moving forward. Code is just couple lines to help me explain idea and will be reworked if community agree that this might be good direction.

How strong parameters are working now:

Let's take example from strong_params gem:

class PeopleController < ActionController::Base
  # This will raise an ActiveModel::ForbiddenAttributes exception because it's using mass assignment
  # without an explicit permit step.
  def create
    Person.create(params[:person])
  end

  # This will pass with flying colors as long as there's a person key in the parameters, otherwise
  # it'll raise a ActionController::MissingParameter exception, which will get caught by
  # ActionController::Base and turned into that 400 Bad Request reply.
  def update
    person = current_account.people.find(params[:id])
    person.update_attributes!(person_params)
    redirect_to person
  end

  private
    # Using a private method to encapsulate the permissible parameters is just a good pattern
    # since you'll be able to reuse the same permit list between create and update. Also, you
    # can specialize this method with per-user checking of permissible attributes.
    def person_params
      params.require(:person).permit(:name, :age)
    end
end

Strong params help us with one issue: params passed to ActiveRecord attributes require to be permitted, otherwise it will raise error. This prevent from writing to columns we do not allow, and is considered secure enough.

Unfortunately there is second problem in this code that strong params can't solve right now: when we are searching for record (i.e. in mentioned update method) we are allowing any data sent in id key. It might not always be single key - sometimes it might be array or other type. This lead to errors like CVE-2013-0155:

unless params[:token].nil? 
  user = User.find_by_token(params[:token]) 
  user.reset_password! 
end

With token param equal empty array or [nil] such code will pass and be potentially insecure. Right now Rails uses temporary fix that compacts tables and converts them to nil. This is useful to prevent accidentally allowing nil value, but is not preventing passing multiple values. Because of that there is potential vector of attack by sending huge number of tokens instead of one with chance to find one (it's unlikely, but still this is potential security problem). Unfortunately patching find method is not solution as we can use find, find_by, where and multiple other statements - maintaining fixes in all of them would be nightmare. There is also problem with valid parameters that are changed comparing to what server receives - it breaks nested attributes and json.

Because of that it might be good idea to move checking parameters from ActiveRecord to ActionController::Parameters directly. That would make much easier to control how and when params would be used.

Concept:

At this point ActionController::Parameters allows to fetch params via [] and fetch methods. This behavior returns value, and if this value is type of Hash it has copied status of permit. Unfortunately it's unable to store such params on Array and other types. It's possible to use strong params like that:

params.permit(:token) # Will return nil for Array

unless params[:token].nil? 
  user = User.find_by_token(params[:token]) 
  user.reset_password! 
end

Unfortunately, because it's not enforced there are not many teams that are doing so. This concept is about enforcing such behavior - without permit calling [] or fetch on params would raise error about unpermitted params.

# params.permit(:token)

unless params[:token].nil? # Raises ActionController::UnpermittedParameters or other error
  user = User.find_by_token(params[:token]) 
  user.reset_password! 
end

That would prevent both errors like CVE-2013-0155 and ensure valid type of params (if you would allow Array then single value will not be allowed). Such behavior might be considered beneficial if we think that up to now all params should be treated as insecure.

There is two problems with enforcing using permit on all parameters:

  1. All current application will need to add permit in every search
  2. It's not very convenient to write permit every time we are using id

So I'm thinking about adding class method permit_params as syntactic sugar:

class PeopleController < ActionController::Base

  permit_params :id, only: :update

  def update
    person = current_account.people.find(params[:id])
    person.update_attributes!(person_params)
    redirect_to person
  end

end

That would make permitting much easier for different actions.

Second idea is to permit string and integer values at default, which would cover around 90% of situations, and would require only allowing array/hash if needed.

Please let me know what do you think about this idea - it's just concept for now and I'm open to suggestion in which direction I should go, but it might make managing parameters much easier. I'm also sure that this idea is big change so it would require some preparation and couldn't enter master in near future.

Contributor

tomash commented Dec 6, 2013

+1 to anything that will let us prevent things like spree/spree#2492 in a sane way

@imanel imanel referenced this pull request Dec 20, 2013

Closed

Remove deep munge #13157

Owner

jeremy commented Dec 20, 2013

I'm a fan of moving this responsibility out of AR and in to strong params.

Strong params is focused on which keys are permitted, but we also need to say which values are permitted.

One approach is to taint all param keys (using Ruby builtin Object#taint) then only untaint them when permitted.

Upgrading apps takes a lot of effort and breaks plugins. That'll be unavoidable, though. Best to find a gentle path, like providing a plugin that works with 3.2 and 4.0 apps, like we did with rails_xss in 2.3. Then folks could move to stronger_params on their current app before making the big plunge to Rails 4.2 where it's required.

Contributor

imanel commented Dec 20, 2013

Shouldn't all values be already tainted? I will check that obviously, but that would make my work much easier.

I'm also considering overwriting all Hash methods that return values to raise exception if value is tainted - not sure if this will not be overkill, but that would solve some problems. I also believe that values of type from permitted scalar types should be untainted by default.

One remaining problem with this solution is that some folks are passing params outside controller, and that could lead to raising errors in places hard do catch (views?). So instead of raising we could return nil, but that on the other hand will obscure results (but I believe that it's how it works now for permit method). What do you think?

Contributor

lukesarnacki commented Jan 21, 2014

I wanted to stir this discussion a little bit by writing about current approaches that came out after some discussions with @imanel and some other people.

Currently there are 2 approaches I want to discuss.

1. Filter parameters by default

This is similar to what is written in this PR description. Main thing of this approach is to filter all parameters, when permit is not used, besides action, controller and probably id, to make life little bit easier.

Currently there is action_on_unpermitted_parameters accessor in Parameters. When set to :log, unpermitted parameters will be set to nil, when set to :raise ActionController::UnpermittedParameters exception will be raised. In that regard new solution could be similar to current one. In PR’s description there is exception as default, but default itself is not that important.

Regardless what default will be, the most important thing to remember is that this approach will be noticeable as soon as calling fetch and [] methods. So just as quick reminder:

def update
  params[:token] # => nil 
  # (…)
end

In this code, params[:token] would return nil and appropriate message will be printed to logs, because there wasn’t permit! method called. After using permit! method, actual value could be returned:

params.permit!(:token)
params[:token] #=> "abcdef"

To make life easier there would be class method, as @imanel wrote before:

class PeopleController < ActionController::Base
  permit_params :token, only: :update
end

There are few concerns that I have regarding this approach. Main one is of course that this would be very intrusive. Even when logging filtered parameters, this is quite radical solution. It is easy to think about potential problems, like what should inspect return. Params won’t be removed, they will just be filtered, but for developer it will look like they are gone.

params.inspect # => {}
params[:token] # => nil (and log message)
params.permit!(:token)
params.inspect # => { token: "HODOR" }
params[:token] # => "HODOR"

Or if we want to implement inspect to see what params really are:

params.inspect # => { token: "HODOR" }
params[:token] # => nil (and log message)
params.permit!(:token)
params.inspect # => { token: "HODOR" }
params[:token] # => "HODOR"

It is arguable, whether former or latter inspect behaviour makes more sense, but for sure it would bring some confusion. Also when using former implementation, there is question - log or not to log (as this also filters values, same as [] and fetch).

I have kind of love / hate relationship with this approach and already tried to start implementing this, but I want to hear what other people think about this.

2. Mark params as tainted by default (and don't trust those little scumbags)

Second approach is the one that I came up with while seeking for something bit less radical.

As @jeremy suggested, params could be tainted (by using ruby taint method) by default. tainted can be used in a few ways (also as implementation detail of first approach, but there are better and faster approaches to do that). The way I see it: bottom line is that tainted values wouldn’t be trusted by ORM.

So in classic deep_munge justification example:

def update
  unless params[:token].nil? # this value is not nil, but it is mark as tainted
    User.find_by_token(params[:token])
    # At this point ActiveRecord exception will be raised because params[:token] is tainted
  end
end

So to ilustrate how sending potentially dangerous params { token: [] } would work:

def update
  params.permit!(:token) # this would untaint only scalar token values

  unless params[:token].nil? # [] is not nil but it is still tainted
    User.find_by_token(params[:token])
    # At this point ActiveRecord exception will be raised because params[:token] is not scalar and wasn't untainted
  end
end

Of course it would be possible to untaint arrays as well:

  params.permit!(tokens: [])

Downside of this approach is that it would have to be implemented in ActiveRecord and potentially in other ORM’s. So still ActiveRecord would have to do some of the work, but it could be little bit more dumb - “I don’t know anything about what happens in controller, but I know that I shouldn’t trust tainted objects”.

Also I am not sure about how it would affect performance when dealing with large params. Because for ruby params are untainted by default.

On the other side, till the point of querying database, there wouldn't be much confusion. Raising exception while running query would be for sure less intrusive as parameters would act as they currently do.

Any comments / suggestions would be appreciated :) @jeremy @chancancode

Contributor

imanel commented Jan 21, 2014

Personally I hoped for solution that would not require implementing outside of strong params (i.e. ORMs) as this could be hard to enforce - tools like Elasticsearch, Redis etc. has dedicated gems that has nothing to do with Rails, so making them implement such solution might be not best idea. I personally prefer first option, as this is secure by default (and require no involvement outside of strong_params library), but I'm well aware that it will piss a lot of developers.

As @lukesarnacki said we would really appreciate some feedback, as such decisions should not be taken lightly.

Member

NZKoz commented Jan 21, 2014

returning nil for un-permitted params will just open up a whole other can of worms, imagine people forgetting to permit in this case

  if user.forgotten_password_token == params[:token]
    self.current_user = user
  end

Raising an exception sounds like a great option, so long as it's opt in because it'll be a complete nuisance. You could also add a one-line helper such as (note, not actually advocating that name):

  User.find_by_token(params.get_string[:token]) # raises if :token doesn't hold a string??

However I'd still prefer we ensured that Active Record doesn't get tricked by things as simple as putting {} or [] where we were expecting a string. I wouldn't rely on AR being the only fix because as @imanel says there's lots of other libraries, but that's no reason not to have defense in depth

Contributor

imanel commented Jan 21, 2014

Quote:

if user.forgotten_password_token == params[:token]
  self.current_user = user
end

Wait - wouldn't it also be problem if you will simply pass nil as token? ;-)

Usually nil instead of expected value is easiest option - it means that client didn't provided param. In case of proposed filtering the only meaning is that client didn't provided valid value so for example:

params.permit!(:token)
params[:token] #=> nil for params = { :token => {} }
params[:token] #=> 1 for params = { :token => 1 }

params.permit(:token => [])
params[:token] #=> nil for params = { :token => 1 }
params[:token] #=> [1] for params = { :token => [1] }

Second problem is enforcing methods like get_string - usually developer who knows about potential issue here will use to_s ATM, which will fix bugs like CVE-2013-0155. The problem is situation when we are talking about developer that is not aware of such possibilities (or is aware, but is in rush and forgets about it). That's why deep_munge was implemented - to be secure by default.

I'm also pretty sure that it's not possible to fix AR by itself - we need way to pass empty array if we need, and to prevent such behavior when we don't. Because of that I personally believe this should not relate to AR code at all and be implemented much earlier - especially that there are a lot of other gems that will not follow AR example.

Contributor

lukesarnacki commented Jan 23, 2014

However I'd still prefer we ensured that Active Record doesn't get tricked by things as simple as putting {} or [] where we were expecting a string. I wouldn't rely on AR being the only fix because as @imanel says there's lots of other libraries, but that's no reason not to have defense in depth.

The way I see it, if filtering params by default is too intrusive for core team (it seems intrusive to me also), then agreeing on some guideline (like don't trust tainted objects) is probably best way to go. I know, that this path is longer and for sure there are libraries which will ignore this, but at least this is simple concept and uses simple ruby feature.

Thing to note here is that most probably not all of the ORM's even have such problem.

Such solution seems harder, cause it needs more evangelization, but it will become "rails way" after time.

@chancancode chancancode added JRuby and removed JRuby labels Jun 26, 2014

@rafaelfranca rafaelfranca modified the milestones: 4.2.0, 5.0.0 Aug 18, 2014

Contributor

piotrj commented Jul 29, 2015

@imanel is that still something we want to pursue, is that still an issue? (cc @rafaelfranca )

As for specifics if we go this way I don't think returning nil if the parameter has not been permitted is not such good idea. I can easily imagine someone going in:

  params.inspect # => { token: "HODOR" } - ok, that's cool
  params[:token] # => nil - WAIT A MINUTE, WHAT?

If Unpermitted exception was raised the developer would be one google query away from figuring out what's wrong.

Contributor

imanel commented Jul 29, 2015

It looks like it will not be needed for now after this change. I'm closing it.

@imanel imanel closed this Jul 29, 2015

Contributor

piotrj commented Jul 29, 2015

@imanel this commit didn't change much. It's more performant cause it traverses the hash only once and the method is not called deep_munge anymore but the idea and functionality is the same, right? Or am I missing something.

Contributor

imanel commented Jul 30, 2015

You're right - I closed this PR prematurely. On the other hand it's been nearly two years since opening it and I was unable to deliver any sane solution (I've tested several and neither of them sounds reasonable enough to include in core rails).

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

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