From 579a337750c85bab1f1e6d798c10cbb012f3f819 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 13 Feb 2025 18:06:43 +0100 Subject: [PATCH 1/9] fix: Fix psalm taint error in L10N factory MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/L10N/Factory.php | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/private/L10N/Factory.php b/lib/private/L10N/Factory.php index eb84f264f5fc8..5645693f8d908 100644 --- a/lib/private/L10N/Factory.php +++ b/lib/private/L10N/Factory.php @@ -108,9 +108,7 @@ public function get($app, $lang = null, $locale = null) { $locale = $forceLocale; } - if ($lang === null || !$this->languageExists($app, $lang)) { - $lang = $this->findLanguage($app); - } + $lang = $this->validateLanguage($app, $lang); if ($locale === null || !$this->localeExists($locale)) { $locale = $this->findLocale($lang); @@ -130,6 +128,29 @@ public function get($app, $lang = null, $locale = null) { }); } + /** + * Check that $lang is an existing language and not null, otherwise return the language to use instead + * + * @psalm-taint-escape callable + * @psalm-taint-escape cookie + * @psalm-taint-escape file + * @psalm-taint-escape has_quotes + * @psalm-taint-escape header + * @psalm-taint-escape html + * @psalm-taint-escape include + * @psalm-taint-escape ldap + * @psalm-taint-escape shell + * @psalm-taint-escape sql + * @psalm-taint-escape unserialize + */ + private function validateLanguage(string $app, ?string $lang): string { + if ($lang === null || !$this->languageExists($app, $lang)) { + return $this->findLanguage($app); + } else { + return $lang; + } + } + /** * Find the best language * From fec865cc29ee0bc54dbd29c07e8cbe3d477bfca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 11:16:27 +0100 Subject: [PATCH 2/9] chore: Correctly flag json encoding methods as escaping html and quotes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Especially with JSON_HEX_TAG it’s perfectly fine to echo JSON, and we only use it in JSON output anyway. Signed-off-by: Côme Chilliet --- build/psalm-baseline-security.xml | 8 -------- lib/private/legacy/OC_JSON.php | 5 +++-- lib/public/AppFramework/Http/JSONResponse.php | 3 +++ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index 8740e96d78bb8..f15718796c23f 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -104,12 +104,4 @@ - - - buildProviderList()->render()]]> - - - buildProviderList()->render()]]> - - diff --git a/lib/private/legacy/OC_JSON.php b/lib/private/legacy/OC_JSON.php index d2b8595112341..6daef18dd61e5 100644 --- a/lib/private/legacy/OC_JSON.php +++ b/lib/private/legacy/OC_JSON.php @@ -74,7 +74,6 @@ public static function checkAdminUser() { * Send json error msg * @deprecated 12.0.0 Use a AppFramework JSONResponse instead * @suppress PhanDeprecatedFunction - * @psalm-taint-escape html */ public static function error($data = []) { $data['status'] = 'error'; @@ -86,7 +85,6 @@ public static function error($data = []) { * Send json success msg * @deprecated 12.0.0 Use a AppFramework JSONResponse instead * @suppress PhanDeprecatedFunction - * @psalm-taint-escape html */ public static function success($data = []) { $data['status'] = 'success'; @@ -97,6 +95,9 @@ public static function success($data = []) { /** * Encode JSON * @deprecated 12.0.0 Use a AppFramework JSONResponse instead + * + * @psalm-taint-escape has_quotes + * @psalm-taint-escape html */ private static function encode($data) { return json_encode($data, JSON_HEX_TAG); diff --git a/lib/public/AppFramework/Http/JSONResponse.php b/lib/public/AppFramework/Http/JSONResponse.php index efcf79d5e87a6..a226e29a1b58c 100644 --- a/lib/public/AppFramework/Http/JSONResponse.php +++ b/lib/public/AppFramework/Http/JSONResponse.php @@ -58,6 +58,9 @@ public function __construct( * @return string the rendered json * @since 6.0.0 * @throws \Exception If data could not get encoded + * + * @psalm-taint-escape has_quotes + * @psalm-taint-escape html */ public function render() { return json_encode($this->data, JSON_HEX_TAG | JSON_THROW_ON_ERROR | $this->encodeFlags, 2048); From 85fbd3eb0ab1650f9385acd869d5f4ab21006a8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 11:59:23 +0100 Subject: [PATCH 3/9] fix: Work around psalm taint false-positive by not using var_export MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit var_export is listed as a taint sink because it may output stuff depending on the parameters. It was not the case here, but we can simply json_encode the result by passing it as context to the logger method rather than using var_export. Signed-off-by: Côme Chilliet --- lib/private/TaskProcessing/Manager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/TaskProcessing/Manager.php b/lib/private/TaskProcessing/Manager.php index 07e643ab004dc..0582d801e3d98 100644 --- a/lib/private/TaskProcessing/Manager.php +++ b/lib/private/TaskProcessing/Manager.php @@ -999,7 +999,7 @@ public function setTaskResult(int $id, ?string $error, ?array $result, bool $isU $task->setEndedAt(time()); $error = 'The task was processed successfully but the provider\'s output doesn\'t pass validation against the task type\'s outputShape spec and/or the provider\'s own optionalOutputShape spec'; $task->setErrorMessage($error); - $this->logger->error($error . ' Output was: ' . var_export($result, true), ['exception' => $e]); + $this->logger->error($error, ['exception' => $e, 'output' => $result]); } catch (NotPermittedException $e) { $task->setProgress(1); $task->setStatus(Task::STATUS_FAILED); From 25f38883f168df228940c816d41329918407ad35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 12:12:02 +0100 Subject: [PATCH 4/9] fix: Work around false-positive psalm taint error calling print_r in admin_audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same issue as var_export, print_r is listed as sink but it’s not when using return:true. Anyway, using the logger context feature is better. Signed-off-by: Côme Chilliet --- apps/admin_audit/lib/Actions/Action.php | 7 ++----- build/psalm-baseline-security.xml | 5 ----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/apps/admin_audit/lib/Actions/Action.php b/apps/admin_audit/lib/Actions/Action.php index 2566025a8ce57..acd415d82ea9e 100644 --- a/apps/admin_audit/lib/Actions/Action.php +++ b/apps/admin_audit/lib/Actions/Action.php @@ -37,11 +37,8 @@ public function log(string $text, ); } else { $this->logger->critical( - sprintf( - '$params["' . $element . '"] was missing. Transferred value: %s', - print_r($params, true) - ), - ['app' => 'admin_audit'] + '$params["' . $element . '"] was missing. Transferred value: {params}', + ['app' => 'admin_audit', 'params' => $params] ); } return; diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index f15718796c23f..4df4cd7d6875e 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -1,10 +1,5 @@ - - - - - From aac79bad9b70716514345370cb98066aa11138a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 12:35:18 +0100 Subject: [PATCH 5/9] fix: Move config.php taint trust upstream directly in OC\Config class MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This solves some false-positive psalm taint errors Signed-off-by: Côme Chilliet --- build/psalm-baseline-security.xml | 34 ------------------------------- lib/private/Config.php | 24 ++++++++++++++++++++-- lib/private/SystemConfig.php | 20 +----------------- 3 files changed, 23 insertions(+), 55 deletions(-) diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index 4df4cd7d6875e..45f0e54f6486c 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -11,32 +11,11 @@ - - - - - - cache]]> - - - - - - - - - - - - - - - @@ -65,19 +44,6 @@ - - - - - - - - - - - - - getTraceAsString()]]> diff --git a/lib/private/Config.php b/lib/private/Config.php index 0e8d07955af06..3ec21df9f7c84 100644 --- a/lib/private/Config.php +++ b/lib/private/Config.php @@ -65,16 +65,36 @@ public function getKeys() { */ public function getValue($key, $default = null) { if (isset($this->envCache[$key])) { - return $this->envCache[$key]; + return self::trustSystemConfig($this->envCache[$key]); } if (isset($this->cache[$key])) { - return $this->cache[$key]; + return self::trustSystemConfig($this->cache[$key]); } return $default; } + /** + * Since system config is admin controlled, we can tell psalm to ignore any taint + * + * @psalm-taint-escape callable + * @psalm-taint-escape cookie + * @psalm-taint-escape file + * @psalm-taint-escape has_quotes + * @psalm-taint-escape header + * @psalm-taint-escape html + * @psalm-taint-escape include + * @psalm-taint-escape ldap + * @psalm-taint-escape shell + * @psalm-taint-escape sql + * @psalm-taint-escape unserialize + * @psalm-pure + */ + public static function trustSystemConfig(mixed $value): mixed { + return $value; + } + /** * Sets and deletes values and writes the config.php * diff --git a/lib/private/SystemConfig.php b/lib/private/SystemConfig.php index d3fd1f2ab049c..57777b06ed601 100644 --- a/lib/private/SystemConfig.php +++ b/lib/private/SystemConfig.php @@ -116,24 +116,6 @@ public function __construct( ) { } - /** - * Since system config is admin controlled, we can tell psalm to ignore any taint - * - * @psalm-taint-escape sql - * @psalm-taint-escape html - * @psalm-taint-escape ldap - * @psalm-taint-escape callable - * @psalm-taint-escape file - * @psalm-taint-escape ssrf - * @psalm-taint-escape cookie - * @psalm-taint-escape header - * @psalm-taint-escape has_quotes - * @psalm-pure - */ - public static function trustSystemConfig(mixed $value): mixed { - return $value; - } - /** * Lists all available config keys * @return array an array of key names @@ -170,7 +152,7 @@ public function setValues(array $configs) { * @return mixed the value or $default */ public function getValue($key, $default = '') { - return $this->trustSystemConfig($this->config->getValue($key, $default)); + return $this->config->getValue($key, $default); } /** From fa108d5b5414d8fdfa1e5eecd9a7d871d58f4b28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 14:13:23 +0100 Subject: [PATCH 6/9] fix: Correctly tag json encoding in BaseResponse to fix false-positive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …in psalm taint analysis Signed-off-by: Côme Chilliet --- build/psalm-baseline-security.xml | 8 -------- lib/private/AppFramework/OCS/BaseResponse.php | 10 +++++++++- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index 45f0e54f6486c..c7b083b22c520 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -16,14 +16,6 @@ cache]]> - - - - - - - - getPathname(), '.php')]]> diff --git a/lib/private/AppFramework/OCS/BaseResponse.php b/lib/private/AppFramework/OCS/BaseResponse.php index cc7f784576004..5929a3993ec0f 100644 --- a/lib/private/AppFramework/OCS/BaseResponse.php +++ b/lib/private/AppFramework/OCS/BaseResponse.php @@ -99,7 +99,7 @@ protected function renderResult(array $meta): string { ]; if ($this->format === 'json') { - return json_encode($response, JSON_HEX_TAG); + return $this->toJson($response); } $writer = new \XMLWriter(); @@ -111,6 +111,14 @@ protected function renderResult(array $meta): string { return $writer->outputMemory(true); } + /** + * @psalm-taint-escape has_quotes + * @psalm-taint-escape html + */ + protected function toJson(array $array): string { + return \json_encode($array, \JSON_HEX_TAG); + } + protected function toXML(array $array, \XMLWriter $writer): void { foreach ($array as $k => $v) { if ($k === '@attributes' && is_array($v)) { From 7c907223d2c61df3a3ee3ec25cf4d48f058c5751 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 14:28:30 +0100 Subject: [PATCH 7/9] fix: Fix psalm taint false-positive by escaping trusted input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- build/psalm-baseline-security.xml | 8 -------- lib/private/Setup/MySQL.php | 22 +++++++++++----------- 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index c7b083b22c520..d31034d538dd8 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -49,12 +49,4 @@ - - - - - - - - diff --git a/lib/private/Setup/MySQL.php b/lib/private/Setup/MySQL.php index 2708ada31c150..6dd9855d85121 100644 --- a/lib/private/Setup/MySQL.php +++ b/lib/private/Setup/MySQL.php @@ -59,7 +59,7 @@ public function setupDatabase($username) { /** * @param \OC\DB\Connection $connection */ - private function createDatabase($connection) { + private function createDatabase($connection): void { try { $name = $this->dbName; $user = $this->dbUser; @@ -91,7 +91,7 @@ private function createDatabase($connection) { * @param IDBConnection $connection * @throws \OC\DatabaseSetupException */ - private function createDBUser($connection) { + private function createDBUser($connection): void { try { $name = $this->dbUser; $password = $this->dbPassword; @@ -99,15 +99,15 @@ private function createDBUser($connection) { // the anonymous user would take precedence when there is one. if ($connection->getDatabasePlatform() instanceof Mysql80Platform) { - $query = "CREATE USER '$name'@'localhost' IDENTIFIED WITH mysql_native_password BY '$password'"; - $connection->executeUpdate($query); - $query = "CREATE USER '$name'@'%' IDENTIFIED WITH mysql_native_password BY '$password'"; - $connection->executeUpdate($query); + $query = "CREATE USER ?@'localhost' IDENTIFIED WITH mysql_native_password BY ?"; + $connection->executeUpdate($query, [$name,$password]); + $query = "CREATE USER ?@'%' IDENTIFIED WITH mysql_native_password BY ?"; + $connection->executeUpdate($query, [$name,$password]); } else { - $query = "CREATE USER '$name'@'localhost' IDENTIFIED BY '$password'"; - $connection->executeUpdate($query); - $query = "CREATE USER '$name'@'%' IDENTIFIED BY '$password'"; - $connection->executeUpdate($query); + $query = "CREATE USER ?@'localhost' IDENTIFIED BY ?"; + $connection->executeUpdate($query, [$name,$password]); + $query = "CREATE USER ?@'%' IDENTIFIED BY ?"; + $connection->executeUpdate($query, [$name,$password]); } } catch (\Exception $ex) { $this->logger->error('Database user creation failed.', [ @@ -119,7 +119,7 @@ private function createDBUser($connection) { } /** - * @param $username + * @param string $username * @param IDBConnection $connection */ private function createSpecificUser($username, $connection): void { From 640dbd0b5e38ef603c4edcc646ed7df8117c9963 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 15:00:58 +0100 Subject: [PATCH 8/9] fix: Fix false-positive psalm taint errors when outputting plain text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- build/psalm-baseline-security.xml | 8 -------- lib/private/legacy/OC_Template.php | 12 ++++++++++-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/build/psalm-baseline-security.xml b/build/psalm-baseline-security.xml index d31034d538dd8..2777f4e7734bb 100644 --- a/build/psalm-baseline-security.xml +++ b/build/psalm-baseline-security.xml @@ -36,14 +36,6 @@ - - - getTraceAsString()]]> - - - getTraceAsString()]]> - - diff --git a/lib/private/legacy/OC_Template.php b/lib/private/legacy/OC_Template.php index 1026e536b97b3..af363e0a41e69 100644 --- a/lib/private/legacy/OC_Template.php +++ b/lib/private/legacy/OC_Template.php @@ -313,7 +313,15 @@ public static function printExceptionErrorPage($exception, $statusCode = 503) { die(); } - private static function printPlainErrorPage(\Throwable $exception, bool $debug = false) { + /** + * @psalm-taint-escape has_quotes + * @psalm-taint-escape html + */ + private static function fakeEscapeForPlainText(string $str): string { + return $str; + } + + private static function printPlainErrorPage(\Throwable $exception, bool $debug = false): void { header('Content-Type: text/plain; charset=utf-8'); print("Internal Server Error\n\n"); print("The server encountered an internal error and was unable to complete your request.\n"); @@ -323,7 +331,7 @@ private static function printPlainErrorPage(\Throwable $exception, bool $debug = if ($debug) { print("\n"); print($exception->getMessage() . ' ' . $exception->getFile() . ' at ' . $exception->getLine() . "\n"); - print($exception->getTraceAsString()); + print(self::fakeEscapeForPlainText($exception->getTraceAsString())); } } } From c1c59f9a6c5b92ef09e3cf182b198de3633f4aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Mon, 17 Feb 2025 15:01:33 +0100 Subject: [PATCH 9/9] chore: Add missing star in phpdoc comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/AppFramework/Utility/SimpleContainer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/private/AppFramework/Utility/SimpleContainer.php b/lib/private/AppFramework/Utility/SimpleContainer.php index 56de4a34cf6d7..24918992ea3a3 100644 --- a/lib/private/AppFramework/Utility/SimpleContainer.php +++ b/lib/private/AppFramework/Utility/SimpleContainer.php @@ -176,7 +176,7 @@ public function registerAlias($alias, $target) { }, false); } - /* + /** * @param string $name * @return string */