From 843a47f12cb11f4a347096da0700661da563621a Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 28 Aug 2024 17:03:53 +0200 Subject: [PATCH 1/2] 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 5059199a18273..dc8e33c743c0f 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -20,7 +20,6 @@ use Psr\Log\LoggerInterface; use function get_class; use function json_encode; -use function md5; use function strlen; class JobList implements IJobList { @@ -50,7 +49,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), ]); @@ -60,7 +59,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(); } @@ -81,7 +80,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 @@ -122,7 +121,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 dbf56b6c1157c78bf12a0018cdd632cfe77697cb Mon Sep 17 00:00:00 2001 From: Louis Chemineau Date: Wed, 28 Aug 2024 17:46:00 +0200 Subject: [PATCH 2/2] fix: Migrate existing bg jobs to use sha256 Signed-off-by: Louis Chemineau --- .../Version28000Date20240828142927.php | 83 +++++++++++++++++++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + 3 files changed, 85 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..e36c27add1c64 --- /dev/null +++ b/core/Migrations/Version28000Date20240828142927.php @@ -0,0 +1,83 @@ +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 b8d0dd35f123d..9710ff7dbd5ec 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1364,6 +1364,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 646a41626b7b6..a681b10aa5116 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1397,6 +1397,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',