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

Re: Hacked: commit to rails master on GitHub

#51

Post by cdcarter » Sun, Mar 04, 2012, 6:15 PM UTC

Scriptor wrote: post #35

martingordon wrote: post #19

Earlier quoted context omitted.

It's a vulnerability in Rails, which is what GitHub is built on. The vulnerability was demonstrated by adding a commit to the Rails project on GitHub, indicating that GitHub suffers from the vulnerability. Here's the relevant issue. It might clarify things a bit better: https://github.com/rails/rails/issues/5228

The issue seems to be a result of an entire hash of the parameters passed in the request being sent to the new method for models. Is this really common practice in Rails code? Sure you can specify in the model that certain attributes can't be changed. But shouldn't this stuff be checked when validating form input? Normally I'd have a hash of filters, with the field/column name mapped to the appropriate set of rules f…

It's common in tutorials and quickstart guides, usually with a note saying that you really should be protecting things and not using mass-assignment. It just makes the blog in 5 minutes videos cleaner.

Re: Hacked: commit to rails master on GitHub

#52

Post by cdcarter » Sun, Mar 04, 2012, 6:17 PM UTC

bobx11 wrote: post #18

atambo wrote: post #3

Earlier quoted context omitted.

I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228

This guy brought up the vulnerability and the maintainers didn't seem to take it seriously since he wasn't articulate enough or was not approaching them with enough respect maybe for their liking? I wish they would have kissed his ass a little to get the low-down on the vulnerability so I didn't have to worry about my company's private github repos. He deserves props for bring it up for discussion.

This has been discussed since the early days of Rails, and they have chosen to leave their defaults as such and encourage developers to implement model security as needed. Github (seemingly) did not implement model security. This is a vulnerability that is different from application to application, and if the team was following best practices, is not there.

Re: Hacked: commit to rails master on GitHub

#53

Post by mokus » Sun, Mar 04, 2012, 6:20 PM UTC

pyre wrote: post #14

atambo wrote: post #3

Earlier quoted context omitted.

I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228

Relevant: What I want you to see in that thread I mentioned is the way the core team perceives this. You are not discovering anything unknown, we already know this stuff and we like attr protection to work the way it is. Looks like this guy got really frustrated with the Rails devs basically saying that he didn't know what he was talking about. This reminds me of all of the unsafe defaults that PHP used to have. Same…

This justification seems especially odd to me since Rails did so much in the first place to popularize the idea that the default behavior should be the one most likely to be "right". Don't they (or didn't they at one time) have a mantra "convention over configuration"?

I guess that doesn't apply to security.

Re: Hacked: commit to rails master on GitHub

#54

Post by nate » Sun, Mar 04, 2012, 6:21 PM UTC

Shouldn't rails by default protect belongs_to associations. There is probably a minute number of cases where someone wants mass assignment changes to include the parent's id of that record.

Re: Hacked: commit to rails master on GitHub

#55

Post by vidarh » Sun, Mar 04, 2012, 6:24 PM UTC

bryanbibat wrote: post #28

atambo wrote: post #3

Earlier quoted context omitted.

I think it's a mass assignment vulnerability: https://github.com/rails/rails/issues/5228

If it's a simple mass-assignment vulnerability, the Rails team has nothing to do with it given that mass-assignment is a feature and the vulnerability is well documented: http://edgeguides.rubyonrails.org/security.html#mass-assignm... Heck, I even learned this way back when I was learning Rails: http://railscasts.com/episodes/26-hackers-love-mass-assignme...

The fact that this is even a discussion is sufficient for me to consider it a bug. It's irresponsible of the Rails team to leave this default the way it is given that it's long been a known risk.

That they like to consider it a "feature" doesn't make it any better - it just makes them look like idiots

Re: Hacked: commit to rails master on GitHub

#56

Post by bgentry » Sun, Mar 04, 2012, 6:25 PM UTC

holman wrote: post #33

We've patched and fixed this on GitHub.

Do you plan on notifying customers whose repos he accessed? This looks like more than just a minor breach if he was able to give himself admin rights and had read/write access to any repo.

Re: Hacked: commit to rails master on GitHub

#57

Post by patrickaljord » Sun, Mar 04, 2012, 6:25 PM UTC

Funnily, the first Diaspora release had the same issue and the devs were ridiculed and called noobs by a big part of the HN community and security "experts" wrote big posts about it. The different reaction here is interesting to say the least.

Re: Hacked: commit to rails master on GitHub

#58

Post by eddington » Sun, Mar 04, 2012, 6:26 PM UTC

boundlessdreamz wrote: post #47

arturadib wrote: post #37

Earlier quoted context omitted.

Regardless of the 'hacker's motives/personality, I think this is yet another testimony to the power of open source. When you have this many eyeballs looking at your code, the odds of a good-intentioned (however playful/immature) coder to discover a vulnerability is much greater than those of a real ill-intentioned hacker simply due to the sheer number of the former. The issue will quickly get fixed by the community,…

Hmm? The vulnerability is in github's code (not open source) and not in rails.

It actually looks like the vulnerability is on Rails itself, which Github is built on...

Re: Hacked: commit to rails master on GitHub

#59

Post by bretthopper » Sun, Mar 04, 2012, 6:26 PM UTC

Everyone might as well take this opportunity to add attr_accessible to your models.

Models: find app/models -type f -name \*.rb | wc -l

Models with attr_accessible: grep -r -m1 "attr_accessible" app/models | wc -l

If those numbers aren't the same, and the missing model files inherit from ActiveRecord::Base, then look into adding attr_accessible.

Re: Hacked: commit to rails master on GitHub

#60

Post by bradly » Sun, Mar 04, 2012, 6:27 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…

> Merb's approach was to have mass assignment protection in the controller, and I personally think it's self-evident that it belongs there.

But wasn't Merb merged into Rails? :) Sigh...