aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorCôme Chilliet <come.chilliet@nextcloud.com>2025-02-17 18:06:45 +0100
committerCôme Chilliet <come.chilliet@nextcloud.com>2025-02-17 18:08:23 +0100
commite757b649b7b6415ae5f77e59b5160052896b2c21 (patch)
tree21f63c87b66d8f316c5c2166e58dfb547db79b65
parent9edabfa21fa7e587c0ad95d2d230d215b060ade0 (diff)
downloadnextcloud-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.php18
-rw-r--r--apps/provisioning_api/lib/Controller/AppsController.php27
-rw-r--r--apps/theming/lib/Util.php7
-rw-r--r--build/psalm-baseline-security.xml36
-rw-r--r--core/Controller/SetupController.php3
-rw-r--r--lib/private/Config.php4
-rw-r--r--lib/private/Server.php1
-rw-r--r--lib/private/Session/CryptoWrapper.php49
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);
}