26 ms·
Finding an authorization bypass on my own website
- henning 5y agoI 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_m239339 5y agoThe fault lies with library authors trying to be smart while being ignorant of how SQL works, not JavaScript necessarily.
- Aeolun 5y agoWhat 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 5y agoIt'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.
- axiosgunnar 5y ago…in a JavaScript ORM, not the actual database. Still interesting.
- the_gipsy 5y agoThe 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_99 5y agoI 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.
- progforlyfe 5y agoBingo. 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.
- galaxyLogic 5y agoPrepared Statements are great. Is there any way to force the programmer to use (only) them?
- singron 5y agoFrom the DB, no. E.g. you could always have `query("... a = ? and b = "+b, [a])`. From within a language, you somehow need to restrict how you build up the query. E.g. a query builder or ORM usually makes it hard to use anything besides parameters, but they usually have escape hatches (e.g. a where method that takes arbitrary sql strings). One trick you can do in Go is to create a type like `type sqlLiteral string`. The type is private, so other packages can't directly instantiate it, but a string literal will automatically coerce to it. Basically, you can't compute sqlLiteral from arbitrary expressions. It must be a single hardcoded string literal.
- 88913527 5y agoIs 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.
- progforlyfe 5y agoIt 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 5y agoThe exploit relies on the behavior of backtick quotes in mySql, which I'd never encountered before. Do other SQLish databases have similar features?
- andreareina 5y agoOther 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 5y agoAlso, 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 5y agoYes. 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 5y agoMySQL 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 https://www.sqlite.org/lang_keywords.html
- hn_throwaway_99 5y agoThis 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.
- deleted 5y ago[deleted]
- zamalek 5y agoExactly. 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 5y agoThese are not prepared statements, just sloppy crafting of SQL. Same mistakes made in PHP two decades ago are being made here.
- rovr138 5y agoThose who do not learn from history, are doomed to repeat it. (or however that saying goes)
- PeterisP 5y agoThe 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.
- Aeolun 5y 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_99 5y agoWell, 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.
- Aeolun 5y agoThis implies that the alternative to using mysqljs is using prepared statements, which is most definitely not the case. The alternative for people using a library like this is to send plain queries to the db.
- moron4hire 5y agoThat only applies when perfect is not yet achievable. Real parameterized queries exist and can be had today.
- exac 5y agoAnd has since June 2004 in MySQL. https://en.wikipedia.org/wiki/MySQL#Milestones https://en.wikipedia.org/wiki/MySQL#Milestones
- onphonenow 5y agoThis 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.
- progforlyfe 5y agoStill, 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...
- onphonenow 5y agoWhy 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 5y agoAt least it helped me understand that "parameterized queries" are a database feature and not just a library abstraction.
- onphonenow 5y agoRight, benefits include performance gains. From HN: https://blog.soykaf.com/post/postgresql-elixir-troubles/ https://blog.soykaf.com/post/postgresql-elixir-troubles/ In addition to security.
- todotask 5y agoIndeed, this title should be change or suggest remove this article for wrong info.
- merb 5y agoone of the authors of the library is quite funny: https://github.com/mysqljs/mysql/issues/274#issuecomment-12896712 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 5y agoIt’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 5y agoThat’s all we’ll and good when it doesn’t have the equivalent of camouflaged knives sticking out at eye level. Yikes.
- technion 5y agoI agree that this particular issue is extremely obvious and quite poor, but in the particular given link we're responding to their response was more than reasonable.
- couchand 5y agoHmm... commentor raises concern that string manipulation dressed up as prepared statements is potentially a source of SQL injection vulnerabilities. Maintainer dismisses concern as "FUD". Eight years later, article is posted showing an exploitable SQL injection vulnerability. I'm thinking they should have listened rather than getting defensive.
- merb 5y agoWell the first sentence is the problem, not the second.
- joshuahaglund 5y agoIt 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 5y agoI see no parameterized queries on the article. The title is misleading.
- tedunangst 5y agoSoon: mysqljs.connection.real_query()
- appleflaxen 5y agoWhen 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 5y agoAmazing 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 5y agoat 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_m239339 5y 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.
- technion 5y agoThis appears to be an exact copy of an issue described 12 days ago: https://flattsecurity.medium.com/finding-an-unseen-sql-injection-by-bypassing-escape-functions-in-mysqljs-mysql-90b27f6542b4 https://flattsecurity.medium.com/finding-an-unseen-sql-injec... The opening segment is the exact same piece of vulnerable code.
- tedunangst 5y agoIndeed, that's why the opening paragraph links to that article.
- deleted 5y ago[deleted]
- nyanpasu64 5y agoI 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 5y agoThe 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.
- SahAssar 5y agoIs there any reason to not use the pg package? It does promises too, and has been rock solid for me.
- throw_m239339 5y 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 5y agoSo it’s not actually parametrized
- SonOfLilit 5y agoAs 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 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 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/sqlstring https://github.com/SonOfLilit/mysql 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 5y agoLooking 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.