5 ms·

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 i

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_99 5y 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.
- progforlyfe 5y 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.
  - galaxyLogic 5y ago
    Prepared Statements are great. Is there any way to force the programmer to use (only) them?
    - singron 5y ago
      From 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.
    - nicoburns 5y ago
      No, because constants and column names are of course legitimate parts of SQL queries (even those using prepared statements), and because queries with no parameters are also legitimate.
    - namibj 5y ago
      Code review and static analysis work great, by allowing obviously-safe dynamic SQL where static SQL is insufficient.
- Merad 5y 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.
  - ovex 5y ago
    The problem is not only that types are not checked but that strings are simply replaced in structured input. Without having looked in detail, I would bet that the implementation also fails at queries which contain question marks inside strings, i.e. question marks that are not placeholders. Even if string escaping was the right way, which it is not, the proper way to do it would be to translate the SQL statement into an AST and then replace the leaves of the tree that are placeholders with the respective escaped strings.
  - hn_throwaway_99 5y ago
    Developers shouldn't have to think about these things. If I'm doing any sort of string interpolation, whether it be in a SQL statement like this or in something like a log or print formatter, I should be able to put whatever the f I want into the value, without checking it. That's the whole point of using interpolated statements like this in the first place: it should be safe to run (but may throw something like a type error in the DB) regardless of what is stuffed in the value. This is solely a mysqljs bug.
    - Merad 5y ago
      If you use any strongly typed language with a halfway decent framework you _don't_ have to think about this because the request will automatically be validated and rejected for invalid input before your code is ever hit.
      - nicoburns 5y ago
        That’s also true of a decent JS library. The problem here being that the library wasn’t decent. A library in a strongly typed language would still need to use parameterised queries or escape the strings just like a JS library. And it would be just as easy or difficult to have an incorrect sanitising function. In fact, the specified string is not invalid input, it would be perfectly valid to store backticks in a string field in MySQL. They just need to be correctly escaped before being submitted to the database.
- the_gipsy 5y 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.
ovex 5y 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-statements.html https://dev.mysql.com/doc/refman/8.0/en/sql-prepared-stateme...
- evanelias 5y 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.
  - ovex 5y ago
    Interesting. I would still insist on not doing it that way, especially when writing a library for universal use. First of all, string replacement on structured input is an immediate red flag. Second, even if you get handling the structured input right by parsing the statement into an AST and replacing the leaves that are placeholders with the escaped strings, there are still potential vulnerabilities. For example, mysql_real_escape_string is still vulnerable when being used with some exotic character sets [1]. I don't know the internals of the (binary) protocol used for communication with the MySQL server though. Couldn't one just save the extra round-trip with length-prefixed strings by sending the query together with the parameters in a single message? [1]: http://www.gosecure.it/blog/art/483/sec/mysql_escape_string-the-charset-vulnerability/ http://www.gosecure.it/blog/art/483/sec/mysql_escape_string-...

hn_throwaway_99 5y 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.

progforlyfe 5y 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.
- galaxyLogic 5y ago
  Prepared Statements are great. Is there any way to force the programmer to use (only) them?
  - singron 5y ago
    From 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.
  - nicoburns 5y ago
    No, because constants and column names are of course legitimate parts of SQL queries (even those using prepared statements), and because queries with no parameters are also legitimate.
  - namibj 5y ago
    Code review and static analysis work great, by allowing obviously-safe dynamic SQL where static SQL is insufficient.
Merad 5y 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.
- ovex 5y ago
  The problem is not only that types are not checked but that strings are simply replaced in structured input. Without having looked in detail, I would bet that the implementation also fails at queries which contain question marks inside strings, i.e. question marks that are not placeholders. Even if string escaping was the right way, which it is not, the proper way to do it would be to translate the SQL statement into an AST and then replace the leaves of the tree that are placeholders with the respective escaped strings.
- hn_throwaway_99 5y ago
  Developers shouldn't have to think about these things. If I'm doing any sort of string interpolation, whether it be in a SQL statement like this or in something like a log or print formatter, I should be able to put whatever the f I want into the value, without checking it. That's the whole point of using interpolated statements like this in the first place: it should be safe to run (but may throw something like a type error in the DB) regardless of what is stuffed in the value. This is solely a mysqljs bug.
  - Merad 5y ago
    If you use any strongly typed language with a halfway decent framework you _don't_ have to think about this because the request will automatically be validated and rejected for invalid input before your code is ever hit.
    - nicoburns 5y ago
      That’s also true of a decent JS library. The problem here being that the library wasn’t decent. A library in a strongly typed language would still need to use parameterised queries or escape the strings just like a JS library. And it would be just as easy or difficult to have an incorrect sanitising function. In fact, the specified string is not invalid input, it would be perfectly valid to store backticks in a string field in MySQL. They just need to be correctly escaped before being submitted to the database.
the_gipsy 5y 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.

ovex 5y 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-statements.html https://dev.mysql.com/doc/refman/8.0/en/sql-prepared-stateme...

evanelias 5y 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.
- ovex 5y ago
  Interesting. I would still insist on not doing it that way, especially when writing a library for universal use. First of all, string replacement on structured input is an immediate red flag. Second, even if you get handling the structured input right by parsing the statement into an AST and replacing the leaves that are placeholders with the escaped strings, there are still potential vulnerabilities. For example, mysql_real_escape_string is still vulnerable when being used with some exotic character sets [1]. I don't know the internals of the (binary) protocol used for communication with the MySQL server though. Couldn't one just save the extra round-trip with length-prefixed strings by sending the query together with the parameters in a single message? [1]: http://www.gosecure.it/blog/art/483/sec/mysql_escape_string-the-charset-vulnerability/ http://www.gosecure.it/blog/art/483/sec/mysql_escape_string-...