Is anyone else laughing at how ridiculous this vulnerability is?
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.
Seriously. This is easier than SQL Injection! Props to Egor for finding it and showing how stupid it is. I cracked up at his "is it really interesting?" line. It makes me wonder if this was a well-known vuln to less-than-classy folk who have already done some damage elsewhere on GitHub.
A similar problem (in Perl) lead us to fork LedgerSMB from SQL-Ledger in part because the author of SQL-Ledger had trouble fixing it....
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.
This is one of the most careless mistakes devs make especially those not so experienced with security. Not without a reason it is there in the fourth spot of OWASP top ten: https://www.owasp.org/index.php/Top_10_2010-A4
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.
Not really laughing. I don't really call myself a programmer, but I am always amazed at these kind of simple-but-dangerous mistakes. Accepting a user_id as a lookup value for a DB update? What for! Take the session_id and look up the user_id from the session table! If required, check if the authenticated user has admin level or "change any user account" rights and only then accept a user_id as a POSTed input.
That appears to be standard practice for "model bound" systems where the internal "domain model" is bound directly to the HTTP protocol. You get this issue with ASP.Net MVC as well on most of the trivially implemented projects out there.
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.
Comments
Is anyone else laughing at how ridiculous this vulnerability is?
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.
Seriously. This is easier than SQL Injection! Props to Egor for finding it and showing how stupid it is. I cracked up at his "is it really interesting?" line. It makes me wonder if this was a well-known vuln to less-than-classy folk who have already done some damage elsewhere on GitHub.
A similar problem (in Perl) lead us to fork LedgerSMB from SQL-Ledger in part because the author of SQL-Ledger had trouble fixing it....
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.
It's a common problem in Rails apps and has been for some years. It's a bit of a beginner mistake, though.
This is one of the most careless mistakes devs make especially those not so experienced with security. Not without a reason it is there in the fourth spot of OWASP top ten: https://www.owasp.org/index.php/Top_10_2010-A4
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.
Not really laughing. I don't really call myself a programmer, but I am always amazed at these kind of simple-but-dangerous mistakes. Accepting a user_id as a lookup value for a DB update? What for! Take the session_id and look up the user_id from the session table! If required, check if the authenticated user has admin level or "change any user account" rights and only then accept a user_id as a POSTed input.
It's a write that shouldn't be authorized but was. It's an access control vulnerability, and they happen all the time.
that were my first toughs too... but sometimes a little hack hasn't to be complex ;)
That appears to be standard practice for "model bound" systems where the internal "domain model" is bound directly to the HTTP protocol. You get this issue with ASP.Net MVC as well on most of the trivially implemented projects out there.
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.