From 099a5a3c2d99ed39bdd8815e1e2790bb9351770b Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 20 Nov 2017 16:53:44 +1300 Subject: [PATCH 01/10] [SS-2017-008] Fix SQL injection in full text search --- src/ORM/Connect/MySQLDatabase.php | 14 +++++++------- src/ORM/PaginatedList.php | 8 ++++---- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/ORM/Connect/MySQLDatabase.php b/src/ORM/Connect/MySQLDatabase.php index ad9730709..051ab965f 100644 --- a/src/ORM/Connect/MySQLDatabase.php +++ b/src/ORM/Connect/MySQLDatabase.php @@ -2,6 +2,8 @@ namespace SilverStripe\ORM\Connect; +use SilverStripe\Assets\File; +use SilverStripe\CMS\Model\SiteTree; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Convert; use SilverStripe\ORM\PaginatedList; @@ -144,7 +146,7 @@ class MySQLDatabase extends Database * @param bool $booleanSearch * @param string $alternativeFileFilter * @param bool $invertedMatch - * @return \SilverStripe\ORM\PaginatedList + * @return PaginatedList * @throws Exception */ public function searchEngine( @@ -158,10 +160,8 @@ class MySQLDatabase extends Database $alternativeFileFilter = "", $invertedMatch = false ) { - $pageClass = 'SilverStripe\\CMS\\Model\\SiteTree'; - $fileClass = 'SilverStripe\\Assets\\File'; - $pageTable = DataObject::getSchema()->tableName($pageClass); - $fileTable = DataObject::getSchema()->tableName($fileClass); + $pageClass = SiteTree::class; + $fileClass = File::class; if (!class_exists($pageClass)) { throw new Exception('MySQLDatabase->searchEngine() requires "SiteTree" class'); } @@ -194,12 +194,13 @@ class MySQLDatabase extends Database // File.ShowInSearch was added later, keep the database driver backwards compatible // by checking for its existence first + $fileTable = DataObject::getSchema()->tableName($fileClass); $fields = $this->getSchemaManager()->fieldList($fileTable); if (array_key_exists('ShowInSearch', $fields)) { $extraFilters[$fileClass] .= " AND ShowInSearch <> 0"; } - $limit = $start . ", " . (int) $pageLength; + $limit = (int)$start . ", " . (int)$pageLength; $notMatch = $invertedMatch ? "NOT " @@ -257,7 +258,6 @@ class MySQLDatabase extends Database $queryParameters = array(); $totalCount = 0; foreach ($lists as $class => $list) { - $table = DataObject::getSchema()->tableName($class); /** @var SQLSelect $query */ $query = $list->dataQuery()->query(); diff --git a/src/ORM/PaginatedList.php b/src/ORM/PaginatedList.php index 7df431e5a..548861eae 100644 --- a/src/ORM/PaginatedList.php +++ b/src/ORM/PaginatedList.php @@ -87,7 +87,7 @@ class PaginatedList extends ListDecorator */ public function setPageLength($length) { - $this->pageLength = $length; + $this->pageLength = (int)$length; return $this; } @@ -99,7 +99,7 @@ class PaginatedList extends ListDecorator */ public function setCurrentPage($page) { - $this->pageStart = ($page - 1) * $this->getPageLength(); + $this->pageStart = ((int)$page - 1) * $this->getPageLength(); return $this; } @@ -134,7 +134,7 @@ class PaginatedList extends ListDecorator */ public function setPageStart($start) { - $this->pageStart = $start; + $this->pageStart = (int)$start; return $this; } @@ -161,7 +161,7 @@ class PaginatedList extends ListDecorator */ public function setTotalItems($items) { - $this->totalItems = $items; + $this->totalItems = (int)$items; return $this; } From 7a79cd039a96ef54182263d5fbb72addf093b171 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Mon, 27 Nov 2017 18:15:53 +1300 Subject: [PATCH 02/10] [SS-2017-010] Prevent install.php from disclosing system passwords --- src/Dev/Install/InstallConfig.php | 38 +++++++++++++++++++------ src/Dev/Install/Installer.php | 5 ++++ src/Dev/Install/config-form.html | 17 ++++++------ src/Dev/Install/install5.php | 46 +++++++++++++++++-------------- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/src/Dev/Install/InstallConfig.php b/src/Dev/Install/InstallConfig.php index 6d6b10088..21465392b 100644 --- a/src/Dev/Install/InstallConfig.php +++ b/src/Dev/Install/InstallConfig.php @@ -26,18 +26,23 @@ class InstallConfig * * @param array $request Request object * @param array $databaseClasses Supported database config + * @param bool $realPassword Set to true to get the real password. If false, any non-posted + * password will be redacted as '********'. Note: Posted passwords are considered disclosed and + * never redacted. * @return array */ - public function getDatabaseConfig($request, $databaseClasses) + public function getDatabaseConfig($request, $databaseClasses, $realPassword = true) { // Get config from request if (isset($request['db']['type'])) { $type = $request['db']['type']; if (isset($request['db'][$type])) { - return array_merge( - [ 'type' => $type ], - $request['db'][$type] - ); + $config = $request['db'][$type]; + // The posted placeholder must be substituted with the real password + if (isset($config['password']) && $config['password'] === Installer::PASSWORD_PLACEHOLDER) { + $config['password'] = Environment::getEnv('SS_DATABASE_PASSWORD') ?: ''; + } + return array_merge([ 'type' => $type ], $config); } } @@ -46,8 +51,16 @@ class InstallConfig 'type' => $this->getDatabaseClass($databaseClasses), 'server' => Environment::getEnv('SS_DATABASE_SERVER') ?: 'localhost', 'username' => Environment::getEnv('SS_DATABASE_USERNAME') ?: 'root', - 'password' => Environment::getEnv('SS_DATABASE_PASSWORD') ?: '', + 'password' => $realPassword + ? (Environment::getEnv('SS_DATABASE_PASSWORD') ?: '') + : Installer::PASSWORD_PLACEHOLDER, // Avoid password disclosure 'database' => Environment::getEnv('SS_DATABASE_NAME') ?: 'SS_mysite', + 'path' => Environment::getEnv('SS_DATABASE_PATH') + ?: Environment::getEnv('SS_SQLITE_DATABASE_PATH') // sqlite compat + ?: null, + 'key' => Environment::getEnv('SS_DATABASE_KEY') + ?: Environment::getEnv('SS_SQLITE_DATABASE_KEY') // sqlite compat + ?: null, ]; } @@ -55,17 +68,26 @@ class InstallConfig * Get admin config from the environment * * @param array $request + * @param bool $realPassword Set to true to get the real password. If false, any non-posted + * password will be redacted as '********'. Note: Posted passwords are considered disclosed and + * never redacted. * @return array */ - public function getAdminConfig($request) + public function getAdminConfig($request, $realPassword = true) { if (isset($request['admin'])) { + $config = $request['admin']; + if (isset($config['password']) && $config['password'] === Installer::PASSWORD_PLACEHOLDER) { + $config['password'] = Environment::getEnv('SS_DEFAULT_ADMIN_PASSWORD') ?: ''; + } return $request['admin']; } return [ 'username' => Environment::getEnv('SS_DEFAULT_ADMIN_USERNAME') ?: 'admin', - 'password' => Environment::getEnv('SS_DEFAULT_ADMIN_PASSWORD') ?: '', + 'password' => $realPassword + ? (Environment::getEnv('SS_DEFAULT_ADMIN_PASSWORD') ?: '') + : Installer::PASSWORD_PLACEHOLDER, // Avoid password disclosure ]; } diff --git a/src/Dev/Install/Installer.php b/src/Dev/Install/Installer.php index 17d1ebe62..55e61adeb 100644 --- a/src/Dev/Install/Installer.php +++ b/src/Dev/Install/Installer.php @@ -20,6 +20,11 @@ use SilverStripe\Security\Security; */ class Installer extends InstallRequirements { + /** + * value='' attribute placeholder for password fields + */ + const PASSWORD_PLACEHOLDER = '********'; + protected function installHeader() { ?> diff --git a/src/Dev/Install/config-form.html b/src/Dev/Install/config-form.html index 6d2851210..5a8149dbd 100644 --- a/src/Dev/Install/config-form.html +++ b/src/Dev/Install/config-form.html @@ -123,14 +123,15 @@ echo $checked ? '
' : '