From f4fc57cb83623b2e849070b7e90543fb32dc58b1 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 28 Aug 2024 17:03:53 +0200 Subject: [PATCH 1/3] fix: Use sha256 to hash arguments of background jobs This is to prevent collision as we are sometime hashing user input, yet using that hash to target the background job in the database. Signed-off-by: Louis Chemineau --- lib/private/BackgroundJob/JobList.php | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 77c25526fb8..7b23f4cdb37 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -43,7 +43,6 @@ use OCP\IDBConnection; use Psr\Log\LoggerInterface; use function get_class; use function json_encode; -use function md5; use function strlen; class JobList implements IJobList { @@ -80,7 +79,7 @@ class JobList implements IJobList { ->values([ 'class' => $query->createNamedParameter($class), 'argument' => $query->createNamedParameter($argumentJson), - 'argument_hash' => $query->createNamedParameter(md5($argumentJson)), + 'argument_hash' => $query->createNamedParameter(hash('sha256', $argumentJson)), 'last_run' => $query->createNamedParameter(0, IQueryBuilder::PARAM_INT), 'last_checked' => $query->createNamedParameter($firstCheck, IQueryBuilder::PARAM_INT), ]); @@ -90,7 +89,7 @@ class JobList implements IJobList { ->set('last_checked', $query->createNamedParameter($firstCheck, IQueryBuilder::PARAM_INT)) ->set('last_run', $query->createNamedParameter(0, IQueryBuilder::PARAM_INT)) ->where($query->expr()->eq('class', $query->createNamedParameter($class))) - ->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(md5($argumentJson)))); + ->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(hash('sha256', $argumentJson)))); } $query->executeStatement(); } @@ -115,7 +114,7 @@ class JobList implements IJobList { ->where($query->expr()->eq('class', $query->createNamedParameter($class))); if (!is_null($argument)) { $argumentJson = json_encode($argument); - $query->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(md5($argumentJson)))); + $query->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(hash('sha256', $argumentJson)))); } // Add galera safe delete chunking if using mysql @@ -160,7 +159,7 @@ class JobList implements IJobList { $query->select('id') ->from('jobs') ->where($query->expr()->eq('class', $query->createNamedParameter($class))) - ->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(md5($argument)))) + ->andWhere($query->expr()->eq('argument_hash', $query->createNamedParameter(hash('sha256', $argument)))) ->setMaxResults(1); $result = $query->executeQuery(); From 7f742cb852e3d72d9622fa9a367365c769423b0f Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Thu, 5 Sep 2024 18:08:39 +0200 Subject: [PATCH 2/3] fix: Migrate existing bg jobs to use sha256 Signed-off-by: Louis Chemineau --- .../Version28000Date20240828142927.php | 77 +++++++++++++++++++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 3 files changed, 79 insertions(+) create mode 100644 core/Migrations/Version28000Date20240828142927.php diff --git a/core/Migrations/Version28000Date20240828142927.php b/core/Migrations/Version28000Date20240828142927.php new file mode 100644 index 00000000000..9c0daff7a3b --- /dev/null +++ b/core/Migrations/Version28000Date20240828142927.php @@ -0,0 +1,77 @@ +getTable('jobs'); + $table->modifyColumn('argument_hash', [ + 'notnull' => false, + 'length' => 64, + ]); + + return $schema; + } + + public function postSchemaChange(IOutput $output, Closure $schemaClosure, array $options): void { + $chunkSize = 1000; + $offset = 0; + $nullHash = hash('sha256', 'null'); + + $selectQuery = $this->connection->getQueryBuilder() + ->select('*') + ->from('jobs') + ->setMaxResults($chunkSize); + + $insertQuery = $this->connection->getQueryBuilder(); + $insertQuery->update('jobs') + ->set('argument_hash', $insertQuery->createParameter('argument_hash')) + ->where($insertQuery->expr()->eq('id', $insertQuery->createParameter('id'))); + + do { + $result = $selectQuery + ->setFirstResult($offset) + ->executeQuery(); + + $jobs = $result->fetchAll(); + $count = count($jobs); + + foreach ($jobs as $jobRow) { + if ($jobRow['argument'] === 'null') { + $hash = $nullHash; + } else { + $hash = hash('sha256', $jobRow['argument']); + } + $insertQuery->setParameter('id', (string)$jobRow['id'], IQueryBuilder::PARAM_INT); + $insertQuery->setParameter('argument_hash', $hash); + $insertQuery->executeStatement(); + } + + $offset += $chunkSize; + } while ($count === $chunkSize); + } +} diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 855994de474..2b7b5b6e652 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1232,6 +1232,7 @@ return array( 'OC\\Core\\Migrations\\Version28000Date20231004103301' => $baseDir . '/core/Migrations/Version28000Date20231004103301.php', 'OC\\Core\\Migrations\\Version28000Date20231103104802' => $baseDir . '/core/Migrations/Version28000Date20231103104802.php', 'OC\\Core\\Migrations\\Version28000Date20231126110901' => $baseDir . '/core/Migrations/Version28000Date20231126110901.php', + 'OC\\Core\\Migrations\\Version28000Date20240828142927' => $baseDir . '/core/Migrations/Version28000Date20240828142927.php', 'OC\\Core\\Migrations\\Version30000Date20240814180800' => $baseDir . '/core/Migrations/Version30000Date20240814180800.php', 'OC\\Core\\Notification\\CoreNotifier' => $baseDir . '/core/Notification/CoreNotifier.php', 'OC\\Core\\Service\\LoginFlowV2Service' => $baseDir . '/core/Service/LoginFlowV2Service.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 22294859422..2dd883a794b 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1265,6 +1265,7 @@ class ComposerStaticInit749170dad3f5e7f9ca158f5a9f04f6a2 'OC\\Core\\Migrations\\Version28000Date20231004103301' => __DIR__ . '/../../..' . '/core/Migrations/Version28000Date20231004103301.php', 'OC\\Core\\Migrations\\Version28000Date20231103104802' => __DIR__ . '/../../..' . '/core/Migrations/Version28000Date20231103104802.php', 'OC\\Core\\Migrations\\Version28000Date20231126110901' => __DIR__ . '/../../..' . '/core/Migrations/Version28000Date20231126110901.php', + 'OC\\Core\\Migrations\\Version28000Date20240828142927' => __DIR__ . '/../../..' . '/core/Migrations/Version28000Date20240828142927.php', 'OC\\Core\\Migrations\\Version30000Date20240814180800' => __DIR__ . '/../../..' . '/core/Migrations/Version30000Date20240814180800.php', 'OC\\Core\\Notification\\CoreNotifier' => __DIR__ . '/../../..' . '/core/Notification/CoreNotifier.php', 'OC\\Core\\Service\\LoginFlowV2Service' => __DIR__ . '/../../..' . '/core/Service/LoginFlowV2Service.php', From 095389d71ab4b56efbbf509da116f43f5a29d82d Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 5 Sep 2024 19:15:55 +0200 Subject: [PATCH 3/3] fix: missing use statements Signed-off-by: Arthur Schiwon --- core/Migrations/Version28000Date20240828142927.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/core/Migrations/Version28000Date20240828142927.php b/core/Migrations/Version28000Date20240828142927.php index 9c0daff7a3b..4b644fdd4bb 100644 --- a/core/Migrations/Version28000Date20240828142927.php +++ b/core/Migrations/Version28000Date20240828142927.php @@ -13,6 +13,8 @@ use Closure; use OCP\DB\ISchemaWrapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; /** * Migrate the argument_hash column of oc_jobs to use sha256 instead of md5.