Merge pull request #5817 from nextcloud/add-basic-sql-injection-checker

Add Phan plugin to check for SQL injections

.drone.yml

@@ -23,7 +23,7 @@ pipeline:
     image: nextcloudci/php5.6:php5.6-7
     commands:
       - composer install
-      - ./lib/composer/bin/parallel-lint --exclude lib/composer/jakub-onderka/ --exclude 3rdparty/symfony/polyfill-php70/Resources/stubs/ --exclude 3rdparty/patchwork/utf8/src/Patchwork/Utf8/Bootup/ --exclude 3rdparty/paragonie/random_compat/lib/ --exclude lib/composer/composer/autoload_static.php --exclude 3rdparty/composer/autoload_static.php .
+      - ./lib/composer/bin/parallel-lint --exclude build/.phan/ --exclude lib/composer/jakub-onderka/ --exclude 3rdparty/symfony/polyfill-php70/Resources/stubs/ --exclude 3rdparty/patchwork/utf8/src/Patchwork/Utf8/Bootup/ --exclude 3rdparty/paragonie/random_compat/lib/ --exclude lib/composer/composer/autoload_static.php --exclude 3rdparty/composer/autoload_static.php .
     when:
       matrix:
         TESTS: syntax-php5.6
@@ -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([