diff options
author | Côme Chilliet <come.chilliet@nextcloud.com> | 2025-02-17 18:06:45 +0100 |
---|---|---|
committer | Côme Chilliet <come.chilliet@nextcloud.com> | 2025-02-17 18:08:23 +0100 |
commit | e757b649b7b6415ae5f77e59b5160052896b2c21 (patch) | |
tree | 21f63c87b66d8f316c5c2166e58dfb547db79b65 | |
parent | 9edabfa21fa7e587c0ad95d2d230d215b060ade0 (diff) | |
download | nextcloud-server-fix/fix-psalm-taint-errors-2.tar.gz nextcloud-server-fix/fix-psalm-taint-errors-2.zip |
fix: Fix psalm taint false-positives by small refactoringsfix/fix-psalm-taint-errors-2
Mostly make it clear that we trust admin input or that we correctly
escape strings.
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
-rw-r--r-- | apps/files_external/lib/Config/ConfigAdapter.php | 18 | ||||
-rw-r--r-- | apps/provisioning_api/lib/Controller/AppsController.php | 27 | ||||
-rw-r--r-- | apps/theming/lib/Util.php | 7 | ||||
-rw-r--r-- | build/psalm-baseline-security.xml | 36 | ||||
-rw-r--r-- | core/Controller/SetupController.php | 3 | ||||
-rw-r--r-- | lib/private/Config.php | 4 | ||||
-rw-r--r-- | lib/private/Server.php | 1 | ||||
-rw-r--r-- | lib/private/Session/CryptoWrapper.php | 49 |
8 files changed, 65 insertions, 80 deletions
diff --git a/apps/files_external/lib/Config/ConfigAdapter.php b/apps/files_external/lib/Config/ConfigAdapter.php index c84fbb19102..db53c8cf6c9 100644 --- a/apps/files_external/lib/Config/ConfigAdapter.php +++ b/apps/files_external/lib/Config/ConfigAdapter.php @@ -40,6 +40,19 @@ class ConfigAdapter implements IMountProvider { } /** + * @param class-string $class + * @return class-string<IObjectStore> + * @throws \InvalidArgumentException + * @psalm-taint-escape callable + */ + private function validateObjectStoreClassString(string $class): string { + if (!\is_subclass_of($class, IObjectStore::class)) { + throw new \InvalidArgumentException('Invalid object store'); + } + return $class; + } + + /** * Process storage ready for mounting * * @throws QueryException @@ -51,10 +64,7 @@ class ConfigAdapter implements IMountProvider { $objectStore = $storage->getBackendOption('objectstore'); if ($objectStore) { - $objectClass = $objectStore['class']; - if (!is_subclass_of($objectClass, IObjectStore::class)) { - throw new \InvalidArgumentException('Invalid object store'); - } + $objectClass = $this->validateObjectStoreClassString($objectStore['class']); $storage->setBackendOption('objectstore', new $objectClass($objectStore)); } diff --git a/apps/provisioning_api/lib/Controller/AppsController.php b/apps/provisioning_api/lib/Controller/AppsController.php index 04dfd8f29b1..4d32584591b 100644 --- a/apps/provisioning_api/lib/Controller/AppsController.php +++ b/apps/provisioning_api/lib/Controller/AppsController.php @@ -28,6 +28,17 @@ class AppsController extends OCSController { } /** + * @throws \InvalidArgumentException + */ + protected function verifyAppId(string $app): string { + $cleanId = $this->appManager->cleanAppId($app); + if ($cleanId !== $app) { + throw new \InvalidArgumentException('Invalid app id given'); + } + return $cleanId; + } + + /** * Get a list of installed apps * * @param ?string $filter Filter for enabled or disabled apps @@ -71,6 +82,11 @@ class AppsController extends OCSController { * 200: App info returned */ public function getAppInfo(string $app): DataResponse { + try { + $app = $this->verifyAppId($app); + } catch (\InvalidArgumentException $e) { + throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED); + } $info = $this->appManager->getAppInfo($app); if (!is_null($info)) { return new DataResponse($info); @@ -91,7 +107,10 @@ class AppsController extends OCSController { #[PasswordConfirmationRequired] public function enable(string $app): DataResponse { try { + $app = $this->verifyAppId($app); $this->appManager->enableApp($app); + } catch (\InvalidArgumentException $e) { + throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED); } catch (AppPathNotFoundException $e) { throw new OCSException('The request app was not found', OCSController::RESPOND_NOT_FOUND); } @@ -103,12 +122,18 @@ class AppsController extends OCSController { * * @param string $app ID of the app * @return DataResponse<Http::STATUS_OK, list<empty>, array{}> + * @throws OCSException * * 200: App disabled successfully */ #[PasswordConfirmationRequired] public function disable(string $app): DataResponse { - $this->appManager->disableApp($app); + try { + $app = $this->verifyAppId($app); + $this->appManager->disableApp($app); + } catch (\InvalidArgumentException $e) { + throw new OCSException($e->getMessage(), OCSController::RESPOND_UNAUTHORISED); + } return new DataResponse(); } } diff --git a/apps/theming/lib/Util.php b/apps/theming/lib/Util.php index c4e1c6b2107..b7022dae8b9 100644 --- a/apps/theming/lib/Util.php +++ b/apps/theming/lib/Util.php @@ -197,7 +197,7 @@ class Util { * @return string|ISimpleFile path to app icon / file of logo */ public function getAppIcon($app) { - $app = str_replace(['\0', '/', '\\', '..'], '', $app); + $app = $this->appManager->cleanAppId($app); try { $appPath = $this->appManager->getAppPath($app); $icon = $appPath . '/img/' . $app . '.svg'; @@ -228,7 +228,10 @@ class Util { * @return string|false absolute path to image */ public function getAppImage($app, $image) { - $app = str_replace(['\0', '/', '\\', '..'], '', $app); + $app = $this->appManager->cleanAppId($app); + /** + * @psalm-taint-escape file + */ $image = str_replace(['\0', '\\', '..'], '', $image); if ($app === 'core') { $icon = \OC::$SERVERROOT . '/core/img/' . $image; diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index 2777f4e7734..d9ab9c91076 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -1,41 +1,5 @@ <?xml version="1.0" encoding="UTF-8"?> <files psalm-version="5.26.1@d747f6500b38ac4f7dfc5edbcae6e4b637d7add0"> - <file src="apps/files_external/lib/Config/ConfigAdapter.php"> - <TaintedCallable> - <code><![CDATA[$objectClass]]></code> - </TaintedCallable> - </file> - <file src="apps/theming/lib/IconBuilder.php"> - <TaintedFile> - <code><![CDATA[$appIcon]]></code> - <code><![CDATA[$imageFile]]></code> - </TaintedFile> - </file> - <file src="lib/private/Config.php"> - <TaintedHtml> - <code><![CDATA[$this->cache]]></code> - </TaintedHtml> - </file> - <file src="lib/private/Route/Router.php"> - <TaintedCallable> - <code><![CDATA[$appNameSpace . '\\Controller\\' . basename($file->getPathname(), '.php')]]></code> - </TaintedCallable> - </file> - <file src="lib/private/Session/CryptoWrapper.php"> - <TaintedCookie> - <code><![CDATA[$this->passphrase]]></code> - </TaintedCookie> - </file> - <file src="lib/private/Setup.php"> - <TaintedFile> - <code><![CDATA[$dataDir]]></code> - </TaintedFile> - </file> - <file src="lib/private/Setup/Sqlite.php"> - <TaintedFile> - <code><![CDATA[$sqliteFile]]></code> - </TaintedFile> - </file> <file src="lib/public/DB/QueryBuilder/IQueryBuilder.php"> <TaintedSql> <code><![CDATA[$column]]></code> diff --git a/core/Controller/SetupController.php b/core/Controller/SetupController.php index eb78d74dd4e..bd3b8265c34 100644 --- a/core/Controller/SetupController.php +++ b/core/Controller/SetupController.php @@ -97,6 +97,9 @@ class SetupController { exit(); } + /** + * @psalm-taint-escape file we trust file path given in POST for setup + */ public function loadAutoConfig(array $post): array { if (file_exists($this->autoConfigFile)) { $this->logger->info('Autoconfig file found, setting up Nextcloud…'); diff --git a/lib/private/Config.php b/lib/private/Config.php index 3ec21df9f7c..a9eb58a1866 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -266,7 +266,7 @@ class Config { * @throws HintException If the config file cannot be written to * @throws \Exception If no file lock can be acquired */ - private function writeData() { + private function writeData(): void { $this->checkReadOnly(); if (!is_file(\OC::$configDir . '/CAN_INSTALL') && !isset($this->cache['version'])) { @@ -276,7 +276,7 @@ class Config { // Create a php file ... $content = "<?php\n"; $content .= '$CONFIG = '; - $content .= var_export($this->cache, true); + $content .= var_export(self::trustSystemConfig($this->cache), true); $content .= ";\n"; touch($this->configFilePath); diff --git a/lib/private/Server.php b/lib/private/Server.php index 968d469aa74..77759de30c5 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1109,7 +1109,6 @@ class Server extends ServerContainer implements IServerContainer { ); return new CryptoWrapper( - $c->get(\OCP\IConfig::class), $c->get(ICrypto::class), $c->get(ISecureRandom::class), $request diff --git a/lib/private/Session/CryptoWrapper.php b/lib/private/Session/CryptoWrapper.php index aceb387ea74..380c699d32d 100644 --- a/lib/private/Session/CryptoWrapper.php +++ b/lib/private/Session/CryptoWrapper.php @@ -1,13 +1,15 @@ <?php +declare(strict_types=1); + /** * SPDX-FileCopyrightText: 2016-2024 Nextcloud GmbH and Nextcloud contributors * SPDX-FileCopyrightText: 2016 ownCloud, Inc. * SPDX-License-Identifier: AGPL-3.0-only */ + namespace OC\Session; -use OCP\IConfig; use OCP\IRequest; use OCP\ISession; use OCP\Security\ICrypto; @@ -30,37 +32,19 @@ use OCP\Security\ISecureRandom; * @package OC\Session */ class CryptoWrapper { + /** @var string */ public const COOKIE_NAME = 'oc_sessionPassphrase'; - /** @var IConfig */ - protected $config; - /** @var ISession */ - protected $session; - /** @var ICrypto */ - protected $crypto; - /** @var ISecureRandom */ - protected $random; - /** @var string */ - protected $passphrase; + protected string $passphrase; - /** - * @param IConfig $config - * @param ICrypto $crypto - * @param ISecureRandom $random - * @param IRequest $request - */ - public function __construct(IConfig $config, - ICrypto $crypto, + public function __construct( + protected ICrypto $crypto, ISecureRandom $random, - IRequest $request) { - $this->crypto = $crypto; - $this->config = $config; - $this->random = $random; - - if (!is_null($request->getCookie(self::COOKIE_NAME))) { - $this->passphrase = $request->getCookie(self::COOKIE_NAME); - } else { - $this->passphrase = $this->random->generate(128); + IRequest $request, + ) { + $passphrase = $request->getCookie(self::COOKIE_NAME); + if ($passphrase === null) { + $passphrase = $random->generate(128); $secureCookie = $request->getServerProtocol() === 'https'; // FIXME: Required for CI if (!defined('PHPUNIT_RUN')) { @@ -71,7 +55,7 @@ class CryptoWrapper { setcookie( self::COOKIE_NAME, - $this->passphrase, + $passphrase, [ 'expires' => 0, 'path' => $webRoot, @@ -83,13 +67,10 @@ class CryptoWrapper { ); } } + $this->passphrase = $passphrase; } - /** - * @param ISession $session - * @return ISession - */ - public function wrapSession(ISession $session) { + public function wrapSession(ISession $session): ISession { if (!($session instanceof CryptoSessionData)) { return new CryptoSessionData($session, $this->crypto, $this->passphrase); } |