Page MenuHomePhabricator

Database::select with the condition [ 'somecolumn' => false ] will match unexpectedly on (almost) any string value in MySQL non-strict mode
Closed, ResolvedPublic

Description

Commit f536c78 changes the behavior of Database::addQuotes to turn boolean true/false into 1/0 as the old behavior of '1'/'' caused errors with MySQL strict mode (soon to be enabled per T108255).

On the other hand, when not in strict mode, 0 = 'foo' will be true (just like in PHP, MySQL will cast the string to integer); that means any method which looks up a record by some string identifier and does not correctly test the input type will probably just return the first record in the DB when passed false (which can easily happen on a BagOStuff miss, Database::selectField returning no result etc. where the error condition is not checked properly). T147414 was an example.

Event Timeline

Tgr created this task.Oct 6 2016, 1:10 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 6 2016, 1:10 AM
Tgr added a comment.EditedOct 6 2016, 1:17 AM

As far as I can see there is no good solution for this.

pushes the burden on the site admin to figure out what the correct behavior is (although it's auto-guessed when [[https://www.mediawiki.org/wiki/Manual:$wgSQLMode|$wgSQLMode]] is used).

As far as I can see there is no good solution for this

Enforce strict mode ASAP?

Tgr added a comment.Oct 6 2016, 8:35 AM

Enforce strict mode ASAP?

As in, make it a prerequisite for using MediaWiki? As long as we support non-strict on third party sites, the code needs to allow for that possibility.

Anomie added a comment.EditedOct 6 2016, 2:42 PM

when not in strict mode, 0 = 'foo' will be true

It seems that strict mode only applies to table insertions, conversions in WHERE are still unstrict. On the other hand, even in strict mode you can insert the string version of a number (e.g. '0') into an integer column, it's only an error if the string doesn't convert to a number cleanly (e.g. '1x').

MariaDB [my_wiki]> select @@VERSION;
+-------------------+
| @@VERSION         |
+-------------------+
| 10.0.27-MariaDB-2 |
+-------------------+
1 row in set (0.00 sec)

MariaDB [my_wiki]> set sql_mode = 'STRICT_ALL_TABLES';
Query OK, 0 rows affected (0.00 sec)

MariaDB [my_wiki]> create temporary table foo (i int);
Query OK, 0 rows affected (0.03 sec)

MariaDB [my_wiki]> insert into foo values ('0'), ('1');
Query OK, 2 rows affected (0.02 sec)
Records: 2  Duplicates: 0  Warnings: 0

MariaDB [my_wiki]> insert into foo values ('');
ERROR 1366 (22007): Incorrect integer value: '' for column 'i' at row 1
MariaDB [my_wiki]> insert into foo values ('1x');
ERROR 1265 (01000): Data truncated for column 'i' at row 1
MariaDB [my_wiki]> select * from foo where i = 'foo';
+------+
| i    |
+------+
|    0 |
+------+
1 row in set, 1 warning (0.00 sec)

MariaDB [my_wiki]> create temporary table bar (x varchar(10));
Query OK, 0 rows affected (0.02 sec)

MariaDB [my_wiki]> insert into bar values ('foo'), ('bar'), ('1'), ('1baz');
Query OK, 4 rows affected (0.02 sec)
Records: 4  Duplicates: 0  Warnings: 0

MariaDB [my_wiki]> select * from bar where x = 0;
+------+
| x    |
+------+
| foo  |
| bar  |
+------+
2 rows in set, 3 warnings (0.00 sec)

MariaDB [my_wiki]> select * from bar where x = 1;
+------+
| x    |
+------+
| 1    |
| 1baz |
+------+
2 rows in set, 3 warnings (0.00 sec)

So the solution is probably just to convert true/false into '1'/'0' instead of 1/0.

Tgr added a comment.Oct 6 2016, 5:43 PM

Cool, that makes life much easier.

Anomie added a comment.Oct 6 2016, 6:11 PM

Cool, that makes life much easier.

Patch looks good to me.

(T147642 could be related?)

Reedy added a subscriber: Reedy.Oct 10 2016, 9:37 PM

Deployed on .21 using hush hush deploy

Deployed on .21 using hush hush deploy

\o/

Since the patch causing this wasn't in 1.27 or earlier, we should be able to upload it publicly and merge it to master now.

Reedy added a comment.Oct 11 2016, 2:41 PM

Deployed on .21 using hush hush deploy

\o/

Since the patch causing this wasn't in 1.27 or earlier, we should be able to upload it publicly and merge it to master now.

WFM. I don't think we need to wait for the OK from the security team. Should we get it out before the .22 branch cut?

That would avoid having to apply the patch to wmf.22 once it is cut.

Anomie closed this task as Resolved.Oct 11 2016, 4:11 PM
Anomie assigned this task to Tgr.
Anomie changed the visibility from "Custom Policy" to "Public (No Login Required)".
Anomie changed Security from Software security bug to None.
Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:39 PM