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

61–70 of 91 posts

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

#61

Post by why-el »

cbs wrote: post #47
why-el wrote: post #36

Earlier quoted context omitted.

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.

And we're confident that this guy is the only guy to ever exploit that weakness on github in the last 4 years?

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.

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

#62

Post by rickchristie »

yumraj wrote: post #2

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.

Read his blog: http://homakov.blogspot.com/ - GitHub suspended his account.

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

#63

Post by joeyh »

jarrett wrote: 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…

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.

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

#64

Post by technoweenie »

rickchristie wrote: post #62
yumraj wrote: post #2

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.

Read his blog: http://homakov.blogspot.com/ - GitHub suspended his account.

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.

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

#65

Post by technoweenie »

ajross wrote: post #50
gcampbell wrote: post #34

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.

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.

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

#66

Post by danso »

jarrett wrote: 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 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-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.

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

#68

Post by bluesnowmonkey »

lattepiu wrote: post #54

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'])

clutch -> crutch

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

#69

Post by marshray »

jimworm wrote: post #60

DHH does it in half a line: 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.

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

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

#70

Post by jarrett »

danso wrote: post #66
jarrett wrote: 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 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…

I think you're right that this is a problem, and I would support changing the default, although I can understand why they don't.

You make a good point that showing a hugely popular app with mistake X suggests that mistake X should be prevented at the framework level.

Post reply on HN