Page MenuHomePhabricator

SQLinjection in Extension:Cargo CREATE TABLE
Closed, ResolvedPublic

Description

Hi,

In "CargoUtils.php" line 665 there is the line $cdb->tableName( $tableName ) . ' ( ' .

IDatabase::tableName is not suitable for malicious input as it won't quote if it detects the string is already quoted, or if it contains some keywords (like ON). Its probably more appropriate to use $cdb->addQuotedIdentifier( $cdb->tableName( $tableName, 'plain' ) ); (Still not really perfect as it could skip the prefix in certain circumstances, but a whole lot better).

The fact that Cargo does not let you use double underscore in table name, and that you can't inject before the keyword TABLE (so no inserting CREATE OR REPLACE) does limit the sorts of evil you can do with this [or at least limitted me. Smarter people may be able to figure out bypasses]. However one potential malicious thing you can do is exploit CREATE TABLE of death, if the person is using MySQL prior to versions 5.5.58, 5.6.38 and 5.7.20, or MariaDB prior to version 5.5.57, 10.0.32, 10.1.26 and 10.2.7:

I did not actually test this, but in theory

{{#cargo_declare:
_table=éééééééééééééééééééééééééééééééééééééééééééééééééééééééééééééééé	(id	int	PRIMARY	KEY)	ENGINE=InnoDB	PARTITION	BY	RANGE	(id)	SUBPARTITION	BY	HASH	(id)		(PARTITION	çççççççççççççççççççççççççççççççççççççççççççççççççççççççççççç	VALUES	LESS	THAN	(70));/*	ON	*/--
}}

[Note, that's using tabs not spaces].

will brick your db if your using a susceptible version of mysql/mariadb.

Event Timeline

Bawolff created this task.Feb 28 2018, 3:03 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 28 2018, 3:03 AM

@Bawolff - yikes! That's pretty bad. Judging by that blog post, I assume by "brick" you mean just crashing the database, as opposed to harming its contents in some way?

This looks like my fault for having the code just check for the existence of spaces, as opposed to all whitespace characters. Would that change be enough to avoid this problem/

Bawolff added a project: Vuln-Inject.EditedFeb 28 2018, 5:00 AM

@Bawolff - yikes! That's pretty bad. Judging by that blog post, I assume by "brick" you mean just crashing the database, as opposed to harming its contents in some way?

My understanding is that the db is fixable after doing that, but it might require some skill to fix it. Im not sure - i didnt try it myself as i didnt want to screw up my db.

This looks like my fault for having the code just check for the existence of spaces, as opposed to all whitespace characters. Would that change be enough to avoid this problem/

It would certainly increase difficulty of exploiting although there may be other ways to separate tokens (perhaps using empty comments /**/). The best solution is to ensure that the table name is quoted as an identifier (pass the second arg to ->tableName() and then wrap it in ->addIdentifierQuotes(). As long as the table name (and other user input) is properly quoted a malicious user cannot do anything evil.

@Bawolff - thanks for the help; I just checked in what I hope is a fix for this security problem, based on your recommendation.

@Bawolff - thanks for the help; I just checked in what I hope is a fix for this security problem, based on your recommendation.

Yes. https://gerrit.wikimedia.org/r/#/c/415753/ looks like a good fix to me.

Yaron_Koren closed this task as Resolved.Mar 2 2018, 6:17 PM

I'm marking this as "Resolved", then - feel free to re-open.

Removing view restrictions as task is resolved.

Bawolff changed the visibility from "Custom Policy" to "Public (No Login Required)".Mar 2 2018, 10:26 PM
Alexia added a subscriber: Alexia.Mar 12 2018, 2:19 PM

I just got notification of this. Patched on Gamepedia/Hydra wikis. However, our version of MySQL is patched already for the table of death issue.