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

81–90 of 91 posts

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

#81

Post by kragen »

ovi256 wrote: post #49
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…

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.

Once you have a good checklist, you can turn it into a library so that the code can check the things on the checklist for you, or change the code so that the things you had on the checklist to avoid are impossible, and the things you had on the checklist happen automatically. Then you can stop checking the things on that checklist and start on a different checklist.

What you're describing is people doing work that can be automated. That can happen in software, but only in incompetent development teams, or under very special circumstances.

Surgery is different from programming because it's manual labor, and it still exists as a human activity because we don't yet have a feasible way to automate it.

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

#82

Post by dean »

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

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.

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

#83

Post by davedx »

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

It's not social engineering. It's a purely technical issue.

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

#84

Post by shubber »

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…

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

Given the amount of logging that occurs if you do set whitelist_attributes, it's not like this is a huge problem to fix. And, that logging (and the fact that your app mysteriously doesn't work) serve as a loud signal as to what action to take. On the other hand, the "insecure by default" solution is a silent and potentially catastrophic failure.

Compare to how brake pads squeal: even the least mechanically savvy driver brings their car to a mechanic when their pads are running thin.

Finally, the suggested fix (which, frankly, wouldn't have helped github) was simply to update the default generator to set whitelist_attributes, rather than merely including a comment to the effect. The "everything is broken" list would be introductory guides, full stop. So, novice developers would be held up until the guides could be updated with good security practice. Experienced devs, who supposedly all know about this, wouldn't have any problem on new apps.

And the core team have basically said "meh, too much trouble." Apparently, they haven't been chasing html_safe! calls through their views, which is frankly way more of a pain than attr_accessble'ing data fields.

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

#85

Post by javascriptlol »

jarrett wrote: post #78
pbreit wrote: post #73

Earlier quoted context omitted.

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…

No. A property of a system that leads to compromises is a vulnerability. Let's not let give in to neo-essentialism. Just because the word "vulnerability" has been assigned some narrow meaning in the past is no reason to enforce that usage. This is a problem, and "vulnerability" is plainly as the sun shines the right word for it.

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

#86

Post by javascriptlol »

SpoonMeiser wrote: post #30

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…

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.

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

#87

Post by javascriptlol »

danso wrote: post #33
phamilton wrote: post #26

Earlier quoted context omitted.

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…

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.

That is, assuming it hasn't been tried before. We don't really know.

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

#88

Post by einhverfr »

pbreit wrote: post #73
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 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.

One of the lessons I learned early on regarding security was to program as if I don't trust my code to be secure. I always ensure that something else is enforcing security to the extent possible. In some cases I take the "run with the least possible privileges" to an extreme, and grant my application no privileges to crucial resources absent user-supplied credentials.

The advantage of this approach is that even otherwise ordinary security wholes become hard to exploit in useful ways. For example, the set of interesting attacks you can pull off from SQL injection when the SQL permissions are tied to your application login are quite a bit less than they are ordinarily and while you can still do nasty things, the attacks tend to require greater internal knowledge of the database, and the scope of vulnerability is narrowed. Get rid of string interpolation in your queries to the extent possible and another issue goes away.

Be paranoid about security and that will serve you well.....

So when I read that a framework is insecure by default, I naturally suppose that I have good reason to stay away from it.

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

#89

Post by einhverfr »

javascriptlol wrote: post #85
jarrett wrote: post #78

Earlier quoted context omitted.

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…

No. A property of a system that leads to compromises is a vulnerability. Let's not let give in to neo-essentialism. Just because the word "vulnerability" has been assigned some narrow meaning in the past is no reason to enforce that usage. This is a problem, and "vulnerability" is plainly as the sun shines the right word for it.

Put another way, security problems are as much architectural problems as programming mistakes. If the architecture is secure, the programming mistakes will be less severe. Start with a good architecture and the rewards, security-wise, will be substantial.

This is the problem. We think of a security problem as "the developer made a mistake." Often it's the software architect who made the mistake, and if we insist that frameworks weed out the bad architects we are all better off.

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

#90

Post by einhverfr »

SpoonMeiser wrote: post #30

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…

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

Post reply on HN