From 1362dbda22f174c16099bccf248ee2de40cd8740 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 37fa65797c2d4..b442c2c3e933e 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -43,7 +43,6 @@ use Psr\Log\LoggerInterface; use function get_class; use function json_encode; -use function md5; use function strlen; class JobList implements IJobList { @@ -73,7 +72,7 @@ public function add($job, $argument = null, ?int $firstCheck = null): void { ->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), ]); @@ -83,7 +82,7 @@ public function add($job, $argument = null, ?int $firstCheck = null): void { ->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(); } @@ -104,7 +103,7 @@ public function remove($job, $argument = null): void { ->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 @@ -145,7 +144,7 @@ public function has($job, $argument): bool { $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 8552ac461efb584c75895a801530223916c0c599 Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 28 Aug 2024 17:46:00 +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 0000000000000..9c0daff7a3b12 --- /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 4872a042b53fd..13b04a9653112 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1274,6 +1274,7 @@ '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\\Version29000Date20231126110901' => $baseDir . '/core/Migrations/Version29000Date20231126110901.php', 'OC\\Core\\Migrations\\Version29000Date20231213104850' => $baseDir . '/core/Migrations/Version29000Date20231213104850.php', 'OC\\Core\\Migrations\\Version29000Date20240124132201' => $baseDir . '/core/Migrations/Version29000Date20240124132201.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 4dba8372083df..8b413e5bc7b8c 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1307,6 +1307,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\\Version29000Date20231126110901' => __DIR__ . '/../../..' . '/core/Migrations/Version29000Date20231126110901.php', 'OC\\Core\\Migrations\\Version29000Date20231213104850' => __DIR__ . '/../../..' . '/core/Migrations/Version29000Date20231213104850.php', 'OC\\Core\\Migrations\\Version29000Date20240124132201' => __DIR__ . '/../../..' . '/core/Migrations/Version29000Date20240124132201.php', From 0b6ec1b1f010e927716c949c09e6a738a4782364 Mon Sep 17 00:00:00 2001 From: Arthur Schiwon Date: Thu, 5 Sep 2024 19:11:06 +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 9c0daff7a3b12..4b644fdd4bb95 100644 --- a/core/Migrations/Version28000Date20240828142927.php +++ b/core/Migrations/Version28000Date20240828142927.php @@ -13,6 +13,8 @@ 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.