How github was hacked
homakov.blogspot.com
homakov.blogspot.com
I just spent a few hours last week hacking through the Stripe CTF game. Environment variables, string formatting injections, and a timing/side channel attack to top it off.
This is just POSTing a value to an endpoint. And it gets written?! To the database?! That's awesome and scary at the same time.
The thing is that this really belongs to a class of vulnerabilities where authentication information is inadequately tied together on the server. This allows any user with valid credentials to fabricate credentials for any other user. In SL it was worse because all you needed was the timestamp and not, say, a valid password, but the same applies.
One thing I will say is that this sort of vulnerability IME suggests inadequate thinking relative to security (and probably other things) on the part of the application designer and therefore raises questions in my mind as to what else may be lurking there.
I really love Github and have been trying to get it adopted in my organization. After the recent events though I'm having second thoughts. I don't think any application is 100% fool proof. But a well known vulnerability; one that is always brought up in any audit, going unnoticed for so long? I honestly did not expect this from Github.
This is not really a "framework bug" as such. It's just crap application architecture.
At the core of this issue is a simple misunderstanding on what the "model" part of MVC actually is. The model represents ONLY the request or "form model", not the data model. There should be a mapping of 1:1 between the request and the "form data model" always with no exception. The controller is responsible for translating that into something useful or doing something with it involving the domain/data model.
Unfortunately where the usual CRUD approach is required, ignorance reigns supreme and the shortest path, not the most correct path tends to appear.
I write this as someone who works on a rather large ASP.Net MVC application (100+ controllers) and has seen this many times already.
Looks like they have two companies for audit/pentest (nGenuity and Matasano Security), plus a consultant. Somehow I doubt tptacek is going to comment on any of this.
I'm not sure I even realized we were on Github's site. If you're asking why we're there, you're right: I obviously can't discuss it here.
It is almost reassuring that the. If was so easy to exploit, because it means that a nonchalant hacker could bring it to mass attention...I'd hate to think of what havoc a dedicated miscreant could've sown in a week. I recently taught a group of neophytes how to use the web inspector, and used the example of altering a form merely to demonstrate the malleability of a webpage. I told them "this is just a parlor trick, don't think that it'll actually work..."
I'd say that's very likely the case. Unfortunate, but, in some ways, understandable. If a person is not able to articulate an issue effectively dismissing them becomes too easy. We all do it.
- David Foster Wallace, Authority and American Usage
"As an American and native English-speaker myself, I have previously been reluctant to suggest this, lest it be taken as a sort of cultural imperialism. But several native speakers of other languages have urged me to point out that English is the working language of the hacker culture and the Internet, and that you will need to know it to function in the hacker community.
"Back around 1991 I learned that many hackers who have English as a second language use it in technical discussions even when they share a birth tongue; it was reported to me at the time that English has a richer technical vocabulary than any other language and is therefore simply a better tool for the job. For similar reasons, translations of technical books written in English are often unsatisfactory (when they get done at all).
"Linus Torvalds, a Finn, comments his code in English (it apparently never occurred to him to do otherwise). His fluency in English has been an important factor in his ability to recruit a worldwide community of developers for Linux. It's an example worth following.
"Being a native English-speaker does not guarantee that you have language skills good enough to function as a hacker. If your writing is semi-literate, ungrammatical, and riddled with misspellings, many hackers (including myself) will tend to ignore you. While sloppy writing does not invariably mean sloppy thinking, we've generally found the correlation to be strong--and we have no use for sloppy thinkers. If you can't yet write competently, learn to."
--Eric S. Raymond, How To Become A Hacker http://www.catb.org/~esr/faqs/hacker-howto.html
Not that being a "Rails Rockstar" necessarily implies being a Hacker--what I like about your quote is that it shows the phenomenon is everywhere. One nice thing for programmers though is that with online text communications you can hide a thick accent that would otherwise be held against you even if what you speak is grammatically perfect.
Dismissing this guy because of his poor English is plain and simply xenophobic prejudice. In this case it came back to the Rails/Github guys and bit them in the ass.
(there's also the accent/dialect/creyole/pidgin distinction, which often revolves around who has an army)
If SWE is taught well, it helps people from poor neighborhoods get taken more seriously. If it wasn't taught, only people from richer neighborhoods would know it, and would be used by rich people to exclude upstarts.
That said, using SWE to discriminate is a bad thing. It's just a little less unfair than discriminating based on the fluency of a dialect that poor kids never had a chance to learn.
A 19 year old Russian male who wasn't taught English properly will need A LOT of effort to learn proper SWE. For some people, it will simply not be feasible. Ability to learn human languages - as opposed to computer languages, much easier to an introvert/math person - is a very distinct ability and you'd be surprised how little correlation it has to intelligence and other skills.
Help reducing discrimination by teaching SWE is akin to help fight discrimination against other races by cosmetics and plastic surgery. Sure, it works on an individual level, but it doesn't help the people who truly have a problem: the ones discriminating.
Does anyone have links to great videos of educated, intelligent discussion in typically looked down-upon accents (e.g., AAVE, deep south, etc)? For instance, I read once that the Queens accent was stigmatized until Feynman became so popular. When I hear videos of him, I notice his distinct speech, but it sounds "smart" because Feynman speaks it!
What is going on here? I think the fear people have is that this attack is laughably simple, but at the same time, was not noticed for a long time. It says something about complexity, all the interlocking parts may have simple problems that are hidden by the abstract models used. It reminds us that the law of unintended consequences are always in effect, and we need to really think out what we are doing, even if the tests all pass. It shows us that even good, well respected software is vulnerable. And it makes us wonder what we have done to open security holes and what is lurking in our code. No one wants to be responsible for such a thing, and it is not pleasant to think about potential consequences of them.
Nothing new under the sun.
Granted, this as default will break an app that does not have the correct attributes declared as mass-assignable, but the alternative is a vulnerable app.
https://github.com/lest/rails/commit/f2fa4837a8a888ee86997be...
We have a form with the fields firstname, surname and date_of_birth. cool. user submits the form. Rails takes the post data and puts in in a data structure called params
Now, at the backend we need to update our database with this new info. Rails allows us to write
user.update(params)
user.save
I alter the form and add another field, say is_admin and set the value to true. Now, if the database has a corresponding field, that also gets updated with the value I've posted. uh oh.
Rails does have the ability to say
attr_protected :is_admin
This is "thin controllers" gone too far -- the model shouldn't have to figure out where it's being updated from and what to allow.
It's really an authorisation issue as to who/what can update which parts of a model - generally this is handled in the controller.
1) The model could be used everywhere so it may be best to negate the issue by locking down the attributes at one source.
2) As you pointed out, the model is in different contexts depending on the privilege of the current user session. It's almost as if I want a thin layer between my model and controller that takes into account session info and informs the model about what can and can't be done.
What I don't know is if RoR allows for this sort of modeling. I have no experience with the framework. It might want something that is similar to getters/setters in Java. If this is the case, such a modeling is problem not going to work since the multiple params will break the spec.
attr_accessible :user_id, :on => :admin
@something.update_attributes(params[:something], :as => :admin)
Sadly, people seem to be running around like headless chickens trying to scotch tape trash bags over a broken window instead of learning how to replace the glass.
If the developer choose to ignore it, is the framework responsible of his action? I don't think so. Like other commenters have said, this is a beginner's mistake and they happen all the time. I don't understand how Rails can be blamed for this. They have done their duty by documenting the issue and it's easy to find (tell me who develops in Rails and is unaware of these guides?)
By the way, this feature is also known in Spring MVC (http://www.springsource.com/security/spring-mvc) and affect frameworks based on it too (ex: Grails). They state this is a "usage issue" and not a bug in the framework.
def update
find_pk.update_attributes(params[:public_key].slice(...))
endThis API sucks rocks.
Assuming this guy is right; the pub key class was allowing any old user to modify the owner_id of the pub key object and change who it belongs to. The pub key class wasn't configured to protect against mass owner_is assignment.
This bug could occur in any framework where someone assumed all attributes submitted are writable by the current user. Rails has no internal concept of users or roles, so building that by default into a model makes no sense.
This is a github bug, not a Rails issue. One could argue it's a questionable, but defensible, decision in the Rails framework, to have such an easy way to take every submitted field and apply it to a model. I'd argue that using such a feature in a production app is a fault of the developer for failing to read their own code, because it's rather obvious and clear what the code does:
@product.update_attributes(params[:product])
Does exactly what you'd expect it to do.
Every other answer, often admittedly, is written by someone who doesn't know anything about Rails, but jumps on the "oh geez Rails has a terrible security hole" bandwagon.
What has happened to this place?
> Does exactly what you'd expect it to do.
Given the existence of attr_accessible, attr_protected, a global on/off switch for the default behavior and nested attributes, you cannot tell what that line does without knowing the contents of at least two files.
And Rails 3.1 does have the concept of roles (not necessarily of users), baked into the same mass assignment logic into the model via the :on argument.
I'm not saying that this is all bad or a bug even, but I don't see how this is trivial to the reader either.
I just learned of the new role feature for attr_accessible because of this controversy. This seems to solve one of the major issues with attr_accessible - that different controllers and users need to update different attributes, so any somewhat complex app would end up widening it's attr_accessible attributes beyond what they should be.
These are still blunt tools though - what if only superadmin users can update a role column to superadmin, but admins can update it to admin or guest. This requires more extensive logic in the controller than simple attribute filtering, demonstrating why this filtering really belongs outside the model. Despite that, I think the new "role" based attr_accessible probably covers most cases and seems quite useful.
For all we know, GitHub may have been using attr_accessible but have expanded it to include columns updatable by admins.
Thank you for the thoughtful comment - perhaps there is hope that HN hasn't been entirely taken over by people talking out of their asses.
This isn't to say it should be this way, just that it's pretty standard behavior.
On top of that, public facing code should be written like
def update
@pk = current_user.public_keys.find(params[:id])
# do the update if you find the key
end