Hacked: commit to rails master on GitHub • Hacker News Board

Re: Hacked: commit to rails master on GitHub

#91

Post by jsnell » Sun, Mar 04, 2012, 7:44 PM UTC

BenjaminCoe wrote: post #86

jsnell wrote: post #78

Earlier quoted context omitted.

The "attack" clearly wasn't malicious, though probably immature. A malicious attacker would have been doing things like gaining access to users' private repos and stealing the code, or trying to sneak in harmful commits to a repo under a false name. This was at most a prank or a demonstration. So why this instead of responsible disclosure? The problem is that it was not really a GitHub issue, it's Rails having a gros…

This clearly was malicious: * it provides a how to for other individuals to repeat the attack, in a public forum. * it was made against an innocent third-party. * I doubt steps were taken to contact the third-party. * it was made on a Sunday morning. making it difficult to scramble and get a fix out the door. "Clearly the Rails core team were not willing to consider any kind of changes to improve the situation" The t…

Whether it was malicious depends on the motives, and it's very hard for me to see where the malice is. To me it looks more like he just wanted to bring attention to the issue in order to get it fixed.

I already agreed that GitHub were innocent bystanders and that the timing was unfortunate. But if getting publicity to the issue was the main point, it's also easy to see why GitHub was the perfect target. I also already explained why it could make perfect sense to demonstrate the vulnerability in a public manner rather than just disclose it to one of the many sites suffering from the problem. None of that is a sign of malice, it's at most bad judgement.

The ticket having been opened only three days ago would be a good point if it hadn't also been closed and declared to be working as intended with a pointer to a previously closed bug about the same issue.

Re: Hacked: commit to rails master on GitHub

#92

Post by dylanz » Sun, Mar 04, 2012, 7:47 PM UTC

wycats wrote: post #44

Here's my proposal for improving the situation: https://gist.github.com/1974187 Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. Moving it into the controller will also make it easier to solve the tension between reducing the friction of getting up and running quickly and having good security defaults. In general, Rails' conventi…

I agree completely. The attr_accessible issue has been around for so long (I blogged about it in 2007: http://news.ycombinator.com/item?id=1031168), I'd assume it is by far one of the easiest things to exploit in a production Rails application. Personally, if I wanted to hack a Rails application, this is the first thing I'd try.

Your proposed controller technique is great. This technique coupled with some intelligent defaults seems like a good idea, with respect to both sides of the story (both a secure-by-default and frictionless convention). I'm excited to see this in the limelight, and your proposal is a fantastic start to this conversation. Thank you sir!

Re: Hacked: commit to rails master on GitHub

#93

Post by momoro » Sun, Mar 04, 2012, 7:48 PM UTC

lucaspiller wrote: post #46

holman wrote: post #33

We've patched and fixed this on GitHub.

Good work. Please could you write a post explaining what you did, so others using Rails can do the same?

They probably simply made user_id protected on whatever table controls commit permissions.

Re: Hacked: commit to rails master on GitHub

#94

Post by moe » Sun, Mar 04, 2012, 7:52 PM UTC

wycats wrote: post #44

Here's my proposal for improving the situation: https://gist.github.com/1974187 Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. Moving it into the controller will also make it easier to solve the tension between reducing the friction of getting up and running quickly and having good security defaults. In general, Rails' conventi…

self-evident

It is. The more interesting question is why it takes rails 7 years (and counting) to come to this conclusion.

I'll take it one further and sing my song about the Rails ActiveRecord implementation here, which is tangentially related.

The promise of AR is to reflect on the database at startup and then "magically work".

The problem in rails is that nothing magically works. What you get out of the box is an insecure world-writable model, as illustrated by this bug. Then you begin scattering your truth over incremental (hand-crafted!) migrations, the model, and the controller, until nobody other than a cascade of unit-tests can even make sense of the interdependencies anymore.

In a world where there is not even a central place to look up which fields exist on a model and what their constraints are - short of runtime introspection, where database constraints live happily alongside and independently of model constraints, where opaque bits of ruby-logic buried in various gems add their own magic to the mix, in such a world it's really no surprise they chose to default to "all fields writable".

Because if they forced the user to do the sensible thing and explicitly list the allowable fields then where's the advantage over a declarative approach (like e.g. Django) anymore?

Imho it's long overdue to take a step back and revisit whether AR is still worth having, or ever was. Imho it causes way more problems than it solves, in contrast to the declarative approach.

Re: Hacked: commit to rails master on GitHub

#95

Post by teej » Sun, Mar 04, 2012, 7:53 PM UTC

Posting it as an issue on the Rails repo and then exploiting GitHub with it is a great way to get attention, but not necessarily the most responsible.

I disclosed a vulnerability to GitHub before. I dropped it into their Issues system marked private with the heading "URGENT". It was a Sunday and I got a response + a fix from Tom Preston-Wener himself within a few hours. That, in my mind, would have been a more responsible approach.

Re: Hacked: commit to rails master on GitHub

#96

Post by mcav » Sun, Mar 04, 2012, 7:53 PM UTC

cagenut wrote: post #84

holman wrote: post #33

We've patched and fixed this on GitHub.

Thank you commenter "holman" on this third party discussion site, but as a paying customer could github please post something official somewhere like now

Holman works at GitHub.

Re: Hacked: commit to rails master on GitHub

#97

Post by sausagefeet » Sun, Mar 04, 2012, 7:54 PM UTC

arturadib wrote: post #43

holman wrote: post #33

We've patched and fixed this on GitHub.

Time from exploit to fix in production: I'd buy Github stock if I could.

At the very least you'd probably be getting it on the cheap at that point.

Re: Hacked: commit to rails master on GitHub

#98

Post by kijin » Sun, Mar 04, 2012, 7:55 PM UTC

rsbrown wrote: post #87

nknight wrote: post #62

Earlier quoted context omitted.

The vulnerability is that Rails is insecure by default. That used to be the case for a lot of things, then finally people noticed how the real world works, and started fixing them. Apparently the Rails developers have actively resisted the lesson everyone else already learned.

This was not a case of "Rails is insecure by default". There's very popular idiom in Rails development of updating model data from a form POST/PUT in one line of code in a controller: my_object.update_attributes(params[:object]) Because many users follow this approach, it made this hack widely exploitable. You can either assign parameters piecemeal in the controller or explicitly set the attr_accessible attributes in…

There's a fine line between (1) "insecure by default" and (2) the existence of a very popular idiom that is dangerous unless accompanied by other checks.

Many PHP apps used to rely on register_globals without proper input checking, and when those apps got hacked, it was clearly their their own fault. Just like GitHub is primarily responsible for today's exploit. But that didn't prevent people from calling PHP "insecure by default" for enabling register_globals in the first place.

Re: Hacked: commit to rails master on GitHub

#99

Post by momoro » Sun, Mar 04, 2012, 7:56 PM UTC

BenjaminCoe wrote: post #86

jsnell wrote: post #78

Earlier quoted context omitted.

The "attack" clearly wasn't malicious, though probably immature. A malicious attacker would have been doing things like gaining access to users' private repos and stealing the code, or trying to sneak in harmful commits to a repo under a false name. This was at most a prank or a demonstration. So why this instead of responsible disclosure? The problem is that it was not really a GitHub issue, it's Rails having a gros…

This clearly was malicious: * it provides a how to for other individuals to repeat the attack, in a public forum. * it was made against an innocent third-party. * I doubt steps were taken to contact the third-party. * it was made on a Sunday morning. making it difficult to scramble and get a fix out the door. "Clearly the Rails core team were not willing to consider any kind of changes to improve the situation" The t…

He could probably have made a lot of money from this if he hadn't disclosed it. A lot of people store various secret keys along with their apps on github. Many people even have entire repositories of secret keys. By taking access to these repos (if he could get access to private repots) he could likely have gotten access to apps from which he could have taken money.

Re: Hacked: commit to rails master on GitHub

#100

Post by speleding » Sun, Mar 04, 2012, 7:56 PM UTC

wycats wrote: post #44

Here's my proposal for improving the situation: https://gist.github.com/1974187 Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there. Moving it into the controller will also make it easier to solve the tension between reducing the friction of getting up and running quickly and having good security defaults. In general, Rails' conventi…

What do you think about the solution proposed in by homakov (the "hacker")? Just mark all the *_id attributes "protected" by default. Seems elegant and really easy to implement and fixes 90% of the problem.