Skip to content

Instantly share code, notes, and snippets.

@peternixey
Created March 5, 2012 13:10
Show Gist options
  • Select an option

  • Save peternixey/1978249 to your computer and use it in GitHub Desktop.

Select an option

Save peternixey/1978249 to your computer and use it in GitHub Desktop.

Revisions

  1. petenixey revised this gist Apr 11, 2012. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -225,7 +225,9 @@ You’re going to have some frustrations. There are going to be things that you
    - Nested attributes: [Instructions here](http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#label-Using+with+attr_accessible)
    - Delayed Job (fix from @borski): add the following line to make the appropriate delayed_Job attributes work:

    Delayed::Job.attr_accessible :priority, :payload_object, :run_at, :locked_at, :failed_at, :locked_by
    ```ruby
    Delayed::Job.attr_accessible :priority, :payload_object, :run_at, :locked_at, :failed_at, :locked_by
    ```

    I’m sure you’ll hit other issues too but you can generally knock them off by adding attributes one by one to the list of accessible ones.

  2. petenixey revised this gist Apr 11, 2012. 1 changed file with 3 additions and 6 deletions.
    9 changes: 3 additions & 6 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -223,6 +223,9 @@ You’re going to have some frustrations. There are going to be things that you
    - Authlogic: you need to remember to make attributes like password, email etc accessible
    - Paperclip: remember to make paperclip attributes accessible
    - Nested attributes: [Instructions here](http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#label-Using+with+attr_accessible)
    - Delayed Job (fix from @borski): add the following line to make the appropriate delayed_Job attributes work:

    Delayed::Job.attr_accessible :priority, :payload_object, :run_at, :locked_at, :failed_at, :locked_by

    I’m sure you’ll hit other issues too but you can generally knock them off by adding attributes one by one to the list of accessible ones.

    @@ -248,12 +251,6 @@ Rails is a brilliant framework designed by brilliant contributors. One of the re

    If GitHub, one of the best Rails teams on the planet can be taken out so easily, in so many places by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.

    **A three step suggestion for how the Rails team could address the isssue:**

    * On the next release start raising warnings in development when an attempt is made to update an attribute without declaring it `attr_accessible`
    * On the release after raise errors in development and warnings in production
    * On the third release make attr_accessible on by default

    ------

    Author: Peter Nixey
  3. petenixey revised this gist Mar 6, 2012. 1 changed file with 7 additions and 0 deletions.
    7 changes: 7 additions & 0 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -13,6 +13,7 @@ If you'd like to follow me on twitter my handle is [@peternixey](http://twitter.

    He was right. The Rails defaults are vulnerable and there’s no better illustration of this than when when one of the best Rails teams in the world is severely compromised.

    ----

    **TL;DR: How to protect your Rails application from the GitHub attack**

    @@ -32,6 +33,12 @@ If you want to protect attributes from being updated you either need to single t

    The initializer switches this round and makes whitelisting the default setting. With the intializer switched on, `update_attributes` will only update attributes on your models which are declared `attr_accessible`.

    **NB this is also true for `.new` and `.create`**

    This strength and vulnerability in Rails is referred to as Mass Assignment and can happen when you pass a hash of arguments into either `.new` or `.create` as well as `.update_attributes`. The latter tends to be the most vulnerable but all are susceptible.

    There is good advice including an even cleaner solution than the one below on the [official Rails documentation for protecting against mass assignment attacks](http://guides.rubyonrails.org/security.html#mass-assignment).

    ----

    **Why this is needed**
  4. petenixey revised this gist Mar 6, 2012. 1 changed file with 0 additions and 2 deletions.
    2 changes: 0 additions & 2 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -253,8 +253,6 @@ Author: Peter Nixey
    Twitter: http://twitter.com/peternixey
    Blog: http://peternixey.com

    ***PLEASE NOTE I AM NOT A MEMBER OF THE GITHUB TEAM. THIS IS JUST A GIST***

    ----

    ###*Update - an update has been added to Rails to whitelist by default*
  5. petenixey revised this gist Mar 6, 2012. 1 changed file with 2 additions and 2 deletions.
    4 changes: 2 additions & 2 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -279,8 +279,8 @@ This is a slightly different fix than the one described here and almost certainl

    *Further reading:*

    * [Official Rails advice on dealing with Mass Assignment vulnerabilies]
    * [Hacker news discussion on this article](http://news.ycombinator.com/item?id=3666564)(http://guides.rubyonrails.org/security.html#mass-assignment)
    * [Official Rails advice on dealing with Mass Assignment vulnerabilies](http://guides.rubyonrails.org/security.html#mass-assignment)
    * [Hacker news discussion on this article](http://news.ycombinator.com/item?id=3666564)
    * [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)
    * [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
    * [Rails Spike’s “Is your application safe?”](http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment)
  6. petenixey revised this gist Mar 6, 2012. 1 changed file with 5 additions and 4 deletions.
    9 changes: 5 additions & 4 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -279,7 +279,8 @@ This is a slightly different fix than the one described here and almost certainl

    *Further reading:*

    * [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
    * [Rails Spike’s “Is your application safe?”](http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment)
    * [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)
    * [Hacker news discussion on this article](http://news.ycombinator.com/item?id=3666564)
    * [Official Rails advice on dealing with Mass Assignment vulnerabilies]
    * [Hacker news discussion on this article](http://news.ycombinator.com/item?id=3666564)(http://guides.rubyonrails.org/security.html#mass-assignment)
    * [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)
    * [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
    * [Rails Spike’s “Is your application safe?”](http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment)
  7. petenixey revised this gist Mar 6, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -257,7 +257,7 @@ Blog: http://peternixey.com

    ----

    ###Update - an update has been added to Rails to whitelist by default
    ###*Update - an update has been added to Rails to whitelist by default*

    *6th March 2012*

  8. petenixey revised this gist Mar 6, 2012. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -259,7 +259,9 @@ Blog: http://peternixey.com

    ###Update - an update has been added to Rails to whitelist by default

    Part a result of everything that happened over the weekend, new projects in Rails will default to whitelisting all attribute arguments by default:
    *6th March 2012*

    Part a result of everything that happened in the past few days, new projects in Rails will default to whitelisting all attribute arguments by default:

    https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736

  9. petenixey revised this gist Mar 6, 2012. 1 changed file with 18 additions and 0 deletions.
    18 changes: 18 additions & 0 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -257,6 +257,24 @@ Blog: http://peternixey.com

    ----

    ###Update - an update has been added to Rails to whitelist by default

    Part a result of everything that happened over the weekend, new projects in Rails will default to whitelisting all attribute arguments by default:

    https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736

    This is a slightly different fix than the one described here and almost certainly better - I would advise using the official Rails solution which simply involves a change in your config file:

    config/application.rb

    ...
    config.active_record.whitelist_attributes = true


    *It will not affect existing projects which will still need to address their issues independently* however it will mean that new Rails projects will be required to whitelist attributes by default. The commit is planned to be included in Rails 3-2-stable

    ----

    *Further reading:*

    * [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
  10. petenixey revised this gist Mar 6, 2012. 1 changed file with 2 additions and 2 deletions.
    4 changes: 2 additions & 2 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -195,9 +195,9 @@ The beauty of this is that it effectively adds `attr_accessible` to every model

    Althugh the GitHub hack was done using `update_attributes`, almost everything that's described here is also true for `.create` and `.new` too.

    If you are not careful with the inputs that you feed into your object creation methods they too will initialize attributes on the object which you might not want to be initalized (for intance User.create( params[:user] ) woud also be vulnerable to `role: :superadmin` being passed in.
    If you are not careful with the inputs that you feed into your object creation methods they too will initialize attributes on the object which you might not want to be initalized. For instance User.create( params[:user] ) woud also be vulnerable to `role: :superadmin` being passed as a parameter.

    This whole category of vulnerability/strength is referred to as *[mass assignment](http://guides.rubyonrails.org/security.html#mass-assignment)*.
    The ability of Rails objects to assign multiple variables simultaneously is referred to as *[mass assignment (docs)](http://guides.rubyonrails.org/security.html#mass-assignment)*.

    ----

  11. petenixey revised this gist Mar 6, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -191,7 +191,7 @@ The beauty of this is that it effectively adds `attr_accessible` to every model

    ----

    ***NB everything described below also applies to `.create` and `.new`***
    ***NB everything described here also applies to `.create` and `.new`***

    Althugh the GitHub hack was done using `update_attributes`, almost everything that's described here is also true for `.create` and `.new` too.

  12. petenixey revised this gist Mar 6, 2012. 1 changed file with 12 additions and 0 deletions.
    12 changes: 12 additions & 0 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -189,6 +189,18 @@ Create a new file:

    The beauty of this is that it effectively adds `attr_accessible` to every model we create (actually what it does it take it away by default but it comes to the same thing). No attribute can be updated unless we declare it `attr_accessible`. We’re secure until we decide otherwise.

    ----

    ***NB everything described below also applies to `.create` and `.new`***

    Althugh the GitHub hack was done using `update_attributes`, almost everything that's described here is also true for `.create` and `.new` too.

    If you are not careful with the inputs that you feed into your object creation methods they too will initialize attributes on the object which you might not want to be initalized (for intance User.create( params[:user] ) woud also be vulnerable to `role: :superadmin` being passed in.

    This whole category of vulnerability/strength is referred to as *[mass assignment](http://guides.rubyonrails.org/security.html#mass-assignment)*.

    ----

    **Possible issues you might have with the initializer**

    Once you setup the initializer, the first thing you’re going to need to do is declare all relevant attributes as `attr_accessible`.
  13. petenixey revised this gist Mar 6, 2012. 1 changed file with 1 addition and 3 deletions.
    4 changes: 1 addition & 3 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -219,12 +219,10 @@ Enforcing that attributes have to be declared `attr_accessible` by default would

    **What should the default authorization setting to be, on or off?**

    [Yehuda Katz makes the point](https://gist.github.com/1974187) that this is an authorization issue which is not a framework issue. The question here though is not *“where should authorization be handled”* but *“what should the default setting for authorization be”*.
    [Yehuda Katz makes the point](https://gist.github.com/1974187) that this is an authorization issue which is not a framework issue. I think the question here though is not *“where should authorization be handled”* but *“what should the default setting for authorization be”*.

    In most other public-facing interfaces in Rails the default setting for authorization is unauthorized. You can’t even reach a controller method unless you explicitly create a route for it. `update_attributes` however defaults to authorized.

    Where this should be handled is not an academic design choice, it’s one that’s carrying a real world cost right now. Arguing over what layer of the app is responsible is like BP blaming Transocean for the Deepwater Horizon. The issue is that oil is leaking.

    **There are a lot of sites vulnerable and more being built every day**

    Rails is a brilliant framework designed by brilliant contributors. One of the reasons I like coding in it is that I feel that I always learn from the code I find (in PHP I generally wanted to rewrite it). This one weakness has always bugged me though and I feel it doesn't do Rails justice.
  14. petenixey revised this gist Mar 6, 2012. 1 changed file with 3 additions and 2 deletions.
    5 changes: 3 additions & 2 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -217,7 +217,7 @@ The argument has been made several times that it is up to the app builder to sec

    Enforcing that attributes have to be declared `attr_accessible` by default would immediately make things better.

    **What should the default authorization setting to be? On or off?**
    **What should the default authorization setting to be, on or off?**

    [Yehuda Katz makes the point](https://gist.github.com/1974187) that this is an authorization issue which is not a framework issue. The question here though is not *“where should authorization be handled”* but *“what should the default setting for authorization be”*.

    @@ -251,4 +251,5 @@ Blog: http://peternixey.com

    * [Egor Homakov’s how-to on how he hacked GitHub](http://homakov.blogspot.com/2012/03/how-to.html)
    * [Rails Spike’s “Is your application safe?”](http://railspikes.com/2008/9/22/is-your-rails-application-safe-from-mass-assignment)
    * [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)
    * [Yehuda Katz’s proposal for addressing mass assignment](https://gist.github.com/1974187)
    * [Hacker news discussion on this article](http://news.ycombinator.com/item?id=3666564)
  15. petenixey revised this gist Mar 6, 2012. No changes.
  16. petenixey revised this gist Mar 5, 2012. 1 changed file with 2 additions and 0 deletions.
    2 changes: 2 additions & 0 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -243,6 +243,8 @@ Author: Peter Nixey
    Twitter: http://twitter.com/peternixey
    Blog: http://peternixey.com

    ***PLEASE NOTE I AM NOT A MEMBER OF THE GITHUB TEAM. THIS IS JUST A GIST***

    ----

    *Further reading:*
  17. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -227,7 +227,7 @@ Where this should be handled is not an academic design choice, it’s one that

    **There are a lot of sites vulnerable and more being built every day**

    Rails is a brilliant framework designed by a brilliant team. One of the reasons I like coding in it is that I feel that I always learn from the code I find (in PHP I generally wanted to rewrite it). This weakness has always bugged me though and it doesn't do Rails justice.
    Rails is a brilliant framework designed by brilliant contributors. One of the reasons I like coding in it is that I feel that I always learn from the code I find (in PHP I generally wanted to rewrite it). This one weakness has always bugged me though and I feel it doesn't do Rails justice.

    If GitHub, one of the best Rails teams on the planet can be taken out so easily, in so many places by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.

  18. petenixey revised this gist Mar 5, 2012. 1 changed file with 2 additions and 0 deletions.
    2 changes: 2 additions & 0 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -227,6 +227,8 @@ Where this should be handled is not an academic design choice, it’s one that

    **There are a lot of sites vulnerable and more being built every day**

    Rails is a brilliant framework designed by a brilliant team. One of the reasons I like coding in it is that I feel that I always learn from the code I find (in PHP I generally wanted to rewrite it). This weakness has always bugged me though and it doesn't do Rails justice.

    If GitHub, one of the best Rails teams on the planet can be taken out so easily, in so many places by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.

    **A three step suggestion for how the Rails team could address the isssue:**
  19. petenixey revised this gist Mar 5, 2012. 1 changed file with 6 additions and 7 deletions.
    13 changes: 6 additions & 7 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -215,25 +215,24 @@ The argument has been made several times that it is up to the app builder to sec

    **If the Rails team are going to stand by the mantra then they also need to accept that the Rails Way to handle updates is conventionally insecure until configured otherwise.**


    Enforcing that attributes have to be declared `attr_accessible` by default would immediately make things better.

    **The question is not "where should authorization be handled" it is what is the default setting**
    **What should the default authorization setting to be? On or off?**

    [Yehuda Katz makes the point](https://gist.github.com/1974187) that this is an authorization issue which is not a framework issue. The question here though is not *“where should authorization be handled”* but *“what should the default setting for authorization be”*.

    In almost everywhere else the default setting for authorization is unauthorized. You can’t even reach a controller method unless you explicitly create a route for it. update_attributes however defaults to authorized.
    In most other public-facing interfaces in Rails the default setting for authorization is unauthorized. You can’t even reach a controller method unless you explicitly create a route for it. `update_attributes` however defaults to authorized.

    This is not an academic design choice, it’s one that’s carrying a real world cost right now. Arguing over what layer of the app is responsible is like BP blaming Transocean for the Deepwater Horizon. The issue is that oil is leaking.
    Where this should be handled is not an academic design choice, it’s one that’s carrying a real world cost right now. Arguing over what layer of the app is responsible is like BP blaming Transocean for the Deepwater Horizon. The issue is that oil is leaking.

    **There are a lot of sites currently vulnerable and more being built**
    **There are a lot of sites vulnerable and more being built every day**

    If GitHub, one of the best Rails teams on the planet can be taken out so easily by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.
    If GitHub, one of the best Rails teams on the planet can be taken out so easily, in so many places by such a simple hack then there is a real and present issue. As Yehuda says, not all security vulnerabilities can be fixed by the framework however this one can and it would make sense to do so.

    **A three step suggestion for how the Rails team could address the isssue:**

    * On the next release start raising warnings in development when an attempt is made to update an attribute without declaring it `attr_accessible`
    * On the release after raise warnings in production and errors in development
    * On the release after raise errors in development and warnings in production
    * On the third release make attr_accessible on by default

    ------
  20. petenixey revised this gist Mar 5, 2012. 1 changed file with 3 additions and 1 deletion.
    4 changes: 3 additions & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,9 @@

    ----

    *Please note: **THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM** or in any way associated with them. It's simply hosted as a [Gist](https://gist.github.com/) because the markdown formatting is excellent and far clearer than anything I could manage on my [personal Tumblr](http://peternixey.com).*
    *Please note: **THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM** or in any way associated with them. It's simply hosted as a [Gist](https://gist.github.com/) because the markdown formatting is excellent and far clearer than anything I could manage on my personal Tumblr at [peternixey.com](http://peternixey.com).*

    If you'd like to follow me on twitter my handle is [@peternixey](http://twitter.com/peternixey)

    ----

  21. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note: **THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM** or in any way associated with them. It's simply hosted as a [Gist](https://gist.github.com/) because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*
    *Please note: **THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM** or in any way associated with them. It's simply hosted as a [Gist](https://gist.github.com/) because the markdown formatting is excellent and far clearer than anything I could manage on my [personal Tumblr](http://peternixey.com).*

    ----

  22. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note: **this is not posted by the GitHub team** or in any way associated with them. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*
    *Please note: **THIS ARTICLE IS NOT WRITTEN BY THE GITHUB TEAM** or in any way associated with them. It's simply hosted as a [Gist](https://gist.github.com/) because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*

    ----

  23. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note **this is not posted by the GitHub team** or in any way associated with them. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*
    *Please note: **this is not posted by the GitHub team** or in any way associated with them. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*

    ----

  24. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note **this is not posted by the GitHub team** or in any way associated with them team. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*
    *Please note **this is not posted by the GitHub team** or in any way associated with them. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*

    ----

  25. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note this is not posted by GitHub or in any way associated with the GitHub team. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my [Tumblr](http://peternixey.com).*
    *Please note **this is not posted by the GitHub team** or in any way associated with them team. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my personal [Tumblr](http://peternixey.com).*

    ----

  26. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -20,7 +20,7 @@ Add the following initializer:

    ActiveRecord::Base.send(:attr_accessible, nil)

    *(this fix is not without its pitfalls - see later for things to watch for)*
    ***(this fix is not without its pitfalls - see later for things to watch for)***

    **What the initializer does**

  27. petenixey revised this gist Mar 5, 2012. 1 changed file with 1 addition and 1 deletion.
    2 changes: 1 addition & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -3,7 +3,7 @@

    ----

    *Please note this is not posted by GitHub or in any way an official post. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my [Tumblr](http://peternixey.com).*
    *Please note this is not posted by GitHub or in any way associated with the GitHub team. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my [Tumblr](http://peternixey.com).*

    ----

  28. petenixey revised this gist Mar 5, 2012. 1 changed file with 7 additions and 1 deletion.
    8 changes: 7 additions & 1 deletion securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -1,10 +1,16 @@
    ##How Homakov hacked GitHub and the line of code that could have prevented it


    ----

    *Please note this is not posted by GitHub or in any way an official post. It's simply hosted as a Gist because the markdown formatting is excellent and far clearer than anything I could manage on my [Tumblr](http://peternixey.com).*

    ----

    [@homakov’s](http://homakov.blogspot.com/) explot on GitHub was simple and straightforward. Calling it an attack makes it sound malicious whereas the truth was that GitHub bolted its front door but left the hinges on quick release. Homakov released the hinges, walked in and shouted to anyone who would listen that they had a problem.

    He was right. The Rails defaults are vulnerable and there’s no better illustration of this than when when one of the best Rails teams in the world is severely compromised.

    ----

    **TL;DR: How to protect your Rails application from the GitHub attack**

  29. petenixey revised this gist Mar 5, 2012. 1 changed file with 2 additions and 2 deletions.
    4 changes: 2 additions & 2 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -231,8 +231,8 @@ If GitHub, one of the best Rails teams on the planet can be taken out so easily
    ------

    Author: Peter Nixey
    Twitter: http://twitter
    Blog: http://peternixey.com .com/peternixey
    Twitter: http://twitter.com/peternixey
    Blog: http://peternixey.com

    ----

  30. petenixey revised this gist Mar 5, 2012. 1 changed file with 24 additions and 23 deletions.
    47 changes: 24 additions & 23 deletions securing_rails_updates.md
    Original file line number Diff line number Diff line change
    @@ -49,23 +49,23 @@ and a very simple User class:
    end


    **Why this `user` is vulnerable**
    **Why the User class is vulnerable**

    > u = User.create name: ‘Peter Nixey’, role: :subscriber;
    => #<User id: 1, role: :subscriber, name: "Peter Nixey",
    created_at: "2012-03-05 09:39:31", updated_at: "2012-03-05 09:39:31">

    By default, `update_attributes` (which is what you’ll probably use in your update method) updates any attributes that are passed into it - usually via `params[:model_name]`. It’s wonderfully quick and simple but open to abuse:

    `update_params` will for instance happily update not only our name but also our role:
    `update_params` will for instance happily update not only your name but also your role:

    > u.update_attributes name: ‘Jenson Button’, role: :superadmin;
    => #<User id: 1, role: "superadmin", name: "Jenson Button",
    created_at: "2012-03-05 09:39:31", updated_at: "2012-03-05 09:40:53">

    *By fiddling with the user update form we just updated our role from subscriber to superadmin.*

    This is not a good thing and is not really what we’d like as our default access setting.
    This is not good.

    ### How Homakov used update_attributes to hack the Rails GitHub account

    @@ -84,9 +84,9 @@ Homakov assumed (correctly) that GitHub had a table containing users’ public k
    end
    end

    Homakov PUT an update to his own existing public key which included a new `user_id`. The user_id he used was that of a member of the Rails repository team.
    Homakov PUT an update to his own existing public key which included a new `user_id`. The user_id he used was that of a member of the Rails repository members.

    The controller then simply updated all of the parameters Homakov passed it, including the new user_id. With an SSH key on his machine registered to the repository of a Rails member all he then needed to do was [push](https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57). This was the same hack he used for [posting from the future](https://github.com/rails/rails/issues/5239).
    The controller then simply updated all of the parameters Homakov passed it, including the new `user_id`. With an SSH key on his machine registered to the repository of a Rails member all he then needed to do was [push](https://github.com/rails/rails/commit/b83965785db1eec019edf1fc272b1aa393e6dc57). This was the same hack he used for [posting from the future](https://github.com/rails/rails/issues/5239).

    **Why don't I just avoid `update_attributes`?**

    @@ -100,7 +100,7 @@ We could do but it would take five lines where update_attributes only takes one

    **How to protect update_attributes: attr_protected (not recommended)**

    Everything that happened happened because the user_id attribute should not have been updatable via update_attributes. Rails has a method to prevent exactly this and it’s called `attr_protected`.
    Everything that happened happened because the `user_id` attribute should not have been updatable via update_attributes. Rails has a method to prevent exactly this and it’s called `attr_protected`.

    class User < ActiveRecord::Base
    attr_protected :role
    @@ -113,9 +113,7 @@ With that line added it doesn’t matter whether we pass the role in via a PUT,
    WARNING: Can't mass-assign protected attributes: role
    => true

    The problem is that `attr_protected` only protects us on attributes we actually add it to

    The problem with `attr_protected` is that it only works when we remember to add it. If we don’t put it in we don’t get protection.
    The problem is that `attr_protected` only protects us on attributes we actually declare to be `attr_protected`. It only works when we remember to add it. If we don’t put it in we don’t get protection.

    I prefer to know I’m protected and safe until I chose to be unsafe and that (in theory) is what `attr_accessible` gives us.

    @@ -129,7 +127,7 @@ Delcaring any attribute as `attr_accessible` implies that all the other attribut
    attr_accessible :name
    end

    role is now protected since we haven't declared it `attr_accessible`:
    `:role` is now protected since we haven't declared it `attr_accessible`:

    u = User.create name: "Peter Nixey", role: :subscriber;
    u.update_attributes role: :superadmin
    @@ -151,60 +149,63 @@ and leave our model unchanged:
    attr_accessible :name
    end

    then the `account_type` field is automatically protected for us:
    then the `account_type` field is automatically protected:

    u = User.create name: “Peter Nixey”, account_type: ‘free’
    u = User.create name: “Peter Nixey”, account_type: ‘free’;
    u.update_attributes account_type: ‘paid’
    WARNING: Can't mass-assign protected attributes: account_type
    => true

    **Either way you can still manually update attributes**

    We can still update role directly it’s simply that it’s not vulnerable to an update through our update controller:
    We've not locked ourselves out of our own model. We can still update role directly it’s simply that it’s not vulnerable to being injected during `update_attributes`

    u.role = :superadmin
    u.save
    (0.2ms) UPDATE "users" SET "role" = 'superadmin',
    "updated_at" = '2012-03-05 10:11:05.042023' WHERE "users"."id" = 1
    => true

    **Even attr_accessible only protects us when we remember it**
    **However, even attr_accessible only protects us when we remember it**

    The problem with `attr_protected` was that it only protected us when we remembered to add it to the attribute.

    The problem with `attr_accessible` is that it only protects us when we remember to add it to the model. Sometimes (as GitHub showed us) it’s easy to forget to do that.

    ###The disable_mass_assignment initializer gives security by default
    ###The disable_mass_assignment initializer protects us by default

    Create a new file:
    *config/initializers/disable_mass_assignment.rb*

    ActiveRecord::Base.send(:attr_accessible, nil)

    The beauty of this is that it effectively adds `attr_accessible` to every model we create (actually what it does it take it away by default but it comes to the same thing). No attribute can be updated unless we declare it `attr_accessible`. We’re secure until we decide not to be.
    The beauty of this is that it effectively adds `attr_accessible` to every model we create (actually what it does it take it away by default but it comes to the same thing). No attribute can be updated unless we declare it `attr_accessible`. We’re secure until we decide otherwise.

    **Possible issues you might have with the initializer**

    If you add `attr_accessible` the first thing you’re going to need to do is to declare the relevant attributes as `attr_accessible`.
    Once you setup the initializer, the first thing you’re going to need to do is declare all relevant attributes as `attr_accessible`.

    A good test suite will help a lot here but with or without one, go through each model adding each parameter that you want accessible to your `attr_accessible` arguments:
    A good test suite will help a lot here but either way you need to go through each model adding each parameter that you want to be accessible to your `attr_accessible` arguments:

    class Xyz
    attr_accessible attribute_a, attribute_b, attribute_c
    class User
    attr_accessible :email, :first_name, :last_name, :full_name
    end

    You’re going to have some frustrations. There are going to be things that you don’t see coming which will fail silently. Problems I’ve had are:

    - Authlogic: you need to remember to make attributes like password, email etc accessible
    - Paperclip: remember to make paperclip attributes accessible
    - Nested attributes: Instructions here: http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#label-Using+with+attr_accessible
    - Nested attributes: [Instructions here](http://api.rubyonrails.org/classes/ActiveRecord/NestedAttributes/ClassMethods.html#label-Using+with+attr_accessible)

    I’m sure you’ll hit other issues too but you can generally knock them off by adding attributes one by one to the list of accessible ones.

    **How Rails could address this**
    ###How Rails could address this

    I wouldn’t pretend to have anything like the oversight of the Rails landscape that the Rails core team do. I’ve only built a very few apps and I’m no guru. However...

    The argument that it is up to the app builder to secure their own app is however dangerous. Rails’ mantra is convention over configuration. **If the Rails team are going to stand by this mantra then they also need to accept that the Rails Way to handle updates is conventionally insecure until configured otherwise.**
    The argument has been made several times that it is up to the app builder to secure their own app. I don't agree with this though. Rails’ mantra is convention over configuration.

    **If the Rails team are going to stand by the mantra then they also need to accept that the Rails Way to handle updates is conventionally insecure until configured otherwise.**


    Enforcing that attributes have to be declared `attr_accessible` by default would immediately make things better.