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 <i>before</i> 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.
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.
Is there any plausible reason why mysqljs should accept anything other than a string for the parameters array (second argument)?<p><pre><code> connection.query("SELECT * FROM accounts WHERE username = ? AND password = ?", [{username: {username: 1}}, "secret"]);
</code></pre>
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.
one of the authors of the library is quite funny: <a href="https://github.com/mysqljs/mysql/issues/274#issuecomment-12896712" rel="nofollow">https://github.com/mysqljs/mysql/issues/274#issuecomment-128...</a> really stupid that such a big library still does not have the most important feature of a sql library.
This is basically the exact same bug as log4shell: trying to be too "smart" by parsing unsafe user input <i>values</i>, as opposed to keeping that kind of logic only on the programmer provided template strings.
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.
The exploit relies on the behavior of backtick quotes in mySql, which I'd never encountered before. Do other SQLish databases have similar features?
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.<p>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.<p>This behavior is barely mentioned in the docs, and the author does not really accept any suggestions or criticism.
This appears to be an exact copy of an issue described 12 days ago:<p><a href="https://flattsecurity.medium.com/finding-an-unseen-sql-injection-by-bypassing-escape-functions-in-mysqljs-mysql-90b27f6542b4" rel="nofollow">https://flattsecurity.medium.com/finding-an-unseen-sql-injec...</a><p>The opening segment is the exact same piece of vulnerable code.
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.
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.
The root mistake is taking in user input without any checking.<p>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.
Looking at this code, this bug would could have been prevented if a statically typed language was used.<p>In addition to, actually using DB-level pre-compiled statements.
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.
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?<p>This just seems like bad code on bad code.
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 (<a href="https://github.com/mysqljs/mysql/issues/731" rel="nofollow">https://github.com/mysqljs/mysql/issues/731</a>).<p>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:<p><a href="https://github.com/mysqljs/sqlstring/issues/60" rel="nofollow">https://github.com/mysqljs/sqlstring/issues/60</a><p>Doug agreed to accept such a request, so I just sat down to figure out the code and a reasonable upgrade plan.<p>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):<p>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.<p>This is the plan as I envision it:<p>* Release SqlString 3.0.0 that has a new allowObjectValues parameter defaulting to <i>false</i>. This is a new major, so it shouldn't break anybody's code.<p>* 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 <i>true</i> 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.<p>* Release mysqljs 3.0 that changes the default and removes the deprecation warning, so new projects get a sane default.<p>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):<p><a href="https://github.com/SonOfLilit/sqlstring" rel="nofollow">https://github.com/SonOfLilit/sqlstring</a>
<a href="https://github.com/SonOfLilit/mysql" rel="nofollow">https://github.com/SonOfLilit/mysql</a><p>(I didn't write the mysqljs3.0 patch yet to make pull request technicalities simpler, but it's trivial)<p>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.<p>For a brighter, safer future :-),<p>Aur