Live data from Hacker News

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

gist.github.com

71–80 of 91 posts

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

#71

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.

It's not frowned upon, but it would require tighter coupling between the controller and the view...?

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

#72
post #71

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.

It's not frowned upon, but it would require tighter coupling between the controller and the view...?

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.

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

#73
post #48

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. I…

I don't like the gist of your post. You are giving Rails way too much of a pass and assigning too much blame to GitHub. A framework should not be "insecure by design". Period. GitHub engineers are likely about as good as they get and still missed it. That's less an indictment of GitHub engineering and more so poor decision-making on the Rails side.

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

#74

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…

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.

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

#75

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

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/06a3a8a458e70c1b6531ac...

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

#76
post #24

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

Possibly years? They probably wont know either - I can imagine that this could be hard to track if someone is careful enough.

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

#77
post #50

Earlier quoted context omitted.

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.

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.

ref changes are tracked in the reflog, but the "pusher" (as opposed to author and committer) isn't.

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

#78
post #73
post #48

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. I…

I don't like the gist of your post. You are giving Rails way too much of a pass and assigning too much blame to GitHub. A framework should not be "insecure by design". Period. GitHub engineers are likely about as good as they get and still missed it. That's less an indictment of GitHub engineering and more so poor decision-making on the Rails side.

I agree with you that the default should be changed. And note that any application developer can in fact require attr_accessible globally, across all models, with one line of configuration.

The point of my post was not to give Rails a pass, and I apologize for misleading if it came off that way. Rather, I was trying to clarify the situation for those who are less familiar with Rails. The discussions surrounding this issue (including Homakov's own words) seem to erroneously suggest that Homakov discovered a previously unknown vulnerability in Rails. I was merely clarifying that he instead found a vulnerability in a specific Rails app.

Now, it's a matter of opinion as to whether the Rails default should be called a "vulnerability." I say yes, but reasonable people can disagree. What's clear, though, is that no previously unknown vulnerabilities in the framework have been revealed.

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

#80
post #51

Earlier quoted context omitted.

While I mostly agree that the Rails default should probably be changed, I think in fairness it's also worth pointing out that the Rails team does give fair warning about this issue. Specifically, anyone who reads the official Rails Security Guide back-to-back will know about this: http://guides.rubyonrails.org/security.html One might counter that nobody reads these guides back-to-back. I must admit I haven't read eve…

I'm not a big fan of the "this is documented" defense. If Microsoft left a network accessible default passworded Admin account in Windows Server but documented it and told people to change it, would that be okay simply because it was documented? Documentation is no panacea for bad defaults. Part of the OP's point (and he's absolutely right) is that this incident proves beyond a shadow of a doubt that just warning abo…

IIRC, something like that actually happened. Except it was a file server that threw your whole directory tree up on the net. oops! Why you ask? How could that even happen? It was just a few years before everyone had Internet, so it was assumed that your network was a LAN. (Source: http://www.grc.com/su-bondage.htm)

Of course, everyone should have really had a firewall anyway, so this was obviously cool right? After all, it's up to the user to secure their machine.

(Disclosure: That was sarcasm.)

Post reply on HN