From a71cc6b6b761fb2bd574de7c088f16ca88a65547 Mon Sep 17 00:00:00 2001 From: blaipr Date: Thu, 17 Sep 2026 03:08:07 +0200 Subject: [PATCH] fix: a stack trace in a log carries no argument values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DatabaseHandler::update() wrote (string)$source into the Eventlog row for a Throwable, and PHP's default Exception::__toString() embeds getTraceAsString(), which prints each frame's argument values — measured, not assumed: #0 Command line code(3): decryptSecret('SuperSecretMast...', 'an-account-key') Fifteen characters of every string on the stack. The chains that throw into this sink include the crypt and database layers and the LDAP providers, so a master password, an account password or a bind credential can be an argument on the way to the throw point, and the row is readable by anyone whose profile has isEvl() and can be searched and exported. formatStackTrace() is the same trace with every argument reduced to its type, and processException() has always used it for exactly this reason — this sink was the one that did not. processException() had the same defect on its previous-exception branch, one line below where it uses the safe formatter. Two things narrow it, both worth knowing: SPException::__toString() emits no trace at all, so the application's own exception type was never the leaky one — what arrives carrying a trace is a RuntimeException, a PDOException or a library's own, precisely the set thrown from inside crypt and database calls. And all 84 Throwable-sourced notifications use the event name 'exception', which is opt-in rather than in EVENTS_FIXED. The header each exception renders for itself is kept and only the trace is replaced, so SPException logs exactly what it logged before, hint included. --- src/Infrastructure/Functions.php | 5 +- .../Log/Providers/DatabaseHandler.php | 26 +++++++- .../Log/Providers/DatabaseHandlerTest.php | 62 +++++++++++++++++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/Infrastructure/Functions.php b/src/Infrastructure/Functions.php index 2365ba670..65ba81d0e 100644 --- a/src/Infrastructure/Functions.php +++ b/src/Infrastructure/Functions.php @@ -174,7 +174,10 @@ function processException(Throwable $exception): void sprintf( "(P) %s\n%s", __($previous->getMessage()), - $previous->getTraceAsString() + // The same formatter the line above uses, and for the same reason: + // `getTraceAsString()` prints argument values, so a secret passed to any frame on + // the way to the throw point lands in the log file. + formatStackTrace($previous) ), 'EXCEPTION' ); diff --git a/src/Infrastructure/Log/Providers/DatabaseHandler.php b/src/Infrastructure/Log/Providers/DatabaseHandler.php index bc7db5381..d3314ffdc 100644 --- a/src/Infrastructure/Log/Providers/DatabaseHandler.php +++ b/src/Infrastructure/Log/Providers/DatabaseHandler.php @@ -38,6 +38,7 @@ use SP\Application\Security\Ports\EventlogService; use Throwable; +use function SP\formatStackTrace; use function SP\processException; /** @@ -90,7 +91,30 @@ public function update(Event $event): void if ($source instanceof Throwable) { $properties['level'] = 'ERROR'; - $properties['description'] = (string)$source; + + // PHP's default `Exception::__toString()` embeds `getTraceAsString()`, which prints + // each frame's **argument values** rather than their types — the first 15 characters of + // every string on the stack. The chains that throw into this sink include the crypt and + // database layers and the LDAP providers, so a master password, an account password or + // a bind credential can be an argument on the way to the throw point. This row is + // readable by anyone whose profile has `isEvl()`, and the event log can be searched and + // exported. + // + // The header is kept as each exception renders it and only the trace is replaced, with + // `formatStackTrace()` — the same trace reduced to argument *types*, which + // `processException()` has always used for exactly this reason. + // + // Worth knowing while reading this: `SPException::__toString()` overrides PHP's and + // emits no trace at all, so the application's own exception type was never the leaky + // one. What reaches here carrying a trace is a `RuntimeException`, a `PDOException`, a + // `TypeError` or a library's own — which is precisely the set that fails inside crypt + // and database calls. + $rendered = (string)$source; + [$head] = explode("\nStack trace:\n", $rendered, 2); + + $properties['description'] = $head === $rendered + ? $rendered + : sprintf("%s\n%s", $head, formatStackTrace($source)); } else { $properties['description'] = $event->getEventMessage()?->composeText(); } diff --git a/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php b/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php index 486b4a38e..0b36067d0 100644 --- a/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php +++ b/tests/Unit/Infrastructure/Log/Providers/DatabaseHandlerTest.php @@ -50,6 +50,8 @@ #[AllowMockObjectsWithoutExpectations] class DatabaseHandlerTest extends UnitaryTestCase { + private const A_SECRET = 'SuperSecretMasterPassword123'; + private MockObject|EventlogService $eventLogService; private MockObject|LanguageInterface $language; private DatabaseHandler $databaseHandler; @@ -164,6 +166,66 @@ public function testUpdateWithSPExceptionMessage() $this->databaseHandler->update($event); } + /** + * A logged exception records what went wrong, and none of the values that were on the stack. + * + * The row used to be `(string)$source`, and `Exception::__toString()` embeds + * `getTraceAsString()`, which prints each frame's **argument values** — the first 15 characters + * of every string. The chains that throw into this sink include the crypt and database layers + * and the LDAP providers, so a master password, an account password or a bind credential can be + * an argument on the way to the throw point; the row is readable by anyone whose profile has + * `isEvl()`, and the event log can be searched and exported. + * + * `formatStackTrace()` is the same trace with every argument reduced to its type, and + * `processException()` has always used it for exactly this reason. + * + * Whether a trace carries arguments at all is an ini setting that differs between a development + * build and a production one, so it is pinned here rather than assumed — `FunctionsTest` does + * the same, and without it this passes locally and proves nothing wherever the production ini + * is in force. + */ + public function testALoggedExceptionCarriesNoArgumentValues() + { + $ignoreArgs = ini_get('zend.exception_ignore_args'); + ini_set('zend.exception_ignore_args', '0'); + + $description = null; + + $this->eventLogService + ->expects($this->once()) + ->method('create') + ->willReturnCallback( + static function (Eventlog $eventlog) use (&$description): int { + $description = $eventlog->getDescription(); + + return 1; + } + ); + + try { + $throw = static function (string $masterPassword, string $accountKey): void { + throw new RuntimeException('could not decrypt'); + }; + + try { + $throw(self::A_SECRET, 'an-account-key'); + } catch (RuntimeException $e) { + $this->databaseHandler->update(new Event('test_a.update', $e)); + } + } finally { + ini_set('zend.exception_ignore_args', (string)$ignoreArgs); + } + + self::assertIsString($description); + self::assertStringNotContainsString(substr(self::A_SECRET, 0, 15), $description); + self::assertStringNotContainsString('an-account-key', $description); + + // ...and it is still an account of what happened, or withholding the arguments would have + // been achieved just as well by logging nothing. + self::assertStringContainsString('could not decrypt', $description); + self::assertStringContainsString('String', $description, 'arguments are recorded by type'); + } + /** * @throws InvalidClassException */