Add Phan plugin to check for SQL injections

This adds a phan plugin which checks for SQL injections on code using our QueryBuilder, while it isn't perfect it should already catch most potential issues.

As always, static analysis will sometimes have false positives and this is also here the case. So in some cases the analyzer just doesn't know if something is potential user input or not, thus I had to add some `@suppress SqlInjectionChecker` in front of those potential injections.

The Phan plugin hasn't the most awesome code but it works and I also added a file with test cases.

Signed-off-by: Lukas Reschke <lukas@statuscode.ch>

.drone.yml

@@ -50,6 +50,7 @@ pipeline:
       - composer install
       - composer require --dev "etsy/phan:dev-master"
       - ./lib/composer/etsy/phan/phan -k build/.phan/config.php
+      - php ./build/.phan/plugin-checker.php
     when:
       matrix:
         TESTS: phan

apps/dav/lib/CalDAV/CalDavBackend.php

@@ -635,6 +635,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 	 * @param string $calendarUri
 	 * @param array $properties
 	 * @return int
+	 * @suppress SqlInjectionChecker
 	 */
 	function createCalendar($principalUri, $calendarUri, array $properties) {
 		$values = [
@@ -695,6 +696,7 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 	 *
 	 * Read the PropPatch documentation for more info and examples.
 	 *
+	 * @param mixed $calendarId
 	 * @param PropPatch $propPatch
 	 * @return void
 	 */
@@ -702,6 +704,9 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 		$supportedProperties = array_keys($this->propertyMap);
 		$supportedProperties[] = '{' . Plugin::NS_CALDAV . '}schedule-calendar-transp';
 
+		/**
+		 * @suppress SqlInjectionChecker
+		 */
 		$propPatch->handle($supportedProperties, function($mutations) use ($calendarId) {
 			$newValues = [];
 			foreach ($mutations as $propertyName => $propertyValue) {
@@ -1618,6 +1623,9 @@ class CalDavBackend extends AbstractBackend implements SyncSupport, Subscription
 		$supportedProperties = array_keys($this->subscriptionPropertyMap);
 		$supportedProperties[] = '{http://calendarserver.org/ns/}source';
 
+		/**
+		 * @suppress SqlInjectionChecker
+		 */
 		$propPatch->handle($supportedProperties, function($mutations) use ($subscriptionId) {
 
 			$newValues = [];

apps/dav/lib/CardDAV/CardDavBackend.php

@@ -348,6 +348,9 @@ class CardDavBackend implements BackendInterface, SyncSupport {
 			'{' . Plugin::NS_CARDDAV . '}addressbook-description',
 		];
 
+		/**
+		 * @suppress SqlInjectionChecker
+		 */
 		$propPatch->handle($supportedProperties, function($mutations) use ($addressBookId) {
 
 			$updates = [];

apps/files_external/lib/Service/DBConfigService.php

@@ -116,6 +116,7 @@ class DBConfigService {
 	 * Get admin defined mounts
 	 *
 	 * @return array
+	 * @suppress SqlInjectionChecker
 	 */
 	public function getAdminMounts() {
 		$builder = $this->connection->getQueryBuilder();
@@ -160,6 +161,7 @@ class DBConfigService {
 	 * @param int $type any of the self::APPLICABLE_TYPE_ constants
 	 * @param string|null $value user_id, group_id or null for global mounts
 	 * @return array
+	 * @suppress SqlInjectionChecker
 	 */
 	public function getAdminMountsFor($type, $value) {
 		$builder = $this->connection->getQueryBuilder();
@@ -175,6 +177,7 @@ class DBConfigService {
 	 * @param int $type any of the self::APPLICABLE_TYPE_ constants
 	 * @param string[] $values user_ids or group_ids
 	 * @return array
+	 * @suppress SqlInjectionChecker
 	 */
 	public function getAdminMountsForMultiple($type, array $values) {
 		$builder = $this->connection->getQueryBuilder();
@@ -198,6 +201,7 @@ class DBConfigService {
 	 * @param int $type any of the self::APPLICABLE_TYPE_ constants
 	 * @param string|null $value user_id, group_id or null for global mounts
 	 * @return array
+	 * @suppress SqlInjectionChecker
 	 */
 	public function getUserMountsFor($type, $value) {
 		$builder = $this->connection->getQueryBuilder();

build/.phan/config.php

@@ -151,4 +151,9 @@ return [
 	'whitelist_issue_types' => [
 		// 'PhanAccessMethodPrivate',
 	],
+
+	// A list of plugin files to execute
+	'plugins' => [
+		'build/.phan/plugins/SqlInjectionCheckerPlugin.php',
+	],
 ];

build/.phan/plugin-checker.php

@@ -0,0 +1,44 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+$expected = <<<EOT
+build/.phan/tests/SqlInjectionCheckerTest.php:23 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:35 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:37 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:39 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:41 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:43 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:54 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:61 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:62 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:69 SqlInjectionChecker Potential SQL injection detected
+build/.phan/tests/SqlInjectionCheckerTest.php:70 SqlInjectionChecker Potential SQL injection detected
+
+EOT;
+
+$result = shell_exec('php '. __DIR__ . '/../../lib/composer/etsy/phan/phan -k build/.phan/config.php --include-analysis-file-list build/.phan/tests/* --directory build/.phan/tests/');
+
+if($result !== $expected) {
+	echo("Output of phan doesn't match expectation\n");
+	echo("Expected: $expected\n");
+	echo("Result: $result\n");
+	exit(1);
+}

build/.phan/plugins/SqlInjectionCheckerPlugin.php

@@ -0,0 +1,134 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+declare(strict_types=1);
+
+use Phan\PluginV2;
+use Phan\PluginV2\AnalyzeNodeCapability;
+use Phan\PluginV2\PluginAwareAnalysisVisitor;
+
+class SqlInjectionCheckerPlugin extends PluginV2  implements AnalyzeNodeCapability{
+	public static function getAnalyzeNodeVisitorClassName() : string {
+		return SqlInjectionCheckerVisitor::class;
+	}
+}
+
+class SqlInjectionCheckerVisitor extends PluginAwareAnalysisVisitor  {
+
+	private function throwError() {
+		$this->emit(
+			'SqlInjectionChecker',
+			'Potential SQL injection detected',
+			[],
+			\Phan\Issue::SEVERITY_CRITICAL
+		);
+	}
+
+	/**
+	 * Checks whether the query builder functions are using prepared statements
+	 *
+	 * @param \ast\Node $node
+	 */
+	private function checkQueryBuilderParameters(\ast\Node $node) {
+		$dangerousFunctions = [
+			'eq',
+			'neq',
+			'lt',
+			'lte',
+			'gt',
+			'gte',
+			'like',
+			'iLike',
+			'notLike',
+		];
+
+		$safeFunctions = [
+			'createNamedParameter',
+			'createPositionalParameter',
+			'createParameter',
+		];
+
+		$functionsToSearch = [
+			'set',
+			'setValue',
+		];
+
+		$expandedNode = \Phan\Language\UnionType::fromNode($this->context, $this->code_base, $node);
+		$expandedNodeType = (string)$expandedNode->asExpandedTypes($this->code_base);
+
+		if($expandedNodeType === '\OCP\DB\QueryBuilder\IQueryBuilder') {
+			/** @var \ast\Node $child */
+			foreach($node->children as $child) {
+				if(isset($child->kind) && $child->kind === 128) {
+					if(isset($child->children)) {
+						/** @var \ast\Node $subChild */
+						foreach ($child->children as $subChild) {
+							// For set actions
+							if(isset($node->children['method']) && in_array($node->children['method'], $functionsToSearch, true) && !is_string($subChild)) {
+								if(!isset($subChild->children['method']) || !in_array($subChild->children['method'], $safeFunctions, true)) {
+									$this->throwError();
+								}
+							}
+
+							if(isset($subChild->children['method'])) {
+								// For all "eq" etc. actions
+								$method = $subChild->children['method'];
+								if(!in_array($method, $dangerousFunctions, true)) {
+									return;
+								}
+
+								/** @var \ast\Node $functionNode */
+								$functionNode = $subChild->children['args'];
+
+								/** @var \ast\Node $secondParameterNode */
+								$secondParameterNode = $functionNode->children[1];
+								$expandedNode = \Phan\Language\UnionType::fromNode($this->context, $this->code_base, $secondParameterNode);
+
+								// For literals with a plain string or integer inside
+								if(isset($secondParameterNode->children['method']) && $secondParameterNode->children['method'] === 'literal') {
+									/** @var \ast\Node $functionNode */
+									$functionNode = $secondParameterNode->children['args'];
+
+									$expandedNode = \Phan\Language\UnionType::fromNode($this->context, $this->code_base, $functionNode);
+									if(isset($functionNode->children[0]) && (is_string($functionNode->children[0]) || is_int($functionNode->children[0]))) {
+										return;
+									}
+								}
+
+								// If it is an IParameter or a pure string no error is thrown
+								if((string)$expandedNode !== '\OCP\DB\QueryBuilder\IParameter' && !is_string($secondParameterNode)) {
+									$this->throwError();
+								}
+							}
+						}
+					}
+				}
+			}
+		}
+	}
+
+	public function visitMethodCall(\ast\Node $node) {
+		$this->checkQueryBuilderParameters($node);
+	}
+
+}
+
+return new SqlInjectionCheckerPlugin();

build/.phan/tests/SqlInjectionCheckerTest.php

@@ -0,0 +1,72 @@
+<?php
+/**
+ * @copyright Copyright (c) 2017 Lukas Reschke <lukas@statuscode.ch>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+$builder = \OC::$server->getDatabaseConnection()->getQueryBuilder();
+$builder->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $_GET['asdf']));
+
+class SqlInjectionCheckerTest {
+	private $qb;
+
+	public function __construct(\OCP\IDBConnection $dbConnection) {
+		$this->qb = $dbConnection->getQueryBuilder();
+	}
+
+	public function testEqAndNeq() {
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $this->qb->expr()->literal('myString')));
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $this->qb->expr()->literal(0)));
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $this->qb->expr()->literal($_GET['bar'])));
+		$asdf = '123';
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $this->qb->expr()->literal($asdf)));
+		$asdf = 1;
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->neq('asdf', $asdf));
+		$asdf = '123';
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->lt('asdf', $asdf));
+		$this->qb->select('*')->from('ado')->where($this->qb->expr()->eq('s.resourceid', 'a.id'));
+		$this->qb->select('*')->from('ado')->andWhere($this->qb->expr()->gte('asdf', $_GET['asdf']));
+		$this->qb->select('*')
+			->from('ado')
+			->where($this->qb->expr()->eq('asdf', $this->qb->createNamedParameter('asdf')));
+		$this->qb->select('*')
+			->from('ado')
+			->where($this->qb->expr()->eq('asdf', $this->qb->createPositionalParameter('asdf')));
+	}
+
+	public function testInstantiatingDatabaseConnection() {
+		$qb = \OC::$server->getDatabaseConnection();
+		$qb->getQueryBuilder()->select('*')->from('ado')->where($this->qb->expr()->eq('asdf', $_GET['asdf']));
+	}
+
+	public function testSet() {
+		$this->qb->update('file_locks')->set('lock', $this->qb->createNamedParameter('lukaslukaslukas'));
+		$this->qb->update('file_locks')->set('lock', '1234');
+		$asdf = '1234';
+		$this->qb->update('file_locks')->set('lock', $asdf);
+		$this->qb->update('file_locks')->set('lock', $_GET['asdf']);
+	}
+
+	public function testSetValue() {
+		$this->qb->update('file_locks')->setValue('lock', $this->qb->createNamedParameter('lukaslukaslukas'));
+		$this->qb->update('file_locks')->setValue('lock', '1234');
+		$asdf = '1234';
+		$this->qb->update('file_locks')->setValue('lock', $asdf);
+		$this->qb->update('file_locks')->setValue('lock', $_GET['asdf']);
+	}
+}

core/Command/Db/ConvertType.php

@@ -261,6 +261,14 @@ class ConvertType extends Command implements CompletionAwareInterface {
 		return $db->getSchemaManager()->listTableNames();
 	}
 
+	/**
+	 * @param Connection $fromDB
+	 * @param Connection $toDB
+	 * @param $table
+	 * @param InputInterface $input
+	 * @param OutputInterface $output
+	 * @suppress SqlInjectionChecker
+	 */
 	protected function copyTable(Connection $fromDB, Connection $toDB, $table, InputInterface $input, OutputInterface $output) {
 		$chunkSize = $input->getOption('chunk-size');
 

lib/private/BackgroundJob/JobList.php

@@ -284,6 +284,7 @@ class JobList implements IJobList {
 	 * Remove the reservation for a job
 	 *
 	 * @param IJob $job
+	 * @suppress SqlInjectionChecker
 	 */
 	public function unlockJob(IJob $job) {
 		$query = $this->connection->getQueryBuilder();

lib/private/Comments/Manager.php

@@ -691,6 +691,7 @@ class Manager implements ICommentsManager {
 	 * @param \DateTime $dateTime
 	 * @param IUser $user
 	 * @since 9.0.0
+	 * @suppress SqlInjectionChecker
 	 */
 	public function setReadMark($objectType, $objectId, \DateTime $dateTime, IUser $user) {
 		$this->checkRoleParameters('Object', $objectType, $objectId);

lib/private/DB/Connection.php

@@ -272,6 +272,7 @@ class Connection extends \Doctrine\DBAL\Connection implements IDBConnection {
 	 * @return int number of new rows
 	 * @throws \Doctrine\DBAL\DBALException
 	 * @throws PreConditionNotMetException
+	 * @suppress SqlInjectionChecker
 	 */
 	public function setValues($table, array $keys, array $values, array $updatePreconditionValues = []) {
 		try {

lib/private/Files/Cache/Cache.php

@@ -502,6 +502,7 @@ class Cache implements ICache {
 	 * @param string $targetPath
 	 * @throws \OC\DatabaseException
 	 * @throws \Exception if the given storages have an invalid id
+	 * @suppress SqlInjectionChecker
 	 */
 	public function moveFromCache(ICache $sourceCache, $sourcePath, $targetPath) {
 		if ($sourceCache instanceof Cache) {

lib/private/Files/Cache/Propagator.php

@@ -58,6 +58,7 @@ class Propagator implements IPropagator {
 	 * @param string $internalPath
 	 * @param int $time
 	 * @param int $sizeDifference number of bytes the file has grown
+	 * @suppress SqlInjectionChecker
 	 */
 	public function propagateChange($internalPath, $time, $sizeDifference = 0) {
 		$storageId = (int)$this->storage->getStorageCache()->getNumericId();
@@ -140,6 +141,7 @@ class Propagator implements IPropagator {
 
 	/**
 	 * Commit the active propagation batch
+	 * @suppress SqlInjectionChecker
 	 */
 	public function commitBatch() {
 		if (!$this->inBatch) {

lib/private/Files/Config/UserMountCache.php

@@ -334,6 +334,11 @@ class UserMountCache implements IUserMountCache {
 		$query->execute();
 	}
 
+	/**
+	 * @param array $users
+	 * @return array
+	 * @suppress SqlInjectionChecker
+	 */
 	public function getUsedSpaceForUsers(array $users) {
 		$builder = $this->connection->getQueryBuilder();
 

lib/private/Lock/DBLockingProvider.php

@@ -255,6 +255,7 @@ class DBLockingProvider extends AbstractLockingProvider {
 
 	/**
 	 * release all lock acquired by this instance which were marked using the mark* methods
+	 * @suppress SqlInjectionChecker
 	 */
 	public function releaseAll() {
 		parent::releaseAll();

lib/private/Repair/CleanTags.php

@@ -167,6 +167,7 @@ class CleanTags implements IRepairStep {
 	 * @param string $sourceId
 	 * @param string $sourceNullColumn	If this column is null in the source table,
 	 * 								the entry is deleted in the $deleteTable
+	 * @suppress SqlInjectionChecker
 	 */
 	protected function deleteOrphanEntries(IOutput $output, $repairInfo, $deleteTable, $deleteId, $sourceTable, $sourceId, $sourceNullColumn) {
 		$qb = $this->connection->getQueryBuilder();

lib/private/Repair/NC13/RepairInvalidPaths.php

@@ -51,6 +51,10 @@ class RepairInvalidPaths implements IRepairStep {
 		return 'Repair invalid paths in file cache';
 	}
 
+	/**
+	 * @return \Generator
+	 * @suppress SqlInjectionChecker
+	 */
 	private function getInvalidEntries() {
 		$builder = $this->connection->getQueryBuilder();
 
@@ -95,6 +99,11 @@ class RepairInvalidPaths implements IRepairStep {
 		return $this->getIdQuery->execute()->fetchColumn();
 	}
 
+	/**
+	 * @param string $fileid
+	 * @param string $newPath
+	 * @suppress SqlInjectionChecker
+	 */
 	private function update($fileid, $newPath) {
 		if (!$this->updateQuery) {
 			$builder = $this->connection->getQueryBuilder();

lib/private/Repair/OldGroupMembershipShares.php

@@ -65,6 +65,7 @@ class OldGroupMembershipShares implements IRepairStep {
 	 * Must throw exception on error.
 	 *
 	 * @throws \Exception in case of failure
+	 * @suppress SqlInjectionChecker
 	 */
 	public function run(IOutput $output) {
 		$deletedEntries = 0;

lib/private/Repair/RepairInvalidShares.php

@@ -56,6 +56,7 @@ class RepairInvalidShares implements IRepairStep {
 
 	/**
 	 * Adjust file share permissions
+	 * @suppress SqlInjectionChecker
 	 */
 	private function adjustFileSharePermissions(IOutput $out) {
 		$mask = \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_UPDATE | \OCP\Constants::PERMISSION_SHARE;

lib/private/Security/Bruteforce/Throttler.php

@@ -89,6 +89,7 @@ class Throttler {
 	 * @param string $action
 	 * @param string $ip
 	 * @param array $metadata Optional metadata logged to the database
+	 * @suppress SqlInjectionChecker
 	 */
 	public function registerAttempt($action,
 									$ip,

lib/private/Settings/Mapper.php

@@ -198,6 +198,7 @@ class Mapper {
 	 * @param string $idCol
 	 * @param string $id
 	 * @param array $values
+	 * @suppress SqlInjectionChecker
 	 */
 	public function update($table, $idCol, $id, $values) {
 		$query = $this->dbc->getQueryBuilder();

lib/private/Setup/PostgreSQL.php

@@ -34,6 +34,11 @@ use OCP\IDBConnection;
 class PostgreSQL extends AbstractDatabase {
 	public $dbprettyname = 'PostgreSQL';
 
+	/**
+	 * @param string $username
+	 * @throws \OC\DatabaseSetupException
+	 * @suppress SqlInjectionChecker
+	 */
 	public function setupDatabase($username) {
 		try {
 			$connection = $this->connect([