Rookie Coding Mistake Prior To Gab Hack Came From Site's CTO (arstechnica.com) 164
An anonymous reader quotes a report from Ars Technica: Over the weekend, word emerged that a hacker breached far-right social media website Gab and downloaded 70 gigabytes of data by exploiting a garden-variety security flaw known as an SQL injection. A quick review of Gab's open source code shows that the critical vulnerability -- or at least one very much like it -- was introduced by the company's chief technology officer. The change, which in the parlance of software development is known as a "git commit," was made sometime in February from the account of Fosco Marotto, a former Facebook software engineer who in November became Gab's CTO. On Monday, Gab removed the git commit from its website. Below is an image showing the February software change, as shown from a site that provides saved commit snapshots.
The commit shows a software developer using the name Fosco Marotto introducing precisely the type of rookie mistake that could lead to the kind of breach reported this weekend. Specifically, line 23 strips the code of "reject" and "filter," which are API functions that implement a programming idiom that protects against SQL injection attacks. This idiom allows programmers to compose an SQL query in a safe way that "sanitizes" the inputs that website visitors enter into search boxes and other web fields to ensure that any malicious commands are stripped out before the text is passed to backend servers. In their place, the developer added a call to the Rails function that contains the "find_by_sql" method, which accepts unsanitized inputs directly in a query string. Rails is a widely used website development toolkit.
"Sadly Rails documentation doesn't warn you about this pitfall, but if you know anything at all about using SQL databases in web applications, you'd have heard of SQL injection, and it's not hard to come across warnings that find_by_sql method is not safe," Dmitry Borodaenko, a former production engineer at Facebook who brought the commit to my attention wrote in an email. "It is not 100% confirmed that this is the vulnerability that was used in the Gab data breach, but it definitely could have been, and this code change is reverted in the most recent commit that was present in their GitLab repository before they took it offline." Ironically, Fosco in 2012 warned fellow programmers to use parameterized queries to prevent SQL injection vulnerabilities.
The commit shows a software developer using the name Fosco Marotto introducing precisely the type of rookie mistake that could lead to the kind of breach reported this weekend. Specifically, line 23 strips the code of "reject" and "filter," which are API functions that implement a programming idiom that protects against SQL injection attacks. This idiom allows programmers to compose an SQL query in a safe way that "sanitizes" the inputs that website visitors enter into search boxes and other web fields to ensure that any malicious commands are stripped out before the text is passed to backend servers. In their place, the developer added a call to the Rails function that contains the "find_by_sql" method, which accepts unsanitized inputs directly in a query string. Rails is a widely used website development toolkit.
"Sadly Rails documentation doesn't warn you about this pitfall, but if you know anything at all about using SQL databases in web applications, you'd have heard of SQL injection, and it's not hard to come across warnings that find_by_sql method is not safe," Dmitry Borodaenko, a former production engineer at Facebook who brought the commit to my attention wrote in an email. "It is not 100% confirmed that this is the vulnerability that was used in the Gab data breach, but it definitely could have been, and this code change is reverted in the most recent commit that was present in their GitLab repository before they took it offline." Ironically, Fosco in 2012 warned fellow programmers to use parameterized queries to prevent SQL injection vulnerabilities.
smells fishy (Score:2, Insightful)
Re:smells fishy (Score:5, Funny)
You left off the /s
Re: (Score:2)
i second that...
Re:smells fishy (Score:5, Funny)
You left off the /s
No you are mistaken. The best explanation is that that Gab CEO is actually a crypto-SJW in deep hiding and this is in fact a long con from the left. /s?
Re: (Score:2)
I just assumed that is what Trump was from the get go. A long con to destroy the republican party by sending someone of zero integrity to pander to the worst of the party. I mean, the guy was a New York democrat for a very long time.
Re:smells fishy (Score:5, Insightful)
Former Facebook engineers don't make rookie coding mistakes. More likely the account was compromised.
Yeah, you might like to believe that, but I bet he just screwed up.
Even if his account was compromised, then now you're probably talking about a different rookie mistake.
Re:smells fishy (Score:5, Insightful)
i can attest that even a brilliant, experienced engineer can make a mistake, especially when doing tests by disabling security features and accidentally leaving them disabled.
Re: (Score:2)
i can attest that even a brilliant, experienced engineer can make a mistake, especially when doing tests by disabling security features and accidentally leaving them disabled.
One wouldn't normally check those changes into the repository ...
There are all sorts of rookie mistakes ... (Score:4, Insightful)
i can attest that even a brilliant, experienced engineer can make a mistake, especially when doing tests by disabling security features and accidentally leaving them disabled.
One wouldn't normally check those changes into the repository ...
Unless one makes a rookie mistake :-), like failing to check the diff to make sure debugging code or other unwanted changes are present.
Re: (Score:2)
One wouldn't normally check those changes into the repository ...
Of course one would normally. That's how mistakes are created. A team of people with independent verification of commits wouldn't normally check those into the repository. But if you're ever talking about "one" person making a mistake then all bets are off.
Re: (Score:2)
Re: (Score:2)
Test configurations sometimes make it into production systems. I can attest to that, as well. "You never allow direct injection like that" is much like "you never store passwords in clear text." It's an optimistic goal and is sometimes violated even by skilled, security aware engineers.
All of you made the goofy mistake (Score:5, Informative)
The safe way to do a query, especially in a web-accessible application, is to use a prepared query:
PREPARE fooplan (int, text, bool, text) AS
INSERT INTO foo VALUES($1, $2, $3, $4);
EXECUTE fooplan(1, âPride Rockâ(TM), âtâ(TM), @lionname);
That ensures that the user input is treated as data, not as SQL code.
Running it as code after disallowing a few characters, as the original code did, is nearly impossible to do safely. For example, I don't know that I've EVER seen anyone disallow "-", so you can always inject -- (line comment) and have the rest of the query ignored.
Here are a few dozen ways to exploit applications that try filtering out characters like they did:
https://www.netsparker.com/blo... [netsparker.com]
Which means all the people who replied so far made the same mistake - allowing SQL injection.
By the way, the same applies to script injections. You remove words like "script" from the input? Fine, I'll enter "scriscriptpt" and guess what happens when you remove "script" from the middle of that?
You can't make user input safe by removing stuff.
Re: (Score:2)
Btw, I make dumb mistakes too.
My job is to find security mistakes in code; I've been doing it a long time. That doesn't make me immune from mistakes.
I ask co-workers to review my code because knowing a lot about app dev security just means knowing that there are a lot of ways I could have messed up.
Re: (Score:3)
Is that you Fosco Marotto? (Score:5, Funny)
Next you'll be suggesting that Antifa put in the git commit.
Re:smells fishy (Score:4, Insightful)
Re: (Score:2)
Re:smells fishy (Score:5, Informative)
Your logic doesn't necessarily hold, but there is one aspect which suggests thatg your conclusion is right.
Looking at the code, AFAICSI, a safe query was replaced with an unsafe query. The change would have no other effect. So it would seem to be a deliberate backdoor introduced.
Re: (Score:3)
Perhaps the CTO thought that it might speed up the code? Perhaps the CTO is a moron who just wanted to simplify the codebase (it was a lot fewer lines).
Re: (Score:2)
Re:smells fishy (Score:5, Insightful)
Perhaps the CTO is a moron
He signed up as CTO for a neo-N*zi/white-supremacist/QAnon messaging forum, I think that one's a given.
Re: (Score:2)
Re: (Score:2)
Your logic doesn't necessarily hold, but there is one aspect which suggests thatg your conclusion is right.
Looking at the code, AFAICSI, a safe query was replaced with an unsafe query. The change would have no other effect. So it would seem to be a deliberate backdoor introduced.
He was probably obsessing over performance since he was worried about DDOS or something and was trying to "optimize" the code base.
Or he didn't like that "Status.as_home_timeline" function for some reason and was trying to get rid of it.
Or something else entirely, I've done enough code reviews to have learned there's infinite reasons for programmers to write bad code.
As for it being a deliberate backdoor there's really no evidence for it and lots of evidence against it.
For one, who wrote the backdoor?
If his
Re:smells fishy (Score:5, Informative)
Re: (Score:2)
Not really. I looked up the API; it's just as easy to have find_by_sql to use prepared statements or to do input sanitizing as it is to build a string and hand it to the function.
Re: (Score:3)
Re: (Score:2)
Re:smells fishy (Score:5, Insightful)
Why do you think he is a *former* Facebook engineer?
Snarking aside, anybody can make a rookie mistake, that's why you do code review, even when it's the boss. But building a query string from user input and handing it to a function that calls a SQL (or any) interpreter is not the kind of rookie mistake that even experts might make in a moment of inattention. Any function that executes a string as SQL ought to set off alarm bells.
Re: (Score:2)
Executing a string as SQL is fine, so long as it's static.
You mean final (Score:2)
Re: (Score:2)
As long as it is a *string literal*. In other words:
do_some_sql_thing("string literal")
is fine; but
do_some_sql_thing(stringVar)
should trigger a closer look. Even if stringVar is an immutable variable, it may have been initialized with user input.
Re: (Score:3)
But building a query string from user input and handing it to a function that calls a SQL (or any) interpreter is not the kind of rookie mistake that even experts might make in a moment of inattention. Any function that executes a string as SQL ought to set off alarm bells.
Indeed. There is no explanation except pretty fundamental incompetence for this.
It's the kind of fundamental incompetence that appears to reign supreme with these alt-right places (remember Parler?)
With any group of extremists really. The alt-right certainly qualifies as extremist.
Re: (Score:2)
Former Facebook engineers don't make rookie coding mistakes.
Yeah, they would never write code in PHP.
In any case, I will tell you that this SQL won't scale [archive.vn].
Also, this code is kind of deprecated ruby:
def get(limit = 20, max_id = nil, since_id = nil, min_id = nil)
Re: (Score:2)
Someone with the title CTO certainly knows what a code review is. Or perhaps CTO/lead/only programmer are all one guy.
If he's a former facebook engineer and doesn't know how to write, verify, test and deploy quality code, no wonder he's former.
Re: (Score:2)
YOu dont need a special license to call yourself a CTO.
The garbo that picks up your gabrage is more qualified they actually are certified at using garbage trucks
Re: smells fishy (Score:3)
Given how buggy Facebook has been over the years, why on earth would you make such a claim? Furthermore, how much Rails usage is there at FB compared with say PHP? Even so, it doesn't mean this person used it when they were there.
Or maybe you would just trying to be funny in a sarcastic way?
Ocam's Razor calls B.S. in the fish market (Score:2)
Conspiracy theories are much more fun, but Ocam's Razor says the simpler explanation is usually right. So rather than a successful cyber attack (NOT using SQL injection) followed by a successful intrusion, access to the source code, and an update to the source code to enable an SQL injection attack, Ocam says the CTO screwed up. BTW, the source code repo says the same thing.
Re:smells fishy (Score:4, Insightful)
I was a senior software engineer at Facebook and I'm 0% surprised. Haven't you heard of move fast and break things? The emphasis was always on code velocity, not quality.
Re: (Score:2)
I cannot believe in 2021 that any web facing programming language still makes SQL injection so easy to do.
Just about any programming language that is able to communicate with a database will allow you to construct an arbitrary SQL statement, which means that SQL injection is possible and easy. You can perhaps make rules in your own code ("don't use that API"), but the languages themselves all have that ability.
Re: Seems most likely (Score:2)
It should be really simple, use ORM framework or you're fired.
Re: (Score:2)
Right - this is best addressed as an architectural issue. Some software layer in front of SQL that executes predefined operations. An ORM framework and server-side code that applies it, or creating a service layer that just parameters, validates them, and executes its own SQL.
Re: (Score:3)
An ORM is ok if all you want to do is simple things (select/insert/update/delete). But the moment you actually want to take advantage of what a modern SQL database can offer, you are often better off writing SQL code. The gains in performance can be quite large compared to doing the calculation using data objects (all those object constructors/destructors/data transfers cost time), and it actually often results in simpler code. You do know that SQL is Turing complete these days :)
Re: Seems most likely (Score:2)
Any company which allows their web devs to use raw sql will likely have an exploit which costs them a multiple of the saved development time eventually.
If the ORM lacks a feature for whatever dynamic query the web dev thinks he needs, pay to have it included. SQL is an insane language which survived multiple decades past where it should have been retired, it needs a shim for day to day use in exposed code.
Re: (Score:3)
Nonsense. There is not much really wrong with SQL its a pretty effective tools for expressing just about any select,project,join set operation you can imagine.
No ORMs already make pretty terrible compromises in many cases trying to do way to much. I think ORM is great though if you are working in an OO language. It solves the impedance mismatch well and makes for a lot cleaner code addressing 90% of the CRUD type functions most applications need! If you are not using ORM but are using an OO language - YOU
Re: Seems most likely (Score:2)
If industry had settled on standardized language specific APIs and/or binary RPC ones for databases which could clearly separate code from data without relying on special characters, it would have allowed the economy to spend trillions on something other than cleaning up the messes of programmers not as godly as you.
Re: (Score:2)
Whether or not DarkOx is godly or not, SQL exists as it does today. No standard binary protocol exists and it is extremely unlikely any will.
In some sense, databases are being used for uses for which they were never intended. Certainly the SQL designers never thought about how to interact with ORMs, a concept that didn't exist for another 20-30 years.
The basic problem with an ORM in an object-based language is that its data model is actually quite different than the data model for SQL. You can map many of t
Re: (Score:2)
I am replying to myself. For some ideas on what you can do with SQL, have a look here (I have no affiliation with this site, just something I stumbled upon one day).
https://blog.jooq.org/2016/04/25/10-sql-tricks-that-you-didnt-think-were-possible/
While some of his examples are contrived, look at amount of code needed for #3 (running total calculations) - 7 lines of SQL, or #4 (finding largest series with no gaps) - 17 lines of SQL. Imagine writing this in Java or C# (or whatever your favorite language). How
Re: (Score:2)
Re: (Score:2)
Should Have Kept Executives Away From Code (Score:5, Insightful)
I'm not being facetious - a CTO should have more important tasks and responsibilities than writing production code. If they don't take away the title (and salary), plop them down in a cubicle and have them work for a living. Otherwise you have a part time coder and that's just about the most dangerous thing out there.
Isn't this the most basic rule out there?
Re:Should Have Kept Executives Away From Code (Score:5, Insightful)
Your statement is accurate and makes sense for a well functioning organization. Unfortunately, small startups do not normally fit this bill.
What happens in a startup is you can give people whatever title you want to make up. Titles are very inexpensive, and easy to change. What probably happened is the founders created a CTO title for someone, impressed them with the title, and then used that and equity (also inexpensive) to avoid actually paying a large number of people with a varied collection of roles. I've actually had this offer, multiple times, for startups that want to save on their web development costs. My response is normally "No, I'm not interested in becoming your CTO so you can save on your web development costs. We can put together a contract, and I can delegate these issues to my team members that focus more on this level of challenge daily."
I've seen someone go from "intern" to "CEO" because of this. It was actually not a good thing, since this woman's expectations were totally unrealistic afterward. It would have made more sense to call her a "marketing coordinator" or "evangelist" but that probably would not have motivated her as much as "CEO." She's actually taken this off her LinkedIn, as I'm sure she realized how crazy this is. It's probably not a good idea to have a student as a C-level executive...
People are motivated by lots of different things. Sometimes those things can be good, and sometimes those things can result in problems later.
Re: (Score:2)
I'm not being facetious - a CTO should have more important tasks and responsibilities than writing production code.
And a CEO should have more important tasks than personally tweaking the company's logo - but we've seen that happen, too.
Some people just can't keep from meddling.
Re: Should Have Kept Executives Away From Code (Score:2)
Doesn't this also point to another problem too: lack of code review process? That should be the CTO's responsibility to ensure is happening in the absence of a head of engineering.
Re: (Score:2)
The blind leading the blind, eh? The screenshot of the commit in the develop branch says "no related merge requests found". Of course it's possible to do some sort of code review and/or merge in Gitlab creates a commit like this, but maybe it's more likely they didn't do a code review (normally done via a merge request in Gitlab).
I wonder if the CTO (Score:4, Insightful)
is the only developer in a one-programmer shop, or if he was poking his nose into an IT team's codebase where it doesn't belong.
Re: (Score:2)
oh. Wait, what? (Score:4, Insightful)
So, this was the vulnerability, unless maybe it wasn't the vulnerability, because we don't know.
Also, Rails documentation absolutely does warn you about the ">pitfalls of using find_by_sql [slashdot.org] indiscriminately:
He did it on purpose (Score:2)
Re:He did it on purpose (Score:5, Interesting)
There are literally services that scan all public git commits and report vulnerabilities. In my case it was a one-use cloud service connection string hard-coded and checked-in for a throw away pair coding session (and throw away cloud service). But the point stands - have your always-watching service scan for regex patterns in commits in certain repos and hope you get a bite. Nefariousness ensues.
No surprise at all (Score:3)
The inept always get promoted... then they crack the whip over you wondering why you can't achieve the unachievable in the unrealistic time frame they set for you.
Re: (Score:3)
That's called the 'Peter Principle'.
https://en.wikipedia.org/wiki/... [wikipedia.org]
"employees are promoted based on their success in previous jobs until they reach a level at which they are no longer competent"
I had no idea that the book was originally meant to be satire.
This was no rookie mistake. (Score:2)
Obviously Intentional (Score:3)
accident or consiricy? (Score:5, Insightful)
Going to put the tin foil hat on and activate Conspiracy Theorist Mode for a few moments.
Did the old code have a vulnerability already or did the new code add it?
If the old version didn't have the flaw then one could consider that the commit effectively added a back door with built in plausible deniability.
Either the CTO's credentials were compromised, or the CTO wanted a way into the servers that could be denied, or he is just a crappy programmer.
Probably the latter but the other options are interesting to consider.
Re: (Score:2)
TFS is kind of clueless (Score:3)
Re: (Score:2)
No. Mysql added prepared statements in 2004, Postgres in 2002.
Wonderful (Score:3, Insightful)
Gab is such an unending shitshow, the perfect containment chamber for dumb people.
Labelizer 2000 (Score:2)
Dunning Kruger (Score:4, Insightful)
You have to wonder just how deeply the Dunning Kruger effect [wikipedia.org] goes with these people; Its level of incompetence doesn't just apply to a single field, the lack of meta-cognition applies right across the scope of everything, it is fundamentally a lack of common sense to not constantly consider the limits of one's own knowledge.
Far Right (Score:3)
Fosco Da Rookie (Score:4, Interesting)
Re: Frameworks and parametrization (Score:2)
Re: (Score:2)
Same here - my day job is packet surgery, but I have written more than enough web and sql code throughout the last 25 years. The last time I used non-parameterised SQL was if memory serves me right in 1998.
Re: (Score:2)
What would have helped is a parametric query that would have auto-sanitized the inputs.
Re:Obscurity (Score:5, Insightful)
What would have helped is a parametric query that would have auto-sanitized the inputs.
We need to deprecate non-parameterized APIs and remove them from the libraries.
This will only break code that is already broken.
Re: (Score:3)
We need to deprecate non-parameterized APIs and remove them from the libraries.
This will only break code that is already broken.
Genuine question here: how would you do that? How would the library know the difference between a query that's a static one with no parameters, and one where the string has been constructed from interpolation.
Status.find_by_sql("select now()")
vs
Status.find_by_sql("select ssn where user_id = #{@id}")
or even
Re: (Score:2)
Gah, missed off the last quote:
sqlstring = "select ssn where user_id = #{id}"
Status.find_by_sql(sqlstring)
Re: (Score:2)
This would be great if the ORM's you find in the wild would allow you to do everything SQL can do.
As it stands right now (and the foreseeable future) ORM's generate SQL that is supported by most databases. In practice this means it resorts to the constructs that even older versions of MySQL support. The reason being that because of all reasonably popular databases on the market, MySQL is the most limited one concerning SQL fea
Re: (Score:2)
What?
All the orms I have ever used allows you to send "native sql" directly to the database. Then you just have to tell the orm the type of the result you expect, and the orm will then handle the rest, exactly as if you used the orm to build the query.
Re: (Score:2)
What would have helped is a parametric query that would have auto-sanitized the inputs.
We need to deprecate non-parameterized APIs and remove them from the libraries.
This will only break code that is already broken.
Or code that performs queries too complicated to use the parameterized APIs and does its own sanitization.
A better approach might be to more explicitly warn people, ie, instead of:
sql_library.run_query("select now()");
have
sql_library.yes_i_sanitized_before_run_query("select now()");
Re:Obscurity (Score:5, Informative)
Seems a lot like security by obscurity would've helped them here
SQL injection attacks don't need source code access.
If you have a public-facing form, then check your logs. SQL is getting stuffed into the fields by bots a hundred times per day.
Re: (Score:2)
Re: (Score:3)
All security is obscurity. Unless it is offline.
Usually, we (people who do security stuff) distinguish obfuscation from cryptography. Cryptography puts all the security in a special set of bits we sometimes call a 'key' and so we know to take special care of it because it's called a key. Obfuscation relies on things outside the key. There's nothing wrong with obfuscation as long as you are honest about what you are doing. For example side channel mitigation methods are mostly (but not always) obfuscation methods. A bit of obfuscation on top of cryptograp
Re: (Score:2)
Ironically, this makes for more radicalism on the right, not less.
True, but the brain-drain also makes them less effective.
Kinda like droning all the terrorist leadership
Droning the ISIS leadership put radicals in charge. Then those radicals alienated their own people with their extremism. The drone war against ISIS has mostly been a success.
Re: (Score:2)
I believe this argument should be taken to the limit.
Re: (Score:2)
Re: (Score:2)
ridiculous assertion that tests and interviews that merely amount to memorization
ridiculous assertion that tests and interviews that merely amount to memorization are somehow sufficient
Re: (Score:3)
Slashdot has millions of users. A lot of them who wouldn't be afraid to call themselves nerds.
If you've been told that by only a couple of people it would amount to nothing more than nut-picking -- the practice of using some of the more nonsensical opinions from random individuals and pretend they individuals represent the group they ascribe themselves to as a whole accurately.
A poplar form of weakmanning (similar to strawmanning), because in nut-picking cases there usually is some nut that
Re: (Score:2)
Re: (Score:2)
It also tells me exactly what they told you to memorize.
Because whether memorization makes sense or not depends on the context.
For example it's good if you remember remember at least the 'small' (10x10) multiplication table, so you can do simple multiplications by in a fast fashion.
However memorizing a sine table is not as useful and will take a lot more effort to do so for most people. So for most peop
Re: (Score:2)
A lot of them who wouldn't be afraid to call themselves nerds. ..." I prefer the term geek.
Why the side is called "News for Nerds
Nerds are a bit to nerdy for my.
Re: (Score:2)
You know, not actually athletic people, but still people who think themselves to be superior to nerds and geeks because they are free of liberal indoctrination (also known as obtaining a STEM degree from a university).
Re: (Score:2)
Re: (Score:2)
The problem is that Arstechnica and other non-technical sites submit their own stories and summaries that just get run here.
Re: (Score:2)
Re: (Score:2)
The point is that it is hard to call a site called arstechinica nontechnical. It's right in the name.
Re: (Score:2)
Not all nerds are programmers. Judging from your UID I've probably been in the computer field since around the time you were born by my programming is limited to batch files, SQL queries and a little bit of camera scripting.
Re: (Score:2)