[ovex]

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...

[evanelias]

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]

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-...

[bgrainger]

> 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?

No, this wouldn't work, because you have to send COM_STMT_PREPARE (https://dev.mysql.com/doc/internals/en/com-stmt-prepare.html) first, which takes the SQL and returns a "statement ID". Then you can send COM_STMT_EXECUTE (https://dev.mysql.com/doc/internals/en/com-stmt-execute.html) which contains the statement ID and the parameters. Finally, you would ideally send COM_STMT_CLOSE (https://dev.mysql.com/doc/internals/en/com-stmt-close.html) to free the server-side resources for the prepared statement, although this could be "pipelined" with the EXECUTE packet.

[evanelias]

> mysql_real_escape_string is still vulnerable when being used with some exotic character sets

Indeed -- mysql_real_escape_string "mostly" fixes this problem by requiring a db connection as one of its args. Since the driver is usually aware of the connection state, mysql_real_escape_string can check to see if one of those exotic charsets is in-use. The issue is that there are multiple ways to change the connection charset, some of which the driver is aware of (e.g. in PHP mysqli set_charset) but some it is not (running textual statements like SET NAMES or SET CHARACTER SET).

However, generally an attacker won't have the ability to set an arbitrary exotic character set for the connection anyway... unless they already have some other sql injection mechanism, in which case it's a moot point :)

Driver documentation also typically mentions this problem. For example, here's the doc for doing client-side param interpolation in the most popular MySQL driver for Golang: https://github.com/go-sql-driver/mysql#interpolateparams (see warning in italics)

It also explicitly detects if your initial connection settings attempt to use one of those charsets along with param interpolation, and throws an error if so: https://github.com/go-sql-driver/mysql/blob/21f789cd/dsn.go#...

> 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?

AFAIK, no, not with the traditional MySQL binary protocol. The newer "X protocol" introduced in MySQL 5.7 does allow this, but it is not widely implemented in drivers.

[nicoburns]

I actually think it’s fine to do string sanitising here. The string formats for the databases are well specified, and writing a function to that spec shouldn’t be difficult. I believe there’s no need to parse to AST for postgres as you can just wrap the string in quotes, replace any existing quotes in the string with two quotes and you’re done. If other databases have funkier string formats then you may have a point.

at least that’s true for Postgres, I can’t speak for MySQL.

[vlovich123]

I’m kind of shocked that there’s an extra round trip.

One would expect you can do the preparation lazily as there shouldn’t even be a hypothetical performance consideration for using prepared statements.

In other words you send an execute prepared statement request, you can provide the truly prepared statement OR the templated sql string. In the latter case the server would send you the results of preparing the statement before sending you the result of execution so that you can provide it on subsequent requests.

Or use an RPC system that supports promise pipelining so that you could send the prepare request and the execute is sent with the promise of the prepare request.

[evanelias]

I think the core mistake in a lot of these older protocols is conflating the concept of a prepared statement with the concept of a parameterized query.

With a prepared statement, there's an implication that you want to re-use it multiple times, so the DB needs to give you back some sort of handle/identifier on it. But this requires bookkeeping on both the client and server side, and that overhead is often not worthwhile on modern CPUs, especially when running fairly simple CRUD-style queries which are quick to parse. (and at least in modern versions of MySQL, the DB will track a "digest" of the query which removes params anyway, even if you used client-side param interpolation!)

So ideally DB binary protocols should offer less expensive ways of doing parameters for "fire and forget" queries, instead of having to track real prepared statements, but often they don't.

MySQL's newer X Protocol provides a way to avoid the extra round-trip, but it isn't very widely used yet AFAIK, and it still involves prepared statement bookkeeping: https://dev.mysql.com/doc/internals/en/x-protocol-use-cases-...

[vlovich123]

Yes, you should also be able to do a stand-alone parametrized query. The only DB system I’ve seriously programmed against was SQLite where this isn’t an issue.

[namibj]

PostgreSQL Extended Query Protocol won't cause the round trip issue; you can even send multiple parametrized queries back-to-back without any RTT penalties. This requires consideration in the API, though, because errors are delayed until the end of such a sequence, loosing attribution as to which individual query caused the error (at least at a low-level protocol level).

Not that this is a problem for a well-designed library, of course. E.g. tokio-postgres in Rust does it if you use a promise-combinator for the first poll of multiple queries inside the same transaction.

[thinkharderdev]

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"