Permalink
Browse files

Whitelist all attribute assignment by default.

Change the default for newly generated applications to whitelist all attribute assignment.  Also update the generated model classes so users are reminded of the importance of attr_accessible.
  • Loading branch information...
1 parent 864d755 commit 06a3a8a458e70c1b6531ac53c57a302b162fd736 @NZKoz NZKoz committed Mar 4, 2012
@@ -30,6 +30,10 @@ def attributes_with_index
attributes.select { |a| a.has_index? || (a.reference? && options[:indexes]) }
end
+ def accessible_attributes
+ attributes.reject(&:reference?)
+ end
+
hook_for :test_framework
protected
@@ -3,5 +3,10 @@ class <%= class_name %> < <%= parent_class_name.classify %>
<% attributes.select {|attr| attr.reference? }.each do |attribute| -%>
belongs_to :<%= attribute.name %>
<% end -%>
+<% if !accessible_attributes.empty? -%>
+ attr_accessible <%= accessible_attributes.map {|a| ":#{a.name}" }.sort.join(', ') %>
+<% else -%>
+ # attr_accessible :title, :body
+<% end -%>
end
<% end -%>
@@ -58,7 +58,7 @@ class Application < Rails::Application
# This will create an empty whitelist of attributes available for mass-assignment for all models
# in your app. As such, your models will need to explicitly whitelist or blacklist accessible
# parameters by using an attr_accessible or attr_protected declaration.
- # config.active_record.whitelist_attributes = true
+ config.active_record.whitelist_attributes = true
<% unless options.skip_sprockets? -%>
# Enable the asset pipeline
@@ -319,4 +319,14 @@ def test_index_is_skipped_for_references_association
end
end
end
+
+ def test_attr_accessible_added_with_non_reference_attributes
+ run_generator
+ assert_file 'app/models/account.rb', /attr_accessible :age, :name/
+ end
+
+ def test_attr_accessible_added_with_comments_when_no_attributes_present
+ run_generator ["Account"]
+ assert_file 'app/models/account.rb', /# attr_accessible :title, :body/
+ end
end

65 comments on commit 06a3a8a

Contributor

Thanks! Guess the change in config/application.rb would be enough, but people tend to create a shitstorm instead.

Contributor

Was this really intended for 3-2-stable?

Contributor

Looks great! I like it!

Contributor

\m/

Awesome.

Member

@parndt it will be on only for new apps, so yes it's safe to do it on 3-2-stable

Contributor

+1million

Contributor
Contributor

Nice one!

Contributor
eval replied Mar 4, 2012

Once I started using attr_accessible consistently, it was handy to have a role that could access any attribute. sudo_attr_accessibility is the result I came up with https://github.com/eval/sudo_attr_accessibility

Contributor

👍

About time this was made default. It's just way too easy to forget about things with a permitted-by-default scheme.

dirk replied Mar 4, 2012

@eval Considering that sudo is short for su (super-user) and do, wouldn't a more apt name for the access-all role be just su?

Member

@eval you can specify the role, not sure when was it added, but probably 3.1/3.2

Guess we can thank @homakov for that.

Contributor

@drogus Yeah but @eval's way of doing it gives access to all attributes, the roles only give the permissions set.

I think this is a great way to deal with the issue, as it doesn't breaks old apps. I really like the patch.

The downside is, that old apps still keep the vulnerability, maybe we could also add some warnings, similar to deprecation warnings like, "Your model blacklists your attrs and may be vulnerable to mass-assignment - consider using attr_accessible" (And a way to shut the warning of, for those who doesn't care :)

Contributor

Old apps would keep this vulnerability no matter what they add if they don't update their Rails version.

Member

@larzconwell it's dead simple to do something like that in previous rails versions, by doing for example ActiveRecord::Base.attr_accessible nil, the thing is that if you have a big app that does not whitelist attributes, it will all break if you turn it on. That's why there is no simple magical fix for it.

Contributor

How would that code get into someone's Rails version that's already downloaded though? They'd have to re-install Rails, or update the version to get any new code. Right? Maybe I'm just not thinking it through all the way.

jtoy replied Mar 5, 2012

Thanks for fixing this @homakov !!!

Member

@larzconwell this commit only changes things that are generated, so even if you upgrade, it will not change anything in your application. If your app is vulnerable, you need to use attr_accessible or secure your models in some other way, there is no simple fix for it as this is not regular security issue that can be easily fixed by framework. It was probably mistake to not make whitelisting default, but the tools to secure mass assignment was in place for years.

Contributor

Oh yeah I knew that, just a slow moment haha. Sorry for the confusion!

Seems legit.

@larzconwell

Since it really isn't a rails-framework issue, I think it is important to make developers aware of it. If you do regular rails upgrades in your apps (which you really should for security fixes), you should at least got some warnings on vulnerable models.

Contributor

A walk through as to how to incorporate this into existing web applications will be nice.

Contributor

@marcoemrich Definitely true, developers should use attr_accessible no matter what.

@why-el Well an existing application should already have attr_accessible in it, but un-comment config.active_record.whitelist_attributes = true into your config/application.rb

Contributor
tinco replied Mar 5, 2012

Older rails applications will still be vulnerable even when they upgrade their rails version. This commit only fixes the generator of the config file. Granted ofcourse that they were not handling their attribute protection right, like github was.

Contributor

@larzconwell Done. Thanks.

@d-snp right, of course - that's why I would suggest to add a warning system, that informs the developers about their possible vulnerability and recommend switching to a whitelist concept

Rails could output a warning, for all models, that don't use attr_accessible

Contributor
janx replied Mar 5, 2012

thanks @homakov!

thanks @homakov!

a thousand crackers just shed a tear

o/

Nice! I like it.

budu replied Mar 5, 2012

Finally! I've been advocating something like that (through an initializer) at my job and nobody would listen. I can't count the number of vulnerable web app I've seen out there!

Ah, the story has a happy ending!

Contributor
eval replied Mar 5, 2012

@drogus yes, but once you define any role (say 'attr_accessible :name, :as => :possibly_malicious_user_input') i.e. specifically for mass-assignments in controllers, there's no way to ignore roles in other parts of your code (e.g. tests, working in console, whole lot's of non-controller code).

Ways to workaround are

  1. use ':without_protection => true' (cumbersome),
  2. explicitly (re)define all attributes to be attr_accessible for some other role, maybe 'default' (hard to maintain when adding attributes).

This gem helps with 2) in that you can define a role that has always access to all attributes. Maybe better: When doing 'sudo_attr_accessible_as :default' code can be role-unaware by default.

Contributor
eval replied Mar 5, 2012

@dirk good point!

Member

@eval ok, sorry, I just skimmed through the README and didn't get that difference

It still auto-adds the attr_accessible list which is nearly as bad as allowing mass assignment in the first place. At most it should add an informative comment, and the error message about being unable to mass assign attributes should be made better, possibly with a URL to the existing mass assignment Rails guide.

Owner

@eval write a custom TestSanitizer that doesn't remove anything from the attribute hash and configure it in your test environment or you can do it on a test by test basis:

class TestSanitizer < ActiveModel::MassAssignmentSecurity::Sanitizer
  def sanitize(attributes, authorizer)
    attributes # you may want to do something else here
  end
end

class MyTestCase < ActiveSupport::TestCase
  def test_case
    without_attribute_protection do
      # your test code
    end
  end

  def without_attribute_protection
    ActiveRecord::Base.mass_assignment_sanitizer = TestSanitizer.new
    yield
  ensure
    ActiveRecord::Base.mass_assignment_sanitizer = :logger
  end
end
Contributor
gtd replied Mar 5, 2012

@jyap808 No, that wasn't a proper troll, but your comment certainly is :)

jurre replied Mar 5, 2012

@dhh posted this gist[0] earlier today and I'm wondering what everyone feels is the best way to handle this, I'm currently using attr_accessible but I do feel there's some merit to dhh's gist.

[0]https://gist.github.com/1975644

If nothing else, this breaks all 6+ years of prior documentation on generating models and applications. This seems inappropriate and unnecessary in a stable release, when the current behavior is not unreasonable and was so vociferously defended for years.

Contributor

@allix I disagree, the current behavior is unreasonable and unacceptable since every application is insecure by default. The fact that Github could fall victim to a mass assignment vulnerability should be more than enough evidence that this change is necessary. In fact, I'd go even further and say that config.active_record.whitelist_attributes should be set to true by default (not just in the generated application.rb) and developers who don't want it should have to specifically disable it. Any documentation or app that is "broken" by that change was already broken anyway, the developers just didn't realize it, so this change would wake them up and make their apps more secure in the long run.

@stevenh512 Do we even know that GitHub was not using attr_accessible? They could have just opened up columns that are only accessible to admins to all users because of the bluntness of attr_accessible (somewhat remedied by the new role based feature).

Regardless, Rails can't protect developers against not paying attention when they apply untrusted form data directly to models - that is just an amateur mistake, and it seems most applications do just fine as their developers properly filter parameters before updating models. This is quite clearly the fault of GitHub developers, and it's unclear whether attr_accessible by default would have changed anything.

Contributor

True, we don't really know that Github wasn't using attr_accessible, the only way to truly know the exact cause of their particular vulnerability is if they tell us.. but we do know that there are plenty of apps out there that are not using attr_accessbile because 1) it is not enforced by default, 2) there's a lot of bad documentation out there, and 3) beginner/intermediate developers see that it "just works" and don't think about the security implications (which goes back to 1). I think it's a lot easier to fix it in Rails and "break" those already broken applications than it would be to go through (as you said) 6+ years of documentation, fix it, and then hope everybody pays attention.

Fixing XSS vulnerability "broke" a lot of things too, but again, those things were already broken.

Contributor

BTW, how many years of documentation were broken in moving from Rails 1 to 2, or 2 to 3, or even (NOT a "major version" upgrade) 3.0 to 3.1?

Contributor
eval replied Mar 5, 2012

@pixeltrix looks nice - thanks!

Member

Maybe I'm outdated, but I remember a Github conf saying that Github is still mostly under Rails 2.3.
So they do not use attr_accessible.

On another topic, I think this is not the role of the model layer to sanitize the user input.

Member

@drogus: wow, why was I persuaded that it was a 3.0 feature... Sorry for this useless comment.

@stevenh512 regarding documentation, breaking documentation is fine for major or even minor releases. Not so much for a stable release. Anyhow, it's probably for the better.

Contributor

@aliix regarding documentation, if that's the case, why not really fix this (instead of just for new apps) and call it 3.3.0?

And regarding apps, this is just my personal opinion.. but if I had an app that was vulnerable (I don't, becuase step 1 for me is to drop in an initializer that forces me to use attr_accessible.. but for the sake of argument, if I didn't know any better), I would much rather deal with "a Rails update broke my app" compared to "a vulnerability I wasn't aware of because I followed some bad docs caused my entire app to get owned and now my users are ready to hang me."

norv replied Mar 7, 2012

This is a good step, thanks for the fix!

I think there should be at least warnings, for older code, in the measure of possible, to raise awareness, by default. If not downright break those with no attr_accessible.

+1 on the fix, but the reason mass assignment exists in the first place is for convenience and DRYness of syntax. Sometimes it's still convenient to use update_attributes outside of a controller for 'unsafe' attributes and with a default whitelist you can't. What about something that mirrors #html_safe?:

foo.update_attributes({:a => 'bar'}.attr_safe)
Contributor

@norv I agree, but I go a step further and say that a change to force mass-assignment security on all apps (not just new ones), with some reasonable way for a more advanced developer to disable it if they really needed to, wouldn't break a thing. Any app with those kinds of vulnerabilities was already broken the moment it was written and deployed and it's only a matter of time before someone exploits it.

Contributor

@andyvb didn't see your comment while I was typing mine (we must have been commenting at the same time.. lol).. but the situation you describe is already handled by ActiveModel in Rails 3.1+.. that's what scoped attr_accessible whitelists are for.

@stevenh512 thanks for the info! Looks like :without_protection is what I'm looking for.

Contributor

@andyvb I was thinking more of the scoped attr_accessible that Rails 3.1 gives us (attr_accessible :attr1, :attr2, :as => :admin), but yeah, :without_protection would also work and would probably be more backwards compatible. For 3.0 and earlier there's also a Railscast that teaches how to do something similar to 3.1's scoped attr_accessible.

What about *_ids methods?

Lot of people use attr_protected for keys and flags, simply because it is easier to blacklist a few fields than to withelist the rest.

How many of them ever used *_ids method to be aware of that? This is not even documented at http://guides.rubyonrails.org/security.html#countermeasures.

This SHOULD be protected by default in next release.

What about *_ids methods?

Lot of people use attr_protected for keys and flags, simply because it is easier to blacklist a few fields than to withelist the rest.

How many of them ever used *_ids method to be aware of that? This is not even documented at http://guides.rubyonrails.org/security.html#countermeasures.

Пойдет

Please sign in to comment.