Finding an authorization bypass on my own website

https://maxwelldulin.com/BlogPost?post=9185867776

by caaqil • 4 years ago

107 118 4 years ago

maxwelldulin.com

axiosgunnar• 4 years ago

…in a JavaScript ORM, not the actual database.

Still interesting.

the_gipsy2• 4 years ago

The root mistake is taking in user input without any checking.

It's not the author's fault, it's JavaScript that leads this way. Even worse is TypeScript, as its compile-time checking adds a false sense of security.

hn_throwaway_997• 4 years ago

I disagree. I would qualify this as a clear bug in mysqljs.

First off, most other DB drivers will use real prepared statements, so what is passed down to the DB is actually the templated query and a set of values. It looks like mysqljs actually parses and interpolates the string before sending it to the DB engine.

Perhaps more importantly, though, this bug is basically the exact type of bug that caused the log4shell fiasco: trying to be too "smart" with user provided values that should just be treated as "dumb" scalars. The fundamental way of filling out template parameters with something that can refer to DB column names is a major flaw IMO.

progforlyfe3• 4 years ago

Bingo. Any time software is made to try parsing & escaping SQL queries before sending to the server is going to find out the hard way why that is risky.

With prepared statements it's like saying to the DB, here's the query: SELECT * FROM users WHERE email = ? AND password = ?. And here's the data: ["whatever", "whatever"]. You can literally send whatever your heart desires for those two strings, unescaped, to the DB server and let them deal with it.

Merad3• 4 years ago

Both are true. When the request input is blatantly the wrong type like this, you shouldn't even be attempting to execute the sql query. But of course JavaScript encourages developers to be lazy and not even think about such things.

the_gipsy2• 4 years ago

Perhaps more importantly, though, this bug is basically the exact type of bug that caused the log4shell fiasco: trying to be too "smart" with user provided values that should just be treated as "dumb" scalars.

Which is exactly the equivalent of using JSON.parse(). You will find more library combinations, and for many of them the options can't be considered bugs like you do for mysqljs here, and you definitely won't be able to catch it.

The solution is parsing beyond JSON.parse(). You can't have arbitrary user provided structures floating around your program.

ovex3• 4 years ago

I'd argue that the root mistake is that the server-side feature of prepared statements through the client/server binary protocol is not used as described in [1]. Instead, the question marks are replaced using a dirty hack. The whole point of prepared statements is that the client/server protocol takes care of inserting the values into the statement instead of having to fiddle with functions like mysql_real_escape_string.

[1] https://dev.mysql.com/doc/refman/8.0/en/sql-prepared-stateme...

evanelias3• 4 years ago

This is actually not unusual though in practice. Large-scale applications quite frequently do client-side (application-side) handling of bind variables -- including many large websites/apps that you use, I can absolutely guarantee this from personal experience.

The reason is that using a real prepared statement in MySQL (as well as most other DBMS) requires an extra round-trip, which adds latency. It also potentially adds complexity if a proxy/middleware layer is in use, since prepared statements are per-connection, at least in MySQL.

The core problem in this specific case is that this js mysql client library simply did not implement client-side interpolation correctly or securely. This is quite bad; I'm not aware of a similar problem in any other major language's most popular mysql driver in recent years.

thinkharderdev3• 4 years ago

The root mistake is that the actual feature makes using real prepared statements impossible. The whole point of a prepared statement is that you know the query AST ahead of time so you can just plug user input into the AST. I'm guessing since this seems to be intentionally designed this way, the goal is to allow a "parameterized query" like "SELECT * FROM foos WHERE ?" and I can pass { "foo": 1, "bar": 2 } to get "SELECT * FROM foos WHERE foo = 1 AND bar = 2"

88913527• 4 years ago

Is there any plausible reason why mysqljs should accept anything other than a string for the parameters array (second argument)?

  connection.query("SELECT * FROM accounts WHERE username = ? AND password = ?", [{username: {username: 1}}, "secret"]);

Without looking at the implementation details, I would expect this code to throw an exception. A "garbage in, garbage out" philosophy seems dangerous for a SQL statement.

progforlyfe3• 4 years ago

It looks like the article headline is slightly misleading. It says "Parameterized queries", but I wouldn't use that phrase to describe escaping data "client side" and sending the SQL query to the Database essentially as raw. Or maybe I am thinking of "Prepared statements", but I tend to use the terms interchangeably.

thyrsus• 4 years ago

The exploit relies on the behavior of backtick quotes in mySql, which I'd never encountered before. Do other SQLish databases have similar features?

andreareina• 4 years ago

Other rdbms have identifier quoting, yes. But the problem isn't that identifier quoting exists, it's that the library interprets an object as a key = value statement (compounded treating the key as an identifier), which it shouldn't.

    ... AND password = password = 1

is just wrong, quoted or not.

yawaramin• 4 years ago

Also, the fact that the library doesn't validate the type of the input data ({password: 1} when the password should be a string...).

This is just a cascading chain of failures.

useerup• 4 years ago

Yes. Almost all DBMSs has a way to delimit table/column names (for instance when a name contains spaces or special characters.

However, proper DBMSs expose parameters as 1st class concepts through the API. That has several advantages, some of which:

1) It is more secure, as you will not have to do this dangerous escaping before invoking. Parameters are substituted by the DBMS.

2) The DBMS can better understand the "dynamic" part of a query and cache query plans.

epitactic• 4 years ago

MySQL uses `backticks` for quoting identifiers, ANSI SQL and PostgreSQL use "double-quotes", SQL Server and MS Access use [square brackets].

SQLite supports all three: https://www.sqlite.org/lang_keywords.html

henning• 4 years ago

I clicked through expecting some fiendishly clever trick involving weird Unicode characters and encodings or something. Nope, just people using a browser scripting language invented in 7 days on the backend by choice.

throw_m2393393• 4 years ago

The fault lies with library authors trying to be smart while being ignorant of how SQL works, not JavaScript necessarily.

Aeolun5• 4 years ago

What is up with all you people ragging Javascript the language? This is 100% programmer error, and has absolutely nothing to do with the language.

Javascript has plenty of bad points without making without making things up.

h3mb3• 4 years ago

It's a meme on HN at this point. Lazy negative remarks about JS or other web technologies has high demand in the reader side too I guess.

hn_throwaway_997• 4 years ago

This is basically the exact same bug as log4shell: trying to be too "smart" by parsing unsafe user input values, as opposed to keeping that kind of logic only on the programmer provided template strings.

zamalek2• 4 years ago

Exactly. Something called 'sql', 'json', 'xml' (DOCTYPE exploit anyone?), or whatever else should do the thing on the label and nothing else. If the developers are so inclined to add extra magic or automation then they can always make a prelude library.

Also, why in the hell are values being quoted (sql parlance for escaped)? Does the mysql protocol not support actual parameterized queries?

datalopers• 4 years ago

These are not prepared statements, just sloppy crafting of SQL. Same mistakes made in PHP two decades ago are being made here.

rovr138• 4 years ago

Those who do not learn from history, are doomed to repeat it. (or however that saying goes)

PeterisP2• 4 years ago

The key point is that an abstraction layer trying to implement an API that looks like parametrized queries is not equivalent to actual parametrized queries where the query and parameters are kept separately, and the SQL text is parsed and execution plan is formed by the DB engine before the parameters are even considered. If your DB library is inserting the parameters in a text query before sending it off to the server to be parsed as arbitrary SQL, that's not a parametrized query, but just fake smoke and mirrors.

Aeolun5• 4 years ago

that's not a parametrized query, but just fake smoke and mirrors.

And still much preferable to not having it at all.

Don’t let perfect be the enemy of good.

hn_throwaway_997• 4 years ago

Well, in this case mysqljs pretty well fucked up the implementation here in that this sort of bug should never happen when using prepared statements, so I'd say it's worse than not having it at all, because it breaks the standard safety guarantees of prepared statements.

moron4hire• 4 years ago

That only applies when perfect is not yet achievable. Real parameterized queries exist and can be had today.

sqliwtf• 4 years ago

That’s the kind of thing you say to ship a product to market, to eschew a non-critical feature.

Parametrized Queries for a SQL library are a critical “do not ship without” feature. You do not lie and tell your user you have a safe product causing them to have a compromised system.

I hope to God you are not creating production systems anywhere.

gruez• 4 years ago

If your DB library is inserting the parameters in a text query before sending it off to the server to be parsed as arbitrary SQL, that's not a parametrized query, but just fake smoke and mirrors.

Do you have examples of libraries that's actually a "parametrized query"? The python libraries I encountered (psycopg2, sqlalchemy) both seem to be "fake smoke and mirrors".

tedunangst4• 4 years ago

sqlite3 con.execute() appears to prepare and execute a statement, with bound parameters.

kayodelycaon• 4 years ago

Active Record from Ruby on Rails does. It’s actually a problem on older versions because it may incorrectly generate duplicates of queries that could use the same prepared statement but don’t. Can cause unexpected memory usage.

PeterisP2• 4 years ago

This is one of the major differences between psycopg2 and psycopg3 - https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html#se...

bgrainger3• 4 years ago

MySqlConnector (C# - https://github.com/mysql-net/MySqlConnector) uses "smoke and mirrors" for the "text" protocol (because MySQL Server has no way to send parameters as part of a `COM_QUERY` packet, https://dev.mysql.com/doc/internals/en/com-query.html) and true parameters for the "binary" protocol, which does (https://dev.mysql.com/doc/internals/en/com-stmt-execute.html).

Every MySQL client needs to do something similar, due to the design of the MySQL protocol.

btschaegg• 4 years ago

If ADO.NET counts as a library for you: Parameterized queries for MS SQL through ADO.NET.

They are seemingly run through a stored procedure on the server side, with each parameter passed in as an argument. This has some consequences that lead to very obscure behavior, too [1].

For example, you should never create temporary tables in an SQL statement that was initialized with parameters, as they won't survive the end o the statement; they will be destroyed as soon as the innermost scope (within the stored procedure call) finishes.

I ran into this because I tried setting up a single "run this query" method in a more complicated database routine in order to keep my C# code clean... didn't work out as I'd hoped ;)

[1] https://stackoverflow.com/a/46311328

Edit: I just realized that this still isn't a guarantee that parameters are handled as a separate structure by the underlying network protocol. I hope they are -- but I don't know how to check. Maybe the according .NET Core code is on GitHub?

adeel_siddiqui• 4 years ago

Hibernate has actual prepared statements and binds values when the query is executed.

tedunangst4• 4 years ago

From the mysqljs readme: "Caution This also differs from prepared statements in that all ? are replaced, even those contained in comments and strings."

Eek. "select * from classes where teacher = '?'" -> boom.

tyingq• 4 years ago

Wordpress also has faked parameterized queries.

https://github.com/WordPress/WordPress/blob/4ae0744585ea9417...

They do seem to have resisted attacks for quite a while though.

jolux• 4 years ago

Reading WordPress internals like this makes me feel inclined not to use it.

SahAssar2• 4 years ago

Hasn't WP had multiple SQL injection CVE's? WP is famously terrible at security.

bgrainger3• 4 years ago

One major problem is that the MySQL "text" protocol doesn't support true parameterised queries. There's no way to pass parameters in the `COM_QUERY` packet: https://dev.mysql.com/doc/internals/en/com-query.html

That's probably why many of the libraries mentioned in this thread use "smoke and mirrors". Of course, it is quite possible to correctly escape a value by rendering it to a string first, _then_ encoding the whole thing.

To have true parameterised queries, you need to use the "binary" protocol, which many MySQL libraries don't offer support for. (MySQL also has some frustrating limitations with the binary protocol, such as not allowing a SQL string containing more than one statement to be prepared.)

namibj5• 4 years ago

PostgreSQL shares that limitation on it's extended query protocol (needed for prepared statements), though you can send multiple queries off before sending the "I'm waiting, hurry up and get back with answers" packet to the server. Not that the native C library (libpq) supports such pipelining...

tomnipotent4• 4 years ago

to actual parametrized queries

Except there is no such thing as "actual parametrized queries", either as defined by the SQL standard or as supported by any of the major RDBMS vendors.

The closest thing we have are prepared statements, which are basically session-scoped stored procedures used by client-side libraries to pull off the illusion of parametrized queries.

I wouldn't be surprised that if you go back far enough, the idea of parametrized queries probably started in client libraries trying solve for SQL string soup with a dash of sprintf-like syntax sugar.

magicalhippo• 4 years ago

Except there is no such thing as "actual parametrized queries", either as defined by the SQL standard or as supported by any of the major RDBMS vendors.

We're in the process of adding support for MSSQL in addition to the DB server we've been using for ages. This is one of the pain points. They differ in how they handle "parameterized queries" enough to be a PITA.

default-kramer• 4 years ago

Except there is no such thing as "actual parametrized queries" [...] as supported by any of the major RDBMS vendors.

That would surprise me... I thought that when I send a parameterized query to PostgreSQL or MS SQL Server, most (or even all) of the query plan gets created without looking at any parameter values. And if that is true, then I think your "no such thing" claim cannot be true. If the query plan has already been created based on the unparameterized SQL string, then parameter values cannot cause it to do something crazy like drop an unrelated table.

(But I haven't read the source code to either of those RDBMS, so maybe I am about to be surprised.)

tus6662• 4 years ago

Of course there is. If the parameters are incorporated server side after the SQL is parsed then you have true parameterized queries. If the parameters are inserted before the SQL is parsed you have trouble.

tus6662• 4 years ago

This is a perfect example of why I often just read the comments before the links on HN. Saves a lot of face-palming.

cromd• 4 years ago

[...] execution plan is formed by the DB engine before the parameters are even considered

Is this really possible if the server considers table statistics (i.e. frequencies of certain values) when forming a plan?

thinkharderdev3• 4 years ago

Typically the SQL statement will be parsed to a logical plan first, which then gets optimized and then turned into a physical execution plan. Statistics are used for a second layer of optimization on the physical execution plan.

mirekrusin• 4 years ago

You can do it right with ie. combinators [0] where you explicitly specify at which position you are in (value, identifier etc) and have much more control over query building (dynamism and flexibility that value-only parametrized query/prepared statement doesn't provide). Tagged templates are fantastic to represent safe query mini-"dsl".

[0] https://github.com/appliedblockchain/tsql/

onphonenow3• 4 years ago

This title is horrible. I clicked through expecting something interesting with let's say PostgreSQL parameterized queries. This has nothing to do with parameterized queries, this is some kind of standard SQL injection issue on an intermediate layer.

progforlyfe3• 4 years ago

Still, I hope the author realizes the problem so they won't repeat such a mistake. The fact that such a package exists is a little odd to begin with...

onphonenow3• 4 years ago

Why not just use the engines parameterized query support if you are supporting the feature?

They are probably missing out on many of the OTHER benefits of prepared statements. Easily 60%+ of queries can be prepared reducing overhead sometimes significantly.

addandsubtract• 4 years ago

At least it helped me understand that "parameterized queries" are a database feature and not just a library abstraction.

onphonenow3• 4 years ago

Right, benefits include performance gains.

From HN:

https://blog.soykaf.com/post/postgresql-elixir-troubles/

In addition to security.

todotask• 4 years ago

Indeed, this title should be change or suggest remove this article for wrong info.

merb2• 4 years ago

one of the authors of the library is quite funny: https://github.com/mysqljs/mysql/issues/274#issuecomment-128... really stupid that such a big library still does not have the most important feature of a sql library.

tkiolp4• 4 years ago

It’s an open source library that the author made for free in its free time. You either accept it as it is (check its License file), improve it, or walk away and use something else.

lazide• 4 years ago

That’s all we’ll and good when it doesn’t have the equivalent of camouflaged knives sticking out at eye level. Yikes.

joshuahaglund• 4 years ago

It seems to me that none of this would happen if they followed best practices, which (if I understand correctly) is to query for the record using the idendifier (username), make sure there is only one result, then use like some standard crypt library to compare the password to the hashed+salted password?

This just seems like bad code on bad code.

henvic• 4 years ago

I see no parameterized queries on the article. The title is misleading.

tedunangst4• 4 years ago

Soon: mysqljs.connection.real_query()

appleflaxen• 4 years ago

When the API makes your query appear parameterized but it's not, then you may be the victim of attacks that work against non-parameterized queries.

pigbearpig• 4 years ago

Amazing how SQL injection is in the OWASP Top 10 for the last 20 years and then you see something like this. I understand it’s open source but I can’t fathom why the author would be ok with not having true parameterized queries.

shoo• 4 years ago

at the day job i see quite a few fresh potential SQL injection vulns when i do code review or when reading through existing code. many of our colleagues in the industry are junior and haven't had to learn this yet or have deep expertise in some areas but dont have deep experience in writing production code, then end up in teams writing production code.

i didn't realise SQL injection was a thing to watch out for until i'd already been working professionally for 5 years -- that maybe sounds bad, but for the first 5 years i didn't work on any projects that integrated with databases.

throw_m2393393• 4 years ago

at the day job i see quite a few fresh potential SQL injection vulns when i do code review or when reading through existing code. many of our colleagues in the industry are junior and haven't had to learn this yet or have deep expertise in some areas but dont have deep experience in writing production code, then end up in teams writing production code.

Don't blame that on juniors. The people writing these libraries with "magic" bullshit query parsing aren't junior. SQL injection is a very basic security issue that can be very mitigated simply with actual prepared statements.

technion2• 4 years ago

This appears to be an exact copy of an issue described 12 days ago:

https://flattsecurity.medium.com/finding-an-unseen-sql-injec...

The opening segment is the exact same piece of vulnerable code.

tedunangst4• 4 years ago

Indeed, that's why the opening paragraph links to that article.

nyanpasu64• 4 years ago

I think this bug wouldn't happen in a statically typed language where the attacker couldn't pass a variable of a different type than expected. Dynamic typing makes it difficult to know every possible state your code can be in.

phiresky• 4 years ago

The popular pg-promise library for PostgreSQL in NodeJS has a similar issue - for some types of queries ("Formatting Filters") it interpolates parameters itself instead of using real parameterized queries.

This is especially bad because it uses it's own escaping function and escaping in PG depends on a server configuration variable (standard_conforming_strings) that the client doesn't know about.

This behavior is barely mentioned in the docs, and the author does not really accept any suggestions or criticism.

SahAssar2• 4 years ago

Is there any reason to not use the pg package? It does promises too, and has been rock solid for me.

throw_m2393393• 4 years ago

This behavior is barely mentioned in the docs, and the author does not really accept any suggestions or criticism.

This is where the programming community has a role to play.

When library authors blatantly brush off security issues, it's time to call out that behavior publicly and promote a secure fork. a database library should never have hidden behaviors such as theses. And "magics" such has manually building strings into a query like that s, or parsing a provided query to transform it into something else under the hood, should be turned off by default.

This is absolute madness.

jbverschoor• 4 years ago

So it’s not actually parametrized

SonOfLilit• 4 years ago

As a security professional, I was horrified to find out that the maintainers don't consider this a security issue, though they did promise to take this seriously and change the API when they were made aware of it in 2014 (https://github.com/mysqljs/mysql/issues/731).

So I bumped an issue, noting this is all over HN, and offered to write a pull request for the API change proposed by the maintainers:

https://github.com/mysqljs/sqlstring/issues/60

Doug agreed to accept such a request, so I just sat down to figure out the code and a reasonable upgrade plan.

Three hours later, I could already write Doug this email (pasting it here because the issue and codebase are locked to non-contributors so I had to send it via email):

OK, I have a draft pull request ready. Of course, it's a big change and I expect to get a lot of feedback and have a few rounds of back and forth and fixups before it is accepted.

This is the plan as I envision it:

* Release SqlString 3.0.0 that has a new allowObjectValues parameter defaulting to false. This is a new major, so it shouldn't break anybody's code.

* Release mysqljs 2.19.0 (or should it be 2.18.3 for even higher adoption?) that depends on SqlString 3.0.0 but explicitly passes allowObjectValues on every call to it,with a default of true unless the user explicitly set it to false. This is a non-breaking change. This version will also add a deprecation warning whenever a ConnectionConfig is built without explicitly setting allowObjectValues, warning that the default will change in 3.0 and suggesting to set it to false unless it's needed and highlighting the need to typecheck values if it's set to true.

* Release mysqljs 3.0 that changes the default and removes the deprecation warning, so new projects get a sane default.

This involves, of course, changes to two repositories, so here they are (I can't open pull requests because I have not contributed in the past):

https://github.com/SonOfLilit/sqlstring https://github.com/SonOfLilit/mysql

(I didn't write the mysqljs3.0 patch yet to make pull request technicalities simpler, but it's trivial)

Again, I'm new to this project, am not a javascript developer in my day job, and I expect - and am prepared to handle - nontrivial amounts of feedback and requests for improvement.

For a brighter, safer future :-),

Aur

exabrial• 4 years ago

Looking at this code, this bug would could have been prevented if a statically typed language was used.

In addition to, actually using DB-level pre-compiled statements.

Finding an authorization bypass on my own website

Related Stories