The following code and comment appears in includes/db/Database.php:
protected function prepare( $sql, $func = 'DatabaseBase::prepare' ) { /* MySQL doesn't support prepared statements (yet), so just * pack up the query for reference. We'll manually replace * the bits later. */ return array( 'query' => $sql, 'func' => $func ); }
However, the MySQL 5.7 documentation indicates that prepared statements are supported:
http://dev.mysql.com/doc/refman/5.7/en/sql-syntax-prepared-statements.html
Is the comment in Database.php outdated, and if so, could MediaWiki be made more secure against SQL injection by supporting prepared statements with recent versions of MySQL? Or does the support already exist, in spite of the comment?
Best wishes,
Tom
Wenlin Institute, Inc. SPC (a Social Purpose Corporation) 文林研究所社会目的公司 Software for Learning Chinese E-mail: wenlin@wenlin.com Web: http://www.wenlin.com Telephone: 1-877-4-WENLIN (1-877-493-6546) ☯
On Sun, Oct 30, 2016 at 7:37 PM, Tom Bishop, Wenlin Institute tangmu@wenlin.com wrote:
The following code and comment appears in includes/db/Database.php:
protected function prepare( $sql, $func = 'DatabaseBase::prepare' ) { /* MySQL doesn't support prepared statements (yet), so just * pack up the query for reference. We'll manually replace * the bits later. */ return array( 'query' => $sql, 'func' => $func ); }However, the MySQL 5.7 documentation indicates that prepared statements are supported:
http://dev.mysql.com/doc/refman/5.7/en/sql-syntax-prepared-statements.htmlIs the comment in Database.php outdated, and if so, could MediaWiki be made more secure against SQL injection by supporting prepared statements with recent versions of MySQL? Or does the support already exist, in spite of the comment?
Best wishes,
Tom
A large part of Database.php was written back in the MySQL 3 era, which predates many of MySQL's more modern features.
The client-side implementation of PREPARE was added back in Oct 18, 2004 ( https://www.mediawiki.org/wiki/Special:Code/MediaWiki/5962 ). Since then, its never really been used, nor has that code really been touched. Most of our code uses the array based Database::select() wrapper, which has mostly proven effective against sql injections. (There have been instances of SQL injection in MW in the past, although its not very common relative to other types of security vulnerabilities like XSS).
Whether or not we should use prepared queries in MW in general, I don't know. Personally I like our current mechanisms and think they are just fine, but perhaps I just like what I'm used to. In any case, if we were to change to using prepared statements as our current best practice, it would require a either a discussion on wikitech-l or a MediaWiki RFC.
-- Brian
On Oct 31, 2016, at 5:06 AM, Brian Wolff bawolff@gmail.com wrote:
On Sun, Oct 30, 2016 at 7:37 PM, Tom Bishop, Wenlin Institute tangmu@wenlin.com wrote:
The following code and comment appears in includes/db/Database.php:
protected function prepare( $sql, $func = 'DatabaseBase::prepare' ) { /* MySQL doesn't support prepared statements (yet), so just * pack up the query for reference. We'll manually replace * the bits later. */ return array( 'query' => $sql, 'func' => $func ); }
However, the MySQL 5.7 documentation indicates that prepared statements are supported:
http://dev.mysql.com/doc/refman/5.7/en/sql-syntax-prepared-statements.htmlIs the comment in Database.php outdated, and if so, could MediaWiki be made more secure against SQL injection by supporting prepared statements with recent versions of MySQL? Or does the support already exist, in spite of the comment?
Best wishes,
Tom
A large part of Database.php was written back in the MySQL 3 era, which predates many of MySQL's more modern features.
The client-side implementation of PREPARE was added back in Oct 18, 2004 ( https://www.mediawiki.org/wiki/Special:Code/MediaWiki/5962 ). Since then, its never really been used, nor has that code really been touched. Most of our code uses the array based Database::select() wrapper, which has mostly proven effective against sql injections. (There have been instances of SQL injection in MW in the past, although its not very common relative to other types of security vulnerabilities like XSS).
Whether or not we should use prepared queries in MW in general, I don't know. Personally I like our current mechanisms and think they are just fine, but perhaps I just like what I'm used to. In any case, if we were to change to using prepared statements as our current best practice, it would require a either a discussion on wikitech-l or a MediaWiki RFC.
Thank you very much for that information!
My impression so far with Database::select() is that it's hard to understand exactly what goes where, when converting an ordinary SQL query to a set of select() arguments. It's encouraging, though, to know that you've found it to work fine. I'll keep working on it. This page --
https://www.mediawiki.org/wiki/Manual:Database_access
-- includes a dead link to --
https://doc.wikimedia.org/mediawiki-core/master/php/classDatabaseBase.html
Any idea where it should link to?
This comment in the talk page is a little scary: "If you feed an array to select, it's safe. If you construct a string, even a string to be passed to ::select, then it's up to you to take care of safety." Based on that, and on the "mostly" in your "has mostly proven effective", it still seems like prepared statements would be more secure.
Here's a recent article that strongly recommends prepared statements as more secure than input verification:
http://arstechnica.com/information-technology/2016/10/how-security-flaws-wor...
It says, "Prepared statements are a robust and virtually universal solution to the problem of SQL injection. Every API worth using supports them, and yet SQL injection flaws remain in abundance."
Best wishes,
Tom
Wenlin Institute, Inc. SPC (a Social Purpose Corporation) 文林研究所社会目的公司 Software for Learning Chinese E-mail: wenlin@wenlin.com Web: http://www.wenlin.com Telephone: 1-877-4-WENLIN (1-877-493-6546) ☯
On Tue, Nov 1, 2016 at 1:58 PM, Tom Bishop, Wenlin Institute tangmu@wenlin.com wrote:
Thank you very much for that information!
My impression so far with Database::select() is that it's hard to understand exactly what goes where, when converting an ordinary SQL query to a set of select() arguments. It's encouraging, though, to know that you've found it to work fine. I'll keep working on it. This page --
https://www.mediawiki.org/wiki/Manual:Database_access
-- includes a dead link to --
https://doc.wikimedia.org/mediawiki-core/master/php/classDatabaseBase.html
Any idea where it should link to?
I think I fixed the links. The DatabaseBase class was renamed to Database a while ago but the docs weren't all updated.
This comment in the talk page is a little scary: "If you feed an array to select, it's safe. If you construct a string, even a string to be passed to ::select, then it's up to you to take care of safety." Based on that, and on the "mostly" in your "has mostly proven effective", it still seems like prepared statements would be more secure.
Here's a recent article that strongly recommends prepared statements as more secure than input verification:
http://arstechnica.com/information-technology/2016/10/how-security-flaws-wor...
It says, "Prepared statements are a robust and virtually universal solution to the problem of SQL injection. Every API worth using supports them, and yet SQL injection flaws remain in abundance."
Using prepared statements is certainly harder to mess up. Any framework or application that is starting from scratch today should be using prepared statements to execute SQL queries. MediaWiki's database abstraction layer has been around so long that using true prepared statements wasn't possible with MySQL at the time it was written. Instead the MediaWiki abstraction tries to use proper quoting and escaping controlled from the application space. This is delegated to the underlying database functions whenever possible, but does rely on the developer being vigilant about properly using Database::addQuotes() and other escaping functions when making certain calls.
It would be interesting to see an RfC on converting to a prepared statement based execution model written by someone who has gone through our code code and looked at common usage in core and major extensions. Off the top of my head I have no idea how difficult it would be to maintain api compatibility and/or to change the callers that would need to think about constructing a query differently.
Bryan
On Nov 1, 2016, at 4:27 PM, Bryan Davis bd808@wikimedia.org wrote:
On Tue, Nov 1, 2016 at 1:58 PM, Tom Bishop, Wenlin Institute tangmu@wenlin.com wrote:
... https://www.mediawiki.org/wiki/Manual:Database_access
-- includes a dead link to --
https://doc.wikimedia.org/mediawiki-core/master/php/classDatabaseBase.html
Any idea where it should link to?
I think I fixed the links. The DatabaseBase class was renamed to Database a while ago but the docs weren't all updated.
Thank you for fixing that!
...
It would be interesting to see an RfC on converting to a prepared statement based execution model written by someone who has gone through our code code and looked at common usage in core and major extensions. Off the top of my head I have no idea how difficult it would be to maintain api compatibility and/or to change the callers that would need to think about constructing a query differently.
I hope it might be possible to support prepared statements, for use in new code, without requiring all existing code to take advantage of that support right away. I'll keep studying Database.php and MySQL, and if I ever understand well enough to contribute anything useful, I will.
Thanks and best wishes,
Tom
Wenlin Institute, Inc. SPC (a Social Purpose Corporation) 文林研究所社会目的公司 Software for Learning Chinese E-mail: wenlin@wenlin.com Web: http://www.wenlin.com Telephone: 1-877-4-WENLIN (1-877-493-6546) ☯
mediawiki-l@lists.wikimedia.org