Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

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

gist.github.com

Post reply on HN
Print view View original

11–20 of 91 posts

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

#11

Post by danso »

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.

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

#13

Post by petenixey »

LargeWu wrote: post #4

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.

Good flag - I've made the warning even clearer in the Gist

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

#14

Post by thecoffman »

rmc wrote: post #10

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?

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.

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

#15

Post by adriand »

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
The correct way to code this is as follows:

    class PublicKeyController
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.

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

#16

Post by carlio »

rmc wrote: post #10

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?

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.

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

#18

Post by danso »

carlio wrote: post #16
rmc wrote: post #10

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?

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.

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.

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

#19

Post by jgrahamc »

adriand wrote: post #15

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 The correct way to code this is as follows: class PublicKeyController This has two upda…

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?

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

#20

Post by danso »

kristofferR wrote: post #5

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.

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.

Post reply on HN