8 ms·

How Homakov hacked GitHub & the line of code that could have prevented it

316 points by petenixey 15y ago

yumraj 15y ago
I must say that this episode is the best example of how to handle such cases. Homakov found the issue and made his point without any sign of maliciousness. Github, also handled is extremely professionally by accepting it and fixing the problem and then publishing a full report on it, rather than get into a pissing match with Homakov and getting law enforcement and lawyers involved. BigCo's should take a note.
- rickchristie 15y ago
  Read his blog: http://homakov.blogspot.com/ http://homakov.blogspot.com/ - GitHub suspended his account.
  - technoweenie 15y ago
    It was a temporary suspension while we investigated the scope of the damage. There was some other weird activity on his account that he wasn't up front about.
jgrahamc 15y ago
It's surprising to me (not a very experienced Rails developer) that the default behavior should be so open to abuse. Why isn't the default the opposite?
- Peaker 15y ago
  Because convenience is deemed more important than safety. Same reason that "enum" and "int" are pretty much interchangeable in C, that arithmetic conversions and truncations are implicit -- it's not very safe, but it is more convenient.
  - baq 15y ago
    you know, i've heard that from php. it was damn convenient in version 3. we all know how it ended up.
LargeWu 15y ago
Just a heads up for those who only read the TL;DR: Please be aware that if you just put that single line of code in your initializers for your existing app, your app will break anywhere you are using update_attributes(). They do call it out later in the article, but you have to set attr_accessible on all your models.
- petenixey 15y ago
  Good flag - I've made the warning even clearer in the Gist
kristofferR 15y ago
I wonder how many Rails apps there are out there that is still vulnerable to this sort of flaw. Both GitHub and Posterous has fixed it, but there's probably thousands of smaller less known Rails sites/apps that still haven't been patched.
- danso 15y ago
  I know I immediately panicked because I know this was not one of the many issues that I had thought to be aware of. Luckily, I remembered that I've just out of habit/circumstance not used update_attributes...so I avoided the bullet out of dumb luck. I imagine there are many, many sites that are vulnerable to this. I hope the high profile hack (at least on HN) quickly spreads the kind of panic that gets other devs to check their repos today.
  - dean 15y ago
    Although the OP talks about update_attributes, the problem is not limited to that method. The problem is with "mass assignment". So, if you are passing a hash of attributes and values to the new or create method of a model, then you are doing mass assignment, and you are exposed to this issue. For example, if you are doing this in your controller: @user = User.new(params[:user]) then you are doing mass assignment.
deleted 15y ago
[deleted]
- generators 15y ago
  and hence, like hn did with google+ subdomain, every link should include subdomain if its not www.
- deleted 15y ago
  [deleted]
gphil 15y ago
I posted a link to the relevant part of it (http://news.ycombinator.com/item?id=3665429 http://news.ycombinator.com/item?id=3665429) on another thread regarding this exploit already, but the official Rails Security Guide covers this and other common security pitfalls really well. It is worth reading over thoughtfully if you are building a Rails app: http://guides.rubyonrails.org/security.html http://guides.rubyonrails.org/security.html
- davidblondeau 15y ago
  Right, the Rails security guide and the Netscape secure coding guide (https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines) are highly recommended reading for every developer at my company. They are very well written and cover a lot of ground when developing web applications on RoR.
rmc 15y ago
I'm a Python/Django developer and don't really know much Ruby or Ruby on Rails. Does anyone have an outsiders/non-rubyist explaination of how this hack was carried out? Did he modify HTTP headers? POST parameters?
- thecoffman 15y ago
  You would modify the hidden form values (or add parameters that didn't exist) depending on the situation. These new/modified parameters would appear in Rails' params hash which would then be passed to the update function which, by default, will update any fields you hand it.
  - SpoonMeiser 15y ago
    This really doesn't sound like a problem with the update function, this just sounds like web applications 101 - don't trust user data.
- carlio 15y ago
  From what I can tell, the 'rails way' of dealing with form data is something akin to user.__dict__.update(request.POST). So you can just add some additional parameters to your POST data and set arbitrary fields on the model.
  - danso 15y ago
    I don't think this is the best way to describe this. It is not the "Rails Way" to NOT validate form input. The Github team used a method that allows quick and easy assignment of bulk values. This method is used often in non-public-facing tasks but when it is used in connection with a public facing POST request, then validation logic should be implemented. It was not, in Github's case. It seems like a glaring error in retrospect but it's easy to see how this code (or the pattern) would move over from the private to public-facing interface and not trigger any errors or notice.
    - deleted 15y ago
      [deleted]
danso 15y ago
I hope those who ripped into Homakov initially are feeling more mellow towards him. They certainly were justified in thinking him being rash, but his action raised far more awareness about this issue than the typical proper channel. Rails devs (some, not all of them) had dismissed his complaint because "Every Rails developer hears about attr_accessible." Well, I'll be the first to say that I can't remember the last time that this update_attributes vulnerability had been pointed out to me. I certainly can remember all the times that Rails docs reminds me to use their sanitizing helpers when making ActiveRecord queries. To be fair, I haven't developed apps that required the use of user-facing access to update_attributes, and maybe when I got around to using that, I would've wisely consulted the dev guides to make sure I was following best practice. But knowing me, I probably would've likely thought, "Well, that seems simple enough, here goes." It's not that the logic behind this vulnerability is hard to understand...in retrospect, it's as clear and blatant as the processes that lead to SQL injection. But surgical patients die because elite surgeons sometimes forget to wash their hands (Google "Atul Gawande checklist"). It's not an impossibility that a skilled dev team would overlook the update_attributes issue. The Rails team was right in arguing that this wasn't a security risk given a half-competent dev. But they were looking at the problem from the wrong perspective and assumed that everyone is as familiar with Rails best practices as they were. So how else could Homakov convince them otherwise other than pricking a high-profile dev group? What if Homakov managed to alert the Github team, and they managed to fix it quietly? Github would be safe but thousands of Rails sites would still be operating in ignorance. It truly stinks for the Github group that they had to respond to a five-alarm emergency on a Sunday...on the other hand, I think there are going to be a lot of Rails devs who are thankful that they (involuntarily) took one for the team. Thanks to Homakov, it was a small hit.
- ovi256 15y ago
  On that checklist remark, I'd like to see and use more checklists. Hopefully some smarter person will write the "How to put a Rails app to prod" checklist, now that the need is obvious. Same for Django, or any other popular stack. It would be great if the communities created and maintained checklists summarizing best practices for different tasks.
  - 15y ago
madarco 15y ago
It's sad that this same bug was resolved in CakePhp 7years* ago: http://groups.google.com/group/cake-php/browse_thread/thread/03ce2bc624d335b9/eeb5f847369a74d1 http://groups.google.com/group/cake-php/browse_thread/thread...
adriand 15y ago
I agree that the default for Rails should be more secure, but this was a really basic mistake by the GitHub team. Yes, mistakes do happen, but there's a very simple way to avoid the exploit that is postulated in this article. The 'schematic' of what the public key update looks like from the original post: class PublicKeyController < ApplicationController before_filter :authorize_user ... def update @current_key = PublicKey.find_by_id params[:key]['id'] @current_key.update_attributes(params[:key]) end end The correct way to code this is as follows: class PublicKeyController < ApplicationController before_filter :authorize_user ... def update @current_key = current_user.public_keys.find params[:key]['id'] @current_key.update_attributes(params[:key]) end end This has two updates to it that protect against this exploit: 1. Rather than calling "find_by_id" on the PublicKey model, which searches all public keys, you call it on the current user's list of public keys. This scopes the search down to the public keys that they own. Thus, if you pass in the id of a key they do not own, it will not be found, leading us to: 2. Using "find" instead of "find_by_id" will trigger an ActiveRecord::RecordNotFound error (404) if the resource is not found. Of course, find_by_id will just return nil in this instance, so the update_attributes part would still fail, but triggering a 404 is an easier, cleaner way of dealing with this, I think. It's really very simple: you don't let people access stuff they don't own. Now, this does not protect you against faked timestamps, or against privilege escalation by passing in a faked "role" parameter, and so on. You still need to use attr_accessible to protect yourself from that stuff, but scoping resources down to the user who owns them is a simple technique that should be standard practice for applications with authentication.
- jgrahamc 15y ago
  Doesn't the line: @current_key.update_attributes(params[:key]) still mean that an attacker can pass in arbitrary fields to be updated in the public key table? If that's the case then doesn't it open room for an attacker to change a field that is assumed not accessible from the outside? For example, would it be possible to change an _id field that references another table?
  - adriand 15y ago
Estragon 15y ago
Since this vulnerability has been known about for years and experienced crackers have presumably checked for it before, is there any reason to be concerned about more malicious corruption of other repositories on github?
- phamilton 15y ago
  The problems with update_attributes have been known, and messing with things like timestamps probably has been done before, but Rails itself didn't violate any acls. All users still can only access their own resources via Rails. This attack was a combination of Rails and git/ssh keys. There's a bit of a clever aspect to it, one that isn't implied merely by understanding the vulnerabilities of update_attributes. While I think it is likely someone else has thought of it before, it is a little more exotic than something your average cracker is going to try.
  - danso 15y ago
    Really? How is this exotic? Simple inspection of the rails form will reveal attribute and model names. And as Homarov proves, you don't need to even leave the friendly interface of the inspector to POST what you want. The only reason why a cracker hasn't tried this is because it seems too simple to work.
    - javascriptlol 15y ago
      That is, assuming it hasn't been tried before. We don't really know.
  - Estragon 15y ago
    OK, but that doesn't really answer my question, which is why people aren't more concerned about earlier more surreptitious corruptions of the repositories of github via the same vulnerability. It's got to have been an attractive target for the likes of the Operation Aurora [1] folks. [1] http://en.wikipedia.org/wiki/Operation_Aurora http://en.wikipedia.org/wiki/Operation_Aurora
feralchimp 15y ago
I'm not a Ruby or Rails guy, so I can't comment on how likely a typical (or atypically good) Rails dev might be to code this sort of bug into their app. But as a security guy, given the power of Public Key assignment in the context of a system for managing access to Git repositories, I can't help but be a little surprised that model objects that touch Public Keys weren't more thoroughly reviewed. If nothing else, folks everywhere will be thinking a little harder about authorization logic this week.
forza 15y ago
GitHub really needs to clearify if access to private repos was compromised, for how long and if such access would be traceable.
- why-el 15y ago
  Between the time Homakov made his work public and the time it took them to fix it? I think it was close to an hour and on a Sunday. I doubt there are any other cases.
  - cbs 15y ago
    And we're confident that this guy is the only guy to ever exploit that weakness on github in the last 4 years?
    - why-el 15y ago
      No, we are not. My bad, I misunderstood the statement; I thought he was referencing yesterday only, which I thought might not be possible, given the sort amount of time. Obviously if other people knew that's a whole different story.
- ergo14 15y ago
  Possibly years? They probably wont know either - I can imagine that this could be hard to track if someone is careful enough.
o1iver 15y ago
"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." But wouldn't that mean, that the commit would display the username of the user with the `user_id' that he used?
- gcampbell 15y ago
  Github displays the username/avatar for a commit based on that commit's Author field, not the user that pushed the changes to the repository.
  - o1iver 15y ago
    Ok, I see. Thanks!
  - ajross 15y ago
    It's more that they display both. Unlike git core, github actually tracks "push" events to branches (git doesn't care, it only sees commits) as distinct from the commits they contain. So presumably it would have shown the rails developer pushing a change authored by Homakov.
    - technoweenie 15y ago
      Git has no real way to track that, only commits. We do track it internally, but AFAIK it's not exposed except through post-receive hooks.
    - Tobu 15y ago
      ref changes are tracked in the reflog, but the "pusher" (as opposed to author and committer) isn't.
- judofyr 15y ago
  GitHub uses the email-field in the commit to connect a user to a commit.
SpoonMeiser 15y ago
Is it just me, or doesn't this sound very similar to SQL injection, only as applied to an ORM instead? That is, if my understanding is correct, they're taking user posted data and trivially turning it into a command to update data. This doesn't sound like a problem with Rails, in the same way that if I turn data I receive from the user straight into an SQL statement, the fact that people can abuse it isn't a problem with SQL.
- tedunangst 15y ago
  It's a problem with Rails because Rails provided the update facility that takes user data and told people "if you use this, you can build a blog in 15 minutes." If you're turning user data into SQL, at least you're the one who wrote the code. Your database didn't come with a parse POST and update table function.
- javascriptlol 15y ago
  SQL injection is a problem with the way database libraries are designed. Just because some idiotic piece of design has persisted for years doesn't change what it is: a vulnerability.
- einhverfr 15y ago
  They are both confused deputy problems so they are both pretty similar. I think you are right to point out the similarity. I have seen worse though :-P
unclegene 15y ago
Am I the only one who does not understand what is this about? Oh, no, looks like rails team does neither. Stupid code can be written in any framework/language. How much experience does one need to understand a simple rule - _never_ use user input directly. If you have an urge to trust your users - I'd suggest better way: `params[:command]`
unclegene 15y ago
jarrett 15y ago
Homakov kind of gave the impression he had discovered a previously unknown vulnerability in Rails. This is not the case. Rather, he discovered an instance in which one very prominent Rails app (Github) failed to implement a standard Rails security practice. For those not familiar with Rails, it boils down to this: You as the programmer need to use a security feature built into Rails called mass assignment security. If you fail to use this feature, you have a vulnerability. In other words, the default is insecure by design. The alternative would be to make Rails secure by default, but that would mean pretty much nothing would work until you explicitly granted access where necessary. I guess the core team figured "not working by default" was worse than "insecure by default." Homakov obviously disagreed with this design decision. I can understand why, and I mostly feel the same way. So Homakov posted an issue to the Rails repo Github (https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228) suggesting the default be changed. He made a good case and was initially polite. A few days passed, and nobody else had posted to his thread. So, presumably to draw attention to this issue, he exploited the fact that Github had failed to use mass assignment protection. Specifically, he posted a comment with a far-future timestamp, which obviously should be impossible. (I think that's what he did, although Github seems to to have fixed the timestamp now.) He then said this should be proof enough that the Rails defaults need to be changed. The problem with Homakov's argument, as pointed out in subsequent comments in the thread, is that Homakov's hack only demonstrated a mistake on Github's part, not a bug in Rails. It didn't prove anything about Rails that we didn't already know. The only thing surprising he demonstrated was that Github had left open a rather serious vulnerability. TL;DR: Rails has some less-than-secure defaults which all Rails developers are expected to understand and deal with. Homakov found out that Github failed to do so in at least one instance, and he wanted to use that as proof the Rails defaults should be changed.
- joeyh 15y ago
  Web frameworks like Yesod have shown that it's possible to make a web framework that is secure by design (at the type level, even) without compromising on usability.
- danso 15y ago
  I agree with you that this is not a Rails bug. But I disagree that Homakov's hack "only demonstrated a mistake on Github's part." It rebutted the Rails team opinion (and again, not all of them disagreed with Homakov) that this was a trivial, edge-case problem. IIRC, one of the changes in Rails 3 was that interpolation in ERb templates were html sanitized by default: http://stackoverflow.com/questions/4731992/rails-3-how-to-reverse-erb-default-behaviour-so-i-can-inject-html http://stackoverflow.com/questions/4731992/rails-3-how-to-re... The fact that web devs write templates vulnerable to XSS is not Rails fault, but apparently the problem was prevalent enough that HTML sanitizing was turned on by default. Apparently, there wasn't empirical evidence to show that update_attributes had the same rate of mistakes to justify a change in defaults...Homakov's hack was a powerful rebuttal.
jazzychad 15y ago
Is it really frowned upon to do: user.name = params[:user][‘name’] ?? Call me old fashioned, but this is called 'defensive coding' and should (in my opinion) be the norm when dealing with client-generated input. It might be more verbose and not 'The Rails Way', but update_attributes seems like too much magic for my paranoid taste.
- danso 15y ago
  It's not frowned upon, but it would require tighter coupling between the controller and the view...?
  - jazzychad 15y ago
    Yes, that makes sense. If your view's form should only update the user's name, then the logic that handles that form submission should only update the user's name. Allowing the client to inject other attributes (even allowed/whitelisted ones) could potentially break other app logic.
lattepiu 15y ago
The real culprit here is HashWithIndifferentAccess, a crutch that throws away a difference that Ruby has for a reason. The sensible way to do updates, in my opinion, is User.update(:name => params['user']['name']) But there's no way in Rails to keep that syntax while disabling User.update(params['user'])
- bluesnowmonkey 15y ago
  clutch -> crutch
Shanewho 15y ago
Is the solution given in the article any different than using this? config.active_record.whitelist_attributes = true Also, this isn't the first time someone's been bit by this: http://www.kalzumeus.com/2010/09/22/security-lessons-learned-from-the-diaspora-launch/ http://www.kalzumeus.com/2010/09/22/security-lessons-learned...
- petenixey 15y ago
  I didn't know about the solution you mention at the time I wrote the post. I'm reading up on it now and I think it's actually going to end up being the official solution so it's a good flag: https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736 https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac...
jimworm 15y ago
DHH does it in half a line: https://gist.github.com/1975644 https://gist.github.com/1975644 `attr_accessible` should only be used to protect the attributes that are NEVER modified by users. Access to the rest of the attributes may differ by user role, and should be handled by the controller. Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security.
- marshray 15y ago
  `attr_accessible` should only be used to protect the attributes that are NEVER modified by users But web app developer may not be able to know in advance what new columns might be added on the database, possibly by some other team. If I understand this right, in the absence of attr_accessible, any new columns are completely writable by the HTTP request. So having a default-deny whitelist approach is the only sane strategy. Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security. Or give up on Rails. Usually the basic security of the database is not negotiable.
pdufour 15y ago
I don't find this a good solution. A much better solution is to generate a list of form inputs that are present on a page, then when the form is submitted, check if any of the form inputs were changed / any were added.
- obtu 15y ago
  Agreed, whitelisting attributes regardless of context is much too coarse. Here is a form library that implements what you describe: https://news.ycombinator.com/item?id=3666907 https://news.ycombinator.com/item?id=3666907
InclinedPlane 15y ago
I find it hilarious that just in the last few days they've removed register_globals from PHP for good. It seems every language/framework ends up retracing the same set of security/convenience tradeoffs over and over again.

316 points by petenixey 15y ago

I must say that this episode is the best example of how to handle such cases. Homakov found the issue and made his point without any sign of maliciousness. Github, also handled is extremely professionally by accepting it and fixing the problem and then publishing a full report on it, rather than get into a pissing match with Homakov and getting law enforcement and lawyers involved. BigCo's should take a note.

rickchristie 15y ago
Read his blog: http://homakov.blogspot.com/ http://homakov.blogspot.com/ - GitHub suspended his account.
- technoweenie 15y ago
  It was a temporary suspension while we investigated the scope of the damage. There was some other weird activity on his account that he wasn't up front about.

jgrahamc 15y ago

It's surprising to me (not a very experienced Rails developer) that the default behavior should be so open to abuse. Why isn't the default the opposite?

Peaker 15y ago
Because convenience is deemed more important than safety. Same reason that "enum" and "int" are pretty much interchangeable in C, that arithmetic conversions and truncations are implicit -- it's not very safe, but it is more convenient.
- baq 15y ago
  you know, i've heard that from php. it was damn convenient in version 3. we all know how it ended up.

LargeWu 15y ago

Just a heads up for those who only read the TL;DR: Please be aware that if you just put that single line of code in your initializers for your existing app, your app will break anywhere you are using update_attributes(). They do call it out later in the article, but you have to set attr_accessible on all your models.

petenixey 15y ago
Good flag - I've made the warning even clearer in the Gist

kristofferR 15y ago

I wonder how many Rails apps there are out there that is still vulnerable to this sort of flaw. Both GitHub and Posterous has fixed it, but there's probably thousands of smaller less known Rails sites/apps that still haven't been patched.

danso 15y ago
I know I immediately panicked because I know this was not one of the many issues that I had thought to be aware of. Luckily, I remembered that I've just out of habit/circumstance not used update_attributes...so I avoided the bullet out of dumb luck. I imagine there are many, many sites that are vulnerable to this. I hope the high profile hack (at least on HN) quickly spreads the kind of panic that gets other devs to check their repos today.
- dean 15y ago
  Although the OP talks about update_attributes, the problem is not limited to that method. The problem is with "mass assignment". So, if you are passing a hash of attributes and values to the new or create method of a model, then you are doing mass assignment, and you are exposed to this issue. For example, if you are doing this in your controller: @user = User.new(params[:user]) then you are doing mass assignment.

deleted 15y ago

[deleted]

generators 15y ago
and hence, like hn did with google+ subdomain, every link should include subdomain if its not www.
deleted 15y ago
[deleted]

gphil 15y ago

I posted a link to the relevant part of it (http://news.ycombinator.com/item?id=3665429 http://news.ycombinator.com/item?id=3665429) on another thread regarding this exploit already, but the official Rails Security Guide covers this and other common security pitfalls really well. It is worth reading over thoughtfully if you are building a Rails app: http://guides.rubyonrails.org/security.html http://guides.rubyonrails.org/security.html

davidblondeau 15y ago
Right, the Rails security guide and the Netscape secure coding guide (https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines https://wiki.mozilla.org/WebAppSec/Secure_Coding_Guidelines) are highly recommended reading for every developer at my company. They are very well written and cover a lot of ground when developing web applications on RoR.

rmc 15y ago

I'm a Python/Django developer and don't really know much Ruby or Ruby on Rails. Does anyone have an outsiders/non-rubyist explaination of how this hack was carried out? Did he modify HTTP headers? POST parameters?

thecoffman 15y ago
You would modify the hidden form values (or add parameters that didn't exist) depending on the situation. These new/modified parameters would appear in Rails' params hash which would then be passed to the update function which, by default, will update any fields you hand it.
- SpoonMeiser 15y ago
  This really doesn't sound like a problem with the update function, this just sounds like web applications 101 - don't trust user data.
carlio 15y ago
From what I can tell, the 'rails way' of dealing with form data is something akin to user.__dict__.update(request.POST). So you can just add some additional parameters to your POST data and set arbitrary fields on the model.
- danso 15y ago
  I don't think this is the best way to describe this. It is not the "Rails Way" to NOT validate form input. The Github team used a method that allows quick and easy assignment of bulk values. This method is used often in non-public-facing tasks but when it is used in connection with a public facing POST request, then validation logic should be implemented. It was not, in Github's case. It seems like a glaring error in retrospect but it's easy to see how this code (or the pattern) would move over from the private to public-facing interface and not trigger any errors or notice.
  - deleted 15y ago
    [deleted]

danso 15y ago

I hope those who ripped into Homakov initially are feeling more mellow towards him. They certainly were justified in thinking him being rash, but his action raised far more awareness about this issue than the typical proper channel. Rails devs (some, not all of them) had dismissed his complaint because "Every Rails developer hears about attr_accessible." Well, I'll be the first to say that I can't remember the last time that this update_attributes vulnerability had been pointed out to me. I certainly can remember all the times that Rails docs reminds me to use their sanitizing helpers when making ActiveRecord queries. To be fair, I haven't developed apps that required the use of user-facing access to update_attributes, and maybe when I got around to using that, I would've wisely consulted the dev guides to make sure I was following best practice. But knowing me, I probably would've likely thought, "Well, that seems simple enough, here goes." It's not that the logic behind this vulnerability is hard to understand...in retrospect, it's as clear and blatant as the processes that lead to SQL injection. But surgical patients die because elite surgeons sometimes forget to wash their hands (Google "Atul Gawande checklist"). It's not an impossibility that a skilled dev team would overlook the update_attributes issue. The Rails team was right in arguing that this wasn't a security risk given a half-competent dev. But they were looking at the problem from the wrong perspective and assumed that everyone is as familiar with Rails best practices as they were. So how else could Homakov convince them otherwise other than pricking a high-profile dev group? What if Homakov managed to alert the Github team, and they managed to fix it quietly? Github would be safe but thousands of Rails sites would still be operating in ignorance. It truly stinks for the Github group that they had to respond to a five-alarm emergency on a Sunday...on the other hand, I think there are going to be a lot of Rails devs who are thankful that they (involuntarily) took one for the team. Thanks to Homakov, it was a small hit.

ovi256 15y ago
On that checklist remark, I'd like to see and use more checklists. Hopefully some smarter person will write the "How to put a Rails app to prod" checklist, now that the need is obvious. Same for Django, or any other popular stack. It would be great if the communities created and maintained checklists summarizing best practices for different tasks.
- 15y ago

madarco 15y ago

It's sad that this same bug was resolved in CakePhp 7years* ago: http://groups.google.com/group/cake-php/browse_thread/thread/03ce2bc624d335b9/eeb5f847369a74d1 http://groups.google.com/group/cake-php/browse_thread/thread...

adriand 15y ago

I agree that the default for Rails should be more secure, but this was a really basic mistake by the GitHub team. Yes, mistakes do happen, but there's a very simple way to avoid the exploit that is postulated in this article. The 'schematic' of what the public key update looks like from the original post: class PublicKeyController < ApplicationController before_filter :authorize_user ... def update @current_key = PublicKey.find_by_id params[:key]['id'] @current_key.update_attributes(params[:key]) end end The correct way to code this is as follows: class PublicKeyController < ApplicationController before_filter :authorize_user ... def update @current_key = current_user.public_keys.find params[:key]['id'] @current_key.update_attributes(params[:key]) end end This has two updates to it that protect against this exploit: 1. Rather than calling "find_by_id" on the PublicKey model, which searches all public keys, you call it on the current user's list of public keys. This scopes the search down to the public keys that they own. Thus, if you pass in the id of a key they do not own, it will not be found, leading us to: 2. Using "find" instead of "find_by_id" will trigger an ActiveRecord::RecordNotFound error (404) if the resource is not found. Of course, find_by_id will just return nil in this instance, so the update_attributes part would still fail, but triggering a 404 is an easier, cleaner way of dealing with this, I think. It's really very simple: you don't let people access stuff they don't own. Now, this does not protect you against faked timestamps, or against privilege escalation by passing in a faked "role" parameter, and so on. You still need to use attr_accessible to protect yourself from that stuff, but scoping resources down to the user who owns them is a simple technique that should be standard practice for applications with authentication.

jgrahamc 15y ago
Doesn't the line: @current_key.update_attributes(params[:key]) still mean that an attacker can pass in arbitrary fields to be updated in the public key table? If that's the case then doesn't it open room for an attacker to change a field that is assumed not accessible from the outside? For example, would it be possible to change an _id field that references another table?
- adriand 15y ago

Estragon 15y ago

Since this vulnerability has been known about for years and experienced crackers have presumably checked for it before, is there any reason to be concerned about more malicious corruption of other repositories on github?

phamilton 15y ago
The problems with update_attributes have been known, and messing with things like timestamps probably has been done before, but Rails itself didn't violate any acls. All users still can only access their own resources via Rails. This attack was a combination of Rails and git/ssh keys. There's a bit of a clever aspect to it, one that isn't implied merely by understanding the vulnerabilities of update_attributes. While I think it is likely someone else has thought of it before, it is a little more exotic than something your average cracker is going to try.
- danso 15y ago
  Really? How is this exotic? Simple inspection of the rails form will reveal attribute and model names. And as Homarov proves, you don't need to even leave the friendly interface of the inspector to POST what you want. The only reason why a cracker hasn't tried this is because it seems too simple to work.
  - javascriptlol 15y ago
    That is, assuming it hasn't been tried before. We don't really know.
- Estragon 15y ago
  OK, but that doesn't really answer my question, which is why people aren't more concerned about earlier more surreptitious corruptions of the repositories of github via the same vulnerability. It's got to have been an attractive target for the likes of the Operation Aurora [1] folks. [1] http://en.wikipedia.org/wiki/Operation_Aurora http://en.wikipedia.org/wiki/Operation_Aurora

feralchimp 15y ago

I'm not a Ruby or Rails guy, so I can't comment on how likely a typical (or atypically good) Rails dev might be to code this sort of bug into their app. But as a security guy, given the power of Public Key assignment in the context of a system for managing access to Git repositories, I can't help but be a little surprised that model objects that touch Public Keys weren't more thoroughly reviewed. If nothing else, folks everywhere will be thinking a little harder about authorization logic this week.

forza 15y ago

GitHub really needs to clearify if access to private repos was compromised, for how long and if such access would be traceable.

why-el 15y ago
Between the time Homakov made his work public and the time it took them to fix it? I think it was close to an hour and on a Sunday. I doubt there are any other cases.
- cbs 15y ago
  And we're confident that this guy is the only guy to ever exploit that weakness on github in the last 4 years?
  - why-el 15y ago
    No, we are not. My bad, I misunderstood the statement; I thought he was referencing yesterday only, which I thought might not be possible, given the sort amount of time. Obviously if other people knew that's a whole different story.
ergo14 15y ago
Possibly years? They probably wont know either - I can imagine that this could be hard to track if someone is careful enough.

o1iver 15y ago

"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." But wouldn't that mean, that the commit would display the username of the user with the `user_id' that he used?

gcampbell 15y ago
Github displays the username/avatar for a commit based on that commit's Author field, not the user that pushed the changes to the repository.
- o1iver 15y ago
  Ok, I see. Thanks!
- ajross 15y ago
  It's more that they display both. Unlike git core, github actually tracks "push" events to branches (git doesn't care, it only sees commits) as distinct from the commits they contain. So presumably it would have shown the rails developer pushing a change authored by Homakov.
  - technoweenie 15y ago
    Git has no real way to track that, only commits. We do track it internally, but AFAIK it's not exposed except through post-receive hooks.
  - Tobu 15y ago
    ref changes are tracked in the reflog, but the "pusher" (as opposed to author and committer) isn't.
judofyr 15y ago
GitHub uses the email-field in the commit to connect a user to a commit.

SpoonMeiser 15y ago

Is it just me, or doesn't this sound very similar to SQL injection, only as applied to an ORM instead? That is, if my understanding is correct, they're taking user posted data and trivially turning it into a command to update data. This doesn't sound like a problem with Rails, in the same way that if I turn data I receive from the user straight into an SQL statement, the fact that people can abuse it isn't a problem with SQL.

tedunangst 15y ago
It's a problem with Rails because Rails provided the update facility that takes user data and told people "if you use this, you can build a blog in 15 minutes." If you're turning user data into SQL, at least you're the one who wrote the code. Your database didn't come with a parse POST and update table function.
javascriptlol 15y ago
SQL injection is a problem with the way database libraries are designed. Just because some idiotic piece of design has persisted for years doesn't change what it is: a vulnerability.
einhverfr 15y ago
They are both confused deputy problems so they are both pretty similar. I think you are right to point out the similarity. I have seen worse though :-P

unclegene 15y ago

Am I the only one who does not understand what is this about? Oh, no, looks like rails team does neither. Stupid code can be written in any framework/language. How much experience does one need to understand a simple rule - _never_ use user input directly. If you have an urge to trust your users - I'd suggest better way: `params[:command]`

unclegene 15y ago

jarrett 15y ago

Homakov kind of gave the impression he had discovered a previously unknown vulnerability in Rails. This is not the case. Rather, he discovered an instance in which one very prominent Rails app (Github) failed to implement a standard Rails security practice. For those not familiar with Rails, it boils down to this: You as the programmer need to use a security feature built into Rails called mass assignment security. If you fail to use this feature, you have a vulnerability. In other words, the default is insecure by design. The alternative would be to make Rails secure by default, but that would mean pretty much nothing would work until you explicitly granted access where necessary. I guess the core team figured "not working by default" was worse than "insecure by default." Homakov obviously disagreed with this design decision. I can understand why, and I mostly feel the same way. So Homakov posted an issue to the Rails repo Github (https://github.com/rails/rails/issues/5228 https://github.com/rails/rails/issues/5228) suggesting the default be changed. He made a good case and was initially polite. A few days passed, and nobody else had posted to his thread. So, presumably to draw attention to this issue, he exploited the fact that Github had failed to use mass assignment protection. Specifically, he posted a comment with a far-future timestamp, which obviously should be impossible. (I think that's what he did, although Github seems to to have fixed the timestamp now.) He then said this should be proof enough that the Rails defaults need to be changed. The problem with Homakov's argument, as pointed out in subsequent comments in the thread, is that Homakov's hack only demonstrated a mistake on Github's part, not a bug in Rails. It didn't prove anything about Rails that we didn't already know. The only thing surprising he demonstrated was that Github had left open a rather serious vulnerability. TL;DR: Rails has some less-than-secure defaults which all Rails developers are expected to understand and deal with. Homakov found out that Github failed to do so in at least one instance, and he wanted to use that as proof the Rails defaults should be changed.

joeyh 15y ago
Web frameworks like Yesod have shown that it's possible to make a web framework that is secure by design (at the type level, even) without compromising on usability.
danso 15y ago
I agree with you that this is not a Rails bug. But I disagree that Homakov's hack "only demonstrated a mistake on Github's part." It rebutted the Rails team opinion (and again, not all of them disagreed with Homakov) that this was a trivial, edge-case problem. IIRC, one of the changes in Rails 3 was that interpolation in ERb templates were html sanitized by default: http://stackoverflow.com/questions/4731992/rails-3-how-to-reverse-erb-default-behaviour-so-i-can-inject-html http://stackoverflow.com/questions/4731992/rails-3-how-to-re... The fact that web devs write templates vulnerable to XSS is not Rails fault, but apparently the problem was prevalent enough that HTML sanitizing was turned on by default. Apparently, there wasn't empirical evidence to show that update_attributes had the same rate of mistakes to justify a change in defaults...Homakov's hack was a powerful rebuttal.

jazzychad 15y ago

Is it really frowned upon to do: user.name = params[:user][‘name’] ?? Call me old fashioned, but this is called 'defensive coding' and should (in my opinion) be the norm when dealing with client-generated input. It might be more verbose and not 'The Rails Way', but update_attributes seems like too much magic for my paranoid taste.

danso 15y ago
It's not frowned upon, but it would require tighter coupling between the controller and the view...?
- jazzychad 15y ago
  Yes, that makes sense. If your view's form should only update the user's name, then the logic that handles that form submission should only update the user's name. Allowing the client to inject other attributes (even allowed/whitelisted ones) could potentially break other app logic.

lattepiu 15y ago

The real culprit here is HashWithIndifferentAccess, a crutch that throws away a difference that Ruby has for a reason. The sensible way to do updates, in my opinion, is User.update(:name => params['user']['name']) But there's no way in Rails to keep that syntax while disabling User.update(params['user'])

bluesnowmonkey 15y ago
clutch -> crutch

Shanewho 15y ago

Is the solution given in the article any different than using this? config.active_record.whitelist_attributes = true Also, this isn't the first time someone's been bit by this: http://www.kalzumeus.com/2010/09/22/security-lessons-learned-from-the-diaspora-launch/ http://www.kalzumeus.com/2010/09/22/security-lessons-learned...

petenixey 15y ago
I didn't know about the solution you mention at the time I wrote the post. I'm reading up on it now and I think it's actually going to end up being the official solution so it's a good flag: https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac53c57a302b162fd736 https://github.com/rails/rails/commit/06a3a8a458e70c1b6531ac...

jimworm 15y ago

DHH does it in half a line: https://gist.github.com/1975644 https://gist.github.com/1975644 `attr_accessible` should only be used to protect the attributes that are NEVER modified by users. Access to the rest of the attributes may differ by user role, and should be handled by the controller. Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security.

marshray 15y ago
`attr_accessible` should only be used to protect the attributes that are NEVER modified by users But web app developer may not be able to know in advance what new columns might be added on the database, possibly by some other team. If I understand this right, in the absence of attr_accessible, any new columns are completely writable by the HTTP request. So having a default-deny whitelist approach is the only sane strategy. Trying to use `attr_accessible` to protect everything leads to enough frustration to make one eventually give up on security. Or give up on Rails. Usually the basic security of the database is not negotiable.

pdufour 15y ago

I don't find this a good solution. A much better solution is to generate a list of form inputs that are present on a page, then when the form is submitted, check if any of the form inputs were changed / any were added.

obtu 15y ago
Agreed, whitelisting attributes regardless of context is much too coarse. Here is a form library that implements what you describe: https://news.ycombinator.com/item?id=3666907 https://news.ycombinator.com/item?id=3666907

InclinedPlane 15y ago

I find it hilarious that just in the last few days they've removed register_globals from PHP for good. It seems every language/framework ends up retracing the same set of security/convenience tradeoffs over and over again.