From 72b96404d025df2f508ee12ef2b9201aa11889a2 Mon Sep 17 00:00:00 2001 From: blaipr Date: Tue, 18 Aug 2026 09:22:16 +0200 Subject: [PATCH] Count every attempt against the temporary master password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fifty-attempt cap could be defeated by guessing in parallel: $attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS); ... $this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1)); The new value is worked out in PHP from a number read a moment earlier and written back absolute, so guesses arriving together all read the same count and all write the same count back. Fifty of them move the counter by one. This is the credential an administrator issues so somebody can re-key their vault, and it is reachable through login. A per-address tracker also applies, but it keys on REMOTE_ADDR — this global cap is the one meant to hold when the guessing comes from many places at once, and it was the one that did not. `ConfigService::incrementIfBelow()` does the arithmetic and the comparison in one statement and reports whether there was an attempt left; `checkKey()` expires the password when there was not. It was the only persisted counter doing its arithmetic in PHP — `Account.countView`, `countDecrypt` and `PublicLink.countViews` all increment in SQL. Two details in that statement, both of which bit: `CAST(… AS UNSIGNED)` cannot be used. Aura's quoter takes whatever follows AS, so the expression is emitted as CAST(… AS `UNSIGNED) + 1` and will not parse — while the same SQL run by hand is fine, which makes it read as a database problem rather than a builder one. COALESCE(`value`, '0') + 1 does the same arithmetic without the keyword. `+ 0` on the column side keeps the comparison numeric. `Config.value` is a varchar, and two strings compare as text: '10' < '3' is true, so a counter would pass a limit of 3 forever once it reached 10. The limit binds as PDO::PARAM_INT today, which settles it on its own; this makes it not depend on that, and the test says so — either mechanism alone leaves it passing, and it fails when both go. The existing unit test expected save('tempmaster_attempts', $attempts + 1), the absolute write that was the defect. It now expects the atomic call, with expects(never()) on save. CLAUDE.md gains the two SQL gotchas above; that editing src while a suite runs invalidates the run and it comes back green; and ConfigBackup::configToJson()'s unrestricted Serde::deserialize() under "audited, do NOT fix", with the reason restricting it was reverted. --- CLAUDE.md | 28 ++ .../Config/Ports/ConfigService.php | 11 + src/Application/Config/Services/Config.php | 15 + .../Crypt/Services/TemporaryMasterPass.php | 13 +- src/Domain/Config/Ports/ConfigRepository.php | 13 + .../Out/Config/Repositories/Config.php | 41 +++ .../Config/AttemptCountingTest.php | 288 ++++++++++++++++++ .../Services/TemporaryMasterPassTest.php | 54 +++- 8 files changed, 460 insertions(+), 3 deletions(-) create mode 100644 tests/Integration/Application/Config/AttemptCountingTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 9570cc602..e810487c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -220,6 +220,13 @@ A few harness details bite when writing an integration test against a real branc - **Two test processes sharing the fixture database produce failures that are not yours.** A red integration run while an agent or another shell is mid-run is contention until proven otherwise — re-run it alone before believing it. +- **Editing `src` while a suite is running invalidates the run, and it comes back green.** PHP loads + each class the first time something asks for it, so tests that ran before the edit used the old + code and everything after it used the new: the result describes neither version. Unlike the + contention above, there is no red to notice — an unrelated change can be "verified" by a run that + never executed it. Let the run finish, or kill it (inside the container — see the gotcha above) + and start again. `git stash push -- ` is how to get one change verified + on its own when two of them are in the tree. - **Faker's `randomNumber($n)` includes zero**, and forms read a zero id as "not given". A fixture drawing a group or profile id that way fails about one run in a hundred, on CI, in whichever pull request happened to be open. Use `numberBetween(1, …)`. @@ -301,6 +308,18 @@ Every action `Bootstrap` invokes **must** return `SP\Domain\Common\Dtos\ActionRe - Repos build SQL with **Aura.SqlQuery** via `$this->queryFactory`. `->set($col, $rawExpr)` injects a **RAW, unquoted** expression (`'NOW()'`, `0`, `"''"` for an empty string — *not* `''`, which yields invalid SQL). +- **"Raw" still goes past Aura's identifier quoter, which quotes whatever follows `AS`.** + `CAST(COALESCE(\`value\`, '0') AS UNSIGNED) + 1` is emitted as + ``CAST(COALESCE(`value`, '0') AS `UNSIGNED) + 1` `` — the rest of the expression is swallowed into + a quoted identifier and the statement will not parse. The SQL is valid when run by hand, so it + reads as a database problem rather than a builder one; dump `$query->getStatement()` before + believing either. Write the expression without `AS`: `COALESCE(\`value\`, '0') + 1` casts just as + well for arithmetic. +- **A numeric comparison against a varchar column needs the column side forced numeric.** `Config` + stores everything as text, so `value < :limit` compares as text when both sides are strings, and + `'10' < '3'` is true — a counter would pass a limit of 3 forever once it reached 10. `Database` + binds an int as `PDO::PARAM_INT`, which settles it, but `+ 0` on the column makes it independent + of how the value happens to arrive. - **`Model::toArray()` includes relation/non-column properties** (e.g. `UserGroup::$users`) — exclude them from insert `cols` or you get *"Unknown column"*. - A model property left **null** is inserted as `NULL` and **overrides a column's schema DEFAULT** — @@ -436,6 +455,15 @@ says. The unit suite mocks the cache and passed twice while the application was `CONFIG_BACKUP_RUN` tokens. Do not "harden" it by confining the path to `Path::BACKUP` — that breaks the documented, tested feature. (That an admin could target a web-accessible directory is operational guidance, not a code bug.) +- **`ConfigBackup::configToJson()` calls `Serde::deserialize()` without naming a class, and that is + where it stays.** Restricting it to `ConfigData::class` was tried and reverted: a sysPass 3.2 + backup holds `O:20:"SP\Config\ConfigData"`, a class this rewrite does not have, and the `is_a()` + arm throws before the `__PHP_Incomplete_Class` arm can deal with it — so the restriction turns + reading an old backup into a fatal error. The path that actually applies a backup, `restore()`, + **is** restricted, and this one only deserializes in order to re-serialize to JSON, over the + `config_backup` row the application itself wrote. Every other `unserialize()` in `src` passes + `allowed_classes`, and every other `Serde::deserialize()` names what it expects; this is the one + exception and it is deliberate. - **`jquery-ui` is in `package-lock.json` but not in `package.json` — not drift.** It is an `optionalDependencies` entry of `@selectize/selectize` (drag_drop plugin support), locked like any transitive dep (`npm ls jquery-ui` shows the chain; a fresh `npm install` keeps it). It is diff --git a/src/Application/Config/Ports/ConfigService.php b/src/Application/Config/Ports/ConfigService.php index 8c41746b4..1f121bc25 100644 --- a/src/Application/Config/Ports/ConfigService.php +++ b/src/Application/Config/Ports/ConfigService.php @@ -58,5 +58,16 @@ public function saveBatch(ConfigRequest $configRequest): void; * @throws ConstraintException * @throws QueryException */ + /** + * Counts one against a numeric parameter, and says whether there was room for it. + * + * The increment and the limit check are one statement in the repository, so requests arriving + * together each count, rather than all writing back the same number they all read. + * + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): bool; + public function save(string $param, string $value): bool; } diff --git a/src/Application/Config/Services/Config.php b/src/Application/Config/Services/Config.php index 88740d799..6bd09bef7 100644 --- a/src/Application/Config/Services/Config.php +++ b/src/Application/Config/Services/Config.php @@ -120,6 +120,21 @@ function () use ($configRequest) { * @throws ConstraintException * @throws QueryException */ + /** + * Counts one against a numeric parameter, and says whether there was room for it. + * + * False means the limit had already been reached — or the parameter is not there at all, which + * for a counter that is created alongside what it guards means the same thing: nothing left to + * spend. + * + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): bool + { + return $this->configRepository->incrementIfBelow($param, $limit)->getAffectedNumRows() === 1; + } + public function save(string $param, string $value): bool { $config = new ConfigModel(['parameter' => $param, 'value' => $value]); diff --git a/src/Application/Crypt/Services/TemporaryMasterPass.php b/src/Application/Crypt/Services/TemporaryMasterPass.php index c37c9cbee..9375282e7 100644 --- a/src/Application/Crypt/Services/TemporaryMasterPass.php +++ b/src/Application/Crypt/Services/TemporaryMasterPass.php @@ -172,7 +172,18 @@ public function checkKey(string $key): bool ); if (!$isValid) { - $this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1)); + // Counting the attempt is what enforces the limit, so the counting has to be the + // thing that cannot be raced. This used to read the count above, compare it here, + // and write back `$attempts + 1` — so guesses arriving together all read the same + // number and all wrote the same number back, and fifty of them moved the counter + // by one. The per-address tracker still applied, but this is the limit that is + // supposed to hold when the guesses come from everywhere at once, and it did not. + // + // The check above stays as it is: it costs nothing, and it is not what enforces + // anything now. + if (!$this->configService->incrementIfBelow(self::PARAM_ATTEMPTS, self::MAX_ATTEMPTS)) { + $this->expire(); + } } return $isValid; diff --git a/src/Domain/Config/Ports/ConfigRepository.php b/src/Domain/Config/Ports/ConfigRepository.php index bc0578331..d92ef2a1b 100644 --- a/src/Domain/Config/Ports/ConfigRepository.php +++ b/src/Domain/Config/Ports/ConfigRepository.php @@ -39,6 +39,19 @@ */ interface ConfigRepository extends Repository { + /** + * Counts one against a numeric parameter, unless it has already reached the limit. + * + * The increment and the comparison are one statement, so an attempt cannot be lost to another + * request reading the same number at the same moment. + * + * @return QueryResult with one row affected when the attempt was counted, and none + * when the parameter is missing or already at the limit + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): QueryResult; + /** * @param ConfigModel $config * diff --git a/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php b/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php index f895f2fd0..794f52a27 100644 --- a/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php +++ b/src/Infrastructure/Adapter/Out/Config/Repositories/Config.php @@ -46,6 +46,47 @@ final class Config extends BaseRepository implements ConfigRepository { public const TABLE = 'Config'; + /** + * Counts one against a numeric parameter, unless it has already reached the limit. + * + * The server does the arithmetic and the comparison together. Read it, compare it in PHP and + * write back `$value + 1` — which is what counting a failed temporary-password attempt used to + * do — and requests arriving together all read the same number and all write the same number + * back, so fifty attempts advance the counter by one. A limit counted that way is not a limit. + * + * `COALESCE` because `value` is nullable, and a NULL would make both the sum and the + * comparison NULL: the parameter would stop counting rather than start at zero. + * + * The `+ 0` keeps the comparison numeric whoever asks. `Config.value` is a varchar, so if both + * sides arrive as strings the server compares them as text and `'10' < '3'` is true — a + * counter would sail past its limit the moment it reached double figures, which is where a + * limit of fifty starts to matter. Today the right-hand side is an integer and `Database` + * binds it `PDO::PARAM_INT`, which settles it on its own; this makes it not depend on that. + * + * Written without `CAST(… AS UNSIGNED)` on purpose: Aura quotes whatever follows `AS` in a raw + * expression, so that becomes ``CAST(… AS `UNSIGNED) + 1` `` and the statement will not parse. + * + * @return QueryResult with one row affected when the attempt was counted, and none + * when the parameter is missing or already at the limit + * @throws ConstraintException + * @throws QueryException + */ + public function incrementIfBelow(string $param, int $limit): QueryResult + { + $query = $this->queryFactory + ->newUpdate() + ->table(self::TABLE) + ->set('value', 'COALESCE(`value`, \'0\') + 1') + // No LIMIT: `parameter` is the primary key, so at most one row can match anyway. + ->where('parameter = :parameter') + ->where('COALESCE(`value`, \'0\') + 0 < :limit') + ->bindValues(['parameter' => $param, 'limit' => $limit]); + + $queryData = QueryData::build($query)->setOnErrorMessage(__u('Error while updating the config parameter')); + + return $this->db->runQuery($queryData); + } + /** * @param ConfigModel $config * diff --git a/tests/Integration/Application/Config/AttemptCountingTest.php b/tests/Integration/Application/Config/AttemptCountingTest.php new file mode 100644 index 000000000..65619dc09 --- /dev/null +++ b/tests/Integration/Application/Config/AttemptCountingTest.php @@ -0,0 +1,288 @@ +. + */ + +namespace SP\Tests\Integration\Application\Config; + +use DI\ContainerBuilder; +use PDO; +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use SP\Application\Config\Ports\ConfigService; +use SP\Domain\Core\Bootstrap\Path; +use SP\Domain\Database\Ports\DbStorageHandler; +use SP\Domain\File\FileSystem; +use SP\Infrastructure\Definitions\CoreDefinitions; +use SP\Infrastructure\Definitions\DomainDefinitions; +use SP\Tests\Support\DatabaseTrait; + +use function SP\Tests\getDbHandler; +use function SP\Tests\getResource; + +/** + * The counter behind a limit has to be kept by the server. + * + * `TemporaryMasterPass::checkKey()` counted a failed guess by reading `tempmaster_attempts`, + * comparing it in PHP, and writing back `$attempts + 1`: + * + * ```php + * $attempts = (int)$this->configService->getByParam(self::PARAM_ATTEMPTS); + * ... + * $this->configService->save(self::PARAM_ATTEMPTS, (string)($attempts + 1)); + * ``` + * + * Guesses arriving together all read the same number and all write the same number back, so fifty + * of them move the counter by one. The temporary master password is what an administrator issues + * to let somebody re-key their vault, and its fifty-attempt cap is the limit that is supposed to + * hold when the guessing comes from many places at once — which is exactly the case the + * per-address tracker does not cover. + * + * These run against a real database, because the property being asserted is that the *server* + * does the arithmetic: a mocked repository would count however the test told it to. + */ +#[Group('integration')] +final class AttemptCountingTest extends TestCase +{ + use DatabaseTrait; + + private const PARAM = 'tempmaster_attempts'; + private const LIMIT = 3; + + private string $root; + private string $configPath; + private PDO $pdo; + + protected function setUp(): void + { + parent::setUp(); + + self::loadFixtures(); + + $this->root = FileSystem::buildPath( + sys_get_temp_dir(), + 'syspass-attempt-counting-' . bin2hex(random_bytes(6)) + ); + $this->configPath = FileSystem::buildPath($this->root, 'config'); + + foreach ([$this->configPath, $this->cachePath(), $this->tmpPath(), $this->backupPath()] as $dir) { + if (!mkdir($dir, 0777, true) && !is_dir($dir)) { + self::fail(sprintf('Directory "%s" was not created', $dir)); + } + } + + file_put_contents( + FileSystem::buildPath($this->configPath, 'config.xml'), + getResource('config', 'config.xml') + ); + + $this->pdo = getDbHandler()->getConnection(); + } + + protected function tearDown(): void + { + FileSystem::rmdirRecursive($this->root); + + parent::tearDown(); + } + + /** + * Every attempt counts, and the count is exactly how many were made. + * + * Nothing here tells the server what the new number should be — that is the point. The caller + * that worked it out from a number it had read was the caller that could be raced. + */ + #[Test] + public function eachAttemptAdvancesTheCounterByOne(): void + { + $configService = $this->configService(); + $this->givenTheCounterIs(0); + + for ($attempt = 1; $attempt <= self::LIMIT; $attempt++) { + self::assertTrue( + $configService->incrementIfBelow(self::PARAM, self::LIMIT), + sprintf('attempt %d is within the limit and must be counted', $attempt) + ); + + self::assertSame($attempt, $this->counter(), 'the counter must record every attempt'); + } + } + + /** + * Once the limit is reached nothing further is counted, and the counter does not run past it. + */ + #[Test] + public function anAttemptPastTheLimitIsRefusedAndChangesNothing(): void + { + $configService = $this->configService(); + $this->givenTheCounterIs(self::LIMIT); + + self::assertFalse($configService->incrementIfBelow(self::PARAM, self::LIMIT)); + self::assertSame(self::LIMIT, $this->counter(), 'a refused attempt must not move the counter'); + } + + /** + * The limit is a number, not a piece of text. + * + * `Config.value` is a varchar, so if both sides arrive as strings the server compares them as + * text and `'10' < '3'` is true — a counter would sail past its limit the moment it reached + * double figures, which is where a limit of fifty starts to matter. Ten against three is the + * smallest case that tells the two comparisons apart. + * + * Two things keep it numeric — the limit binds as `PDO::PARAM_INT`, and the column side is + * forced with `+ 0` — so removing either alone leaves this passing. It fails when both go, + * which is the state the behaviour actually depends on. + */ + #[Test] + public function aCounterInDoubleFiguresIsPastASingleFigureLimit(): void + { + $this->givenTheCounterIs(10); + + self::assertFalse( + $this->configService()->incrementIfBelow(self::PARAM, 3), + '10 is not below 3, however the two are spelled' + ); + + self::assertSame(10, $this->counter()); + } + + /** + * A counter that has never been written starts from nothing rather than stopping. + * + * `Config.value` is nullable, and a NULL on either side of the comparison would answer NULL — + * which reads as "at the limit", so the parameter would quietly stop counting. + */ + #[Test] + public function aCounterWithNoValueYetStillCounts(): void + { + $this->givenTheCounterIsNull(); + + self::assertTrue($this->configService()->incrementIfBelow(self::PARAM, self::LIMIT)); + self::assertSame(1, $this->counter()); + } + + /** + * A parameter that is not there counts nothing and reports as much, rather than creating one. + * + * The counter is written when the temporary password is issued, so its absence means there is + * nothing to guess at. + */ + #[Test] + public function aCounterThatDoesNotExistIsNotCreated(): void + { + $this->pdo->prepare('DELETE FROM `Config` WHERE `parameter` = :parameter') + ->execute(['parameter' => self::PARAM]); + + self::assertFalse($this->configService()->incrementIfBelow(self::PARAM, self::LIMIT)); + + $statement = $this->pdo->prepare('SELECT COUNT(*) FROM `Config` WHERE `parameter` = :parameter'); + $statement->execute(['parameter' => self::PARAM]); + + self::assertSame(0, (int)$statement->fetchColumn(), 'counting must not conjure the parameter'); + } + + private function givenTheCounterIs(int $value): void + { + $this->writeCounter((string)$value); + } + + private function givenTheCounterIsNull(): void + { + $this->writeCounter(null); + } + + private function writeCounter(?string $value): void + { + $this->pdo->prepare('DELETE FROM `Config` WHERE `parameter` = :parameter') + ->execute(['parameter' => self::PARAM]); + + $this->pdo->prepare('INSERT INTO `Config` (`parameter`, `value`) VALUES (:parameter, :value)') + ->execute(['parameter' => self::PARAM, 'value' => $value]); + } + + private function counter(): int + { + $statement = $this->pdo->prepare('SELECT `value` FROM `Config` WHERE `parameter` = :parameter'); + $statement->execute(['parameter' => self::PARAM]); + + return (int)$statement->fetchColumn(); + } + + private function configService(): ConfigService + { + return $this->buildContainer()->get(ConfigService::class); + } + + private function buildContainer(): ContainerInterface + { + $_ENV['CONFIG_PATH'] = $this->configPath; + + try { + $coreDefinitions = CoreDefinitions::getDefinitions(REAL_APP_ROOT, 'cli'); + } finally { + unset($_ENV['CONFIG_PATH']); + } + + $coreDefinitions['paths'] = array_map( + fn(array $path) => match ($path[0]) { + Path::CACHE => [Path::CACHE, $this->cachePath()], + Path::TMP => [Path::TMP, $this->tmpPath()], + Path::BACKUP => [Path::BACKUP, $this->backupPath()], + default => $path, + }, + $coreDefinitions['paths'] + ); + + $moduleDefinitions = FileSystem::require( + FileSystem::buildPath(REAL_APP_ROOT, 'src', 'Infrastructure', 'Adapter', 'In', 'Cli', 'module.php') + ); + + $builder = new ContainerBuilder(); + $builder->addDefinitions( + DomainDefinitions::getDefinitions(), + $coreDefinitions, + $moduleDefinitions, + [DbStorageHandler::class => getDbHandler()] + ); + + return $builder->build(); + } + + private function cachePath(): string + { + return FileSystem::buildPath($this->root, 'cache'); + } + + private function tmpPath(): string + { + return FileSystem::buildPath($this->root, 'tmp'); + } + + private function backupPath(): string + { + return FileSystem::buildPath($this->root, 'backup'); + } +} diff --git a/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php b/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php index 76e28dd7e..8db9ce290 100644 --- a/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php +++ b/tests/Unit/Application/Crypt/Services/TemporaryMasterPassTest.php @@ -236,10 +236,60 @@ public function testCheckTempMasterPassWithWrongKey() $hash ); + // The attempt is counted by the server, in one statement that also carries the limit. + // This used to expect save('tempmaster_attempts', $attempts + 1) — an absolute value + // worked out here from a number read a moment earlier, which is exactly what let + // simultaneous guesses all write back the same count. $this->configService ->expects(self::once()) - ->method('save') - ->with('tempmaster_attempts', $attempts + 1); + ->method('incrementIfBelow') + ->with('tempmaster_attempts', TemporaryMasterPass::MAX_ATTEMPTS) + ->willReturn(true); + + $this->configService + ->expects(self::never()) + ->method('save'); + + self::assertFalse($this->temporaryMasterPass->checkKey($pass)); + } + + /** + * A wrong key that takes the count to the limit expires the temporary password there and then. + * + * Whether there was an attempt left is the answer to the counting, not something read before + * it, so this is the moment the last one is spent — and the password has to stop working + * without waiting for another request to notice. + * + * @throws ServiceException + */ + public function testCheckTempMasterPassExpiresItWhenTheLastAttemptIsSpent() + { + $now = time(); + $pass = self::$faker->password(); + $hash = password_hash(self::$faker->sha1(), PASSWORD_BCRYPT); + + $this->configService + ->method('getByParam') + ->willReturn((string)($now + 3600), (string)$now, '49', $hash); + + $this->configService + ->expects(self::once()) + ->method('incrementIfBelow') + ->with('tempmaster_attempts', TemporaryMasterPass::MAX_ATTEMPTS) + ->willReturn(false); + + $configRequest = new ConfigRequest(); + $configRequest->add('tempmaster_pass', ''); + $configRequest->add('tempmaster_passkey', ''); + $configRequest->add('tempmaster_passhash', ''); + $configRequest->add('tempmaster_passtime', '0'); + $configRequest->add('tempmaster_maxtime', '0'); + $configRequest->add('tempmaster_attempts', '0'); + + $this->configService + ->expects(self::once()) + ->method('saveBatch') + ->with($configRequest); self::assertFalse($this->temporaryMasterPass->checkKey($pass)); }