From e757b649b7b6415ae5f77e59b5160052896b2c21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 18:06:45 +0100 Subject: [PATCH] fix: Fix psalm taint false-positives by small refactorings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Mostly make it clear that we trust admin input or that we correctly escape strings. Signed-off-by: Côme Chilliet --- .../lib/Config/ConfigAdapter.php | 18 +++++-- .../lib/Controller/AppsController.php | 27 +++++++++- apps/theming/lib/Util.php | 7 ++- build/psalm-baseline-security.xml | 36 -------------- core/Controller/SetupController.php | 3 ++ lib/private/Config.php | 4 +- lib/private/Server.php | 1 - 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 c84fbb19102bd..db53c8cf6c92b 100644 --- a/apps/files_external/lib/Config/ConfigAdapter.php +++ b/apps/files_external/lib/Config/ConfigAdapter.php @@ -39,6 +39,19 @@ public function __construct( ) { } + /** + * @param class-string $class + * @return class-string + * @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 * @@ -51,10 +64,7 @@ private function prepareStorageConfig(StorageConfig &$storage, IUser $user): voi $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 04dfd8f29b190..4d32584591b15 100644 --- a/apps/provisioning_api/lib/Controller/AppsController.php +++ b/apps/provisioning_api/lib/Controller/AppsController.php @@ -27,6 +27,17 @@ public function __construct( parent::__construct($appName, $request); } + /** + * @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 * @@ -71,6 +82,11 @@ public function getApps(?string $filter = null): DataResponse { * 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 @@ public function getAppInfo(string $app): DataResponse { #[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 @@ public function enable(string $app): DataResponse { * * @param string $app ID of the app * @return DataResponse, 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 c4e1c6b210705..b7022dae8b920 100644 --- a/apps/theming/lib/Util.php +++ b/apps/theming/lib/Util.php @@ -197,7 +197,7 @@ public function generateRadioButton($color) { * @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 @@ public function getAppIcon($app) { * @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 2777f4e7734bb..d9ab9c9107606 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -1,41 +1,5 @@ - - - - - - - - - - - - - - cache]]> - - - - - getPathname(), '.php')]]> - - - - - passphrase]]> - - - - - - - - - - - - diff --git a/core/Controller/SetupController.php b/core/Controller/SetupController.php index eb78d74dd4e0f..bd3b8265c34b9 100644 --- a/core/Controller/SetupController.php +++ b/core/Controller/SetupController.php @@ -97,6 +97,9 @@ private function finishSetup(): void { 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 3ec21df9f7c84..a9eb58a186646 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -266,7 +266,7 @@ private function readData() { * @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 @@ private function writeData() { // Create a php file ... $content = "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 968d469aa74a0..77759de30c5d1 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1109,7 +1109,6 @@ public function __construct($webRoot, \OC\Config $config) { ); 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 aceb387ea741f..380c699d32d15 100644 --- a/lib/private/Session/CryptoWrapper.php +++ b/lib/private/Session/CryptoWrapper.php @@ -1,13 +1,15 @@ 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 @@ public function __construct(IConfig $config, setcookie( self::COOKIE_NAME, - $this->passphrase, + $passphrase, [ 'expires' => 0, 'path' => $webRoot, @@ -83,13 +67,10 @@ public function __construct(IConfig $config, ); } } + $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); }