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

51–60 of 91 posts

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

#51

Post by jarrett »

danso wrote: post #11

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 t…

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 every word of every Rails guide. But I have read the security guide in its entirety, and I think every developer owes it to herself and her clients to do the same.

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

#52

Post by [deleted] »

danso wrote: post #18
carlio wrote: post #16

Earlier quoted context omitted.

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…

[deleted]

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

#53

Post by jazzychad »

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.

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

#54

Post by lattepiu »

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

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

#55

Post by quadhome »

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…

I don't think "find" is what you want there either.

The params passed in is auto-deserialized and thus can be a list or a hash with query options instead of your expected "id" value.

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

#56

Post by danso »

jarrett wrote: post #51
danso wrote: post #11

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 t…

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 think that every competent dev knows that something like update_attributes is inherently dangerous. So they probably keep it for backend import/maintenance tasks. At some point, someone copies the code/wrapper-function into the front-end.

The status quo of Rails is that everything is sanitized if you use the helpers. And validators on the models only look at data integrity. So all this protection, at least for me, kind of lulls you into feeling secure, because SQL inject is generally the typical, awful-case scenario.

update_attributes is not really a SQL inject attack vector (since the actual values are sanitized)...it's partially a social engineering scheme.

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

#57

Post by jarrett »

danso wrote: post #56
jarrett wrote: 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 think that every competent dev knows that something like update_attributes is inherently dangerous. So they probably keep it for backend import/maintenance tasks. At some point, someone copies the code/wrapper-function into the front-end. The status quo of Rails is that everything is sanitized if you use the helpers. And validators on the models only look at data integrity. So all this protection, at least for me,…

Actually, I do use update_attributes for public-facing interfaces. But only with attr_accessible. (I never use attr_protected, since blacklists are a disaster waiting to happen.)

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

#59

Post by georgemcbay »

jarrett wrote: post #51
danso wrote: post #11

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 t…

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 about the issue is clearly not enough. If the GitHub team screwed this up, what hope do the majority of the unwashed masses of Rails developers have, warning or no warning?

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

#60

Post by jimworm »

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.

Post reply on HN