Concerns from Reddit

[WyriHaximus]

The first problem is that the parser for locating placeholders is literally strpos($sql, '?');. If you have a query with a literal ? in it then it will fail - e.g. "select col1, 'wat?' some_literal from table where id=?" then you will get an exception. This might not be a very common occurrence and indeed you may never run into it, but it show a great lack of attention to fundamental functionality.

Worse is the way that the values are then escaped. It's basically a direct reimplementation of mysql_escape_string(). Note I didn't say mysqli_escape_string, nor mysql_real_escape_string(). So this library is subject to all of the usual multibyte encoding pitfalls.

[oddmario]

Doesn't the way the values are escaped make any SQL queries executed by this library subjected to a SQL injection attack?

[WyriHaximus]

Not sure yet, need to dive into this deeper, and also deeper in the MySQL protocol to see how we can improve this. Always thought PDO would send a statement + arguments to MySQL and let MySQL put them together. If that's possible that sounds like the route we should take here as well. Wdyt @clue ?

[therealgaxbo]

Firstly regarding the present state, I THINK that these issues will only be exploitable on systems that use the NO_BACKSLASH_ESCAPES mode, or systems that use one of a few specific multibyte character encodings, gbk being a notable example.

This ticket addresses three different issues:

1. Placeholder substitution ignores context
   If a string literal contains a ? then that will be replaced with a quoted and escaped version of a bound parameter. Although this may be an annoying limitation, I don't currently consider it a security vulnerability because I cannot see any way trigger it without an exception being thrown due to placeholder/param count mismatch (this stance could be subject to change).

2. Server quote settings are ignored
   Quotes are always escaped with a backslash, but if NO_BACKSLASH_ESCAPES is set then this is ineffective. As this is the behaviour required by the ANSI spec and is the behaviour adopted by other databases, it seems very plausible that there are a number of installations that are vulnerable due to enabling this.

3. Escaping is done at the byte level, ignoring multibyte character encoding
   Because escaping is done with strtr, it is possible that the bytes that are matched are actually parts of a multibyte character. Escaping them will at best lead to accidental corruption, and in some cases can be used for SQL injection.

My strongly preferred solution to this would be to use genuine prepared statements, as this would solve all three issues in a totally robust way. The library would not need to track how the server is configured at all. The only downsides I can see are:

• An extra server round trip is required for the separate prepare/execute phase.
• If the user wants to build dynamic SQL queries then they may still need a way to escape identifiers as prepared statement placeholders can only be used for values.
• Although this would solve the simple case of [1] listed above, it would not directly work with your current convenience method of binding an array of values to a single placeholder - nor would it directly work with named parameters (bindParamsFromArray to also accept associative arguments #41)

I think there may be a very simple partial fix that can be done very easily though: change the escape character from a backslash to the ANSI standard of doubling up the single quote. This form is valid for both SQL modes and so addresses [2], and I also believe that it just so happens that there are no character encodings where that particular byte injected into a multibyte character could be used maliciously, thus also solving [3]. This is not a good solution IMO, but may be worth considering as a quick stopgap.

I'm not a MySQL user so I apologise if there are any mistakes or misunderstandings in the above!

PS. Regarding PDO - be aware that by default the mysql driver uses PDO::ATTR_EMULATE_PREPARES meaning they are also doing client-side escaping (albeit with more attention to server configuration). This is imo a bad approach, and you should disable PDO::ATTR_EMULATE_PREPARES when comparing.

[clue]

@WyriHaximus Thanks for bringing this up (can you link to the original discussion?) and thanks to @therealgaxbo for the excellent evaluation 👍

Placeholder substitution ignores context
If a string literal contains a ? then that will be replaced with a quoted and escaped version of a bound parameter. Although this may be an annoying limitation, I don't currently consider it a security vulnerability because I cannot see any way trigger it without an exception being thrown due to placeholder/param count mismatch (this stance could be subject to change).

I agree that this is suboptimal and should be improved in a future version. I also agree that while this may be annoying at times, this usually doesn't cause any major issues because it's easy to work around by not mixing literal values with question marks and placeholder values at once. PRs to address this welcome!

Server quote settings are ignored
Quotes are always escaped with a backslash, but if NO_BACKSLASH_ESCAPES is set then this is ineffective. As this is the behaviour required by the ANSI spec and is the behaviour adopted by other databases, it seems very plausible that there are a number of installations that are vulnerable due to enabling this.

This is an interesting one! I haven't personally encountered this in the real world, but I agree that this may indeed be an issue for such installations. PRs to address this welcome!

Escaping is done at the byte level, ignoring multibyte character encoding
Because escaping is done with strtr, it is possible that the bytes that are matched are actually parts of a multibyte character. Escaping them will at best lead to accidental corruption, and in some cases can be used for SQL injection.

This is correct. This library doesn't currently provide any explicit support to change the charset (#133) and kind of assumes an ASCII-compatible charset (Latin1/ISO-8859-1, UTF-8 and friends). In case of an ASCII-compatible charset encoding (which includes a multi-byte charset encodings such as UTF-8), I don't currently see how this could cause any issues (except for the NO_BACKSLASH_ESCAPES option mentioned above). In case of other multi-byte charset encodings (GBK comes to mind), this may indeed be used to cause havoc. I agree this is clearly something that should be addressed and I'm not sure how widespread this problem is in the real world. As a very reasonable work around, it's a good idea to stick to UTF-8 which shouldn't show this issue. I could even see us limiting this to UTF-8 via #133 for security reasons until we have better support for this in the future. PRs to address this welcome!

My strongly preferred solution to this would be to use genuine prepared statements, as this would solve all three issues in a totally robust way. The library would not need to track how the server is configured at all.

I agree that support for prepared statements would be nice. As you've pointed out, there are some pros and cons to each approach, so I could also see us adding an option similar to PDO's. PRs to address this welcome!

[francislavoie]

(can you link to the original discussion?)

It was from here: https://www.reddit.com/r/PHP/comments/ms4bns/mysqlis_features_you_probably_would_like_to_know/guygere/?context=3