From 016b597ce4ec412772b65f09b129ee8ab470a335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 14:23:32 +0200 Subject: [PATCH 1/3] fix(federation): remove plaintext auth token from error log in getSharedSecret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When getSharedSecret received an invalid token it logged both the attacker-supplied token and the expected valid token in plaintext: 'got "X" but expected "Y"'. Since the endpoint is @PublicPage, any unauthenticated caller could trigger this log entry at will for any trusted server URL, potentially exposing the valid federation token to anyone with log-read access. Remove the dbHandler->getToken() call that existed solely to populate the log message, and replace the message with a non-disclosing string. Signed-off-by: Thomas Müller Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- .../lib/Controller/OCSAuthAPIController.php | 3 +- apps/federation/tests/API/OCSAuthAPITest.php | 34 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php index 548bcde27e9..766b724b7e8 100644 --- a/apps/federation/lib/Controller/OCSAuthAPIController.php +++ b/apps/federation/lib/Controller/OCSAuthAPIController.php @@ -159,9 +159,8 @@ public function getSharedSecret($url, $token) { } if ($this->isValidToken($url, $token) === false) { - $expectedToken = $this->dbHandler->getToken($url); $this->logger->error( - 'remote server (' . $url . ') didn\'t send a valid token (got "' . $token . '" but expected "'. $expectedToken . '") while getting shared secret', + 'remote server (' . $url . ') didn\'t send a valid token while getting shared secret', ['app' => 'federation'] ); return ['statuscode' => Http::STATUS_FORBIDDEN]; diff --git a/apps/federation/tests/API/OCSAuthAPITest.php b/apps/federation/tests/API/OCSAuthAPITest.php index 02eb9563eb4..b37de7246a9 100644 --- a/apps/federation/tests/API/OCSAuthAPITest.php +++ b/apps/federation/tests/API/OCSAuthAPITest.php @@ -235,6 +235,40 @@ public function dataTestGetSharedSecret() { ]; } + /** + * Verify that when an invalid token is supplied, the error log message does NOT + * contain either the attacker-supplied token or the expected stored token. + * Leaking either value allows an attacker with log-read access to replay the + * valid token and obtain the shared secret (CVE-class: information disclosure). + */ + public function testGetSharedSecretDoesNotLogTokenValues() { + $url = 'https://trusted.example.com'; + $attackerToken = 'attacker_supplied_token_XYZ'; + $storedToken = 'super_secret_expected_token_ABC'; + + $this->trustedServers + ->method('isTrustedServer')->with($url)->willReturn(true); + + // dbHandler->getToken is used by isValidToken; return the stored token + $this->dbHandler + ->method('getToken')->with($url)->willReturn($storedToken); + + // Capture every call to logger->error and assert no token values appear + $this->logger + ->expects($this->once()) + ->method('error') + ->with( + $this->logicalAnd( + $this->logicalNot($this->stringContains($attackerToken)), + $this->logicalNot($this->stringContains($storedToken)) + ), + $this->anything() + ); + + $result = $this->ocsAuthApi->getSharedSecret($url, $attackerToken); + $this->assertSame(Http::STATUS_FORBIDDEN, $result['statuscode']); + } + /** * @dataProvider dataTestIsTokenValid */ From 135d3a045ffa334afffd9e8cb8cdb8c3a17b09ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Fri, 5 Jun 2026 15:58:57 +0200 Subject: [PATCH 2/3] chore: add changelog entry for #41578 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- changelog/unreleased/41578 | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 changelog/unreleased/41578 diff --git a/changelog/unreleased/41578 b/changelog/unreleased/41578 new file mode 100644 index 00000000000..5bca8c0d617 --- /dev/null +++ b/changelog/unreleased/41578 @@ -0,0 +1,10 @@ +Security: Remove plaintext federation auth token from error log + +When getSharedSecret received an invalid token it logged both the +submitted value and the expected valid token in plaintext. Since the +endpoint is public, any unauthenticated caller could trigger this log +entry at will for any trusted server URL, exposing the valid token to +anyone with log-read access. The token values have been removed from +the log message. + +https://github.com/owncloud/core/pull/41578 From c4bdd06ca9e6599e5cb1affe9bf6654e93aa3f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <1005065+DeepDiver1975@users.noreply.github.com> Date: Tue, 21 Jul 2026 12:59:56 +0200 Subject: [PATCH 3/3] fix(federation): scrub token from GetSharedSecret exception log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GetSharedSecret background job sends the federation token as a GET query parameter. On any non-403 HTTP failure it called logger->logException($e), and the Guzzle exception message embeds the full request URI including "?token=...", leaking the plaintext token into the log - the same class of leak the PR's controller fix closed, but on a second, untouched code path (addresses review by @dj4oC). Replace both logException() calls with a controlled error message that records only the target host and HTTP status. Add a regression test that throws a realistic ClientException whose URI carries the token and asserts the value never reaches the logger. Correct the changelog, which previously overclaimed that the token had been fully removed from logs. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --- .../lib/BackgroundJob/GetSharedSecret.php | 14 +++++- .../BackgroundJob/GetSharedSecretTest.php | 50 +++++++++++++++++++ changelog/unreleased/41578 | 9 +++- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php index e8b8a9b40de..9a67d1e969b 100644 --- a/apps/federation/lib/BackgroundJob/GetSharedSecret.php +++ b/apps/federation/lib/BackgroundJob/GetSharedSecret.php @@ -159,11 +159,21 @@ protected function run($argument) { if ($status === Http::STATUS_FORBIDDEN) { $this->logger->info($target . ' refused to exchange a shared secret with you.', ['app' => 'federation']); } else { - $this->logger->logException($e, ['app' => 'federation']); + // NB: do not log the exception verbatim - the token is sent as a + // GET query parameter, so the Guzzle message embeds the full + // request URI including "?token=...", leaking the plaintext token. + $this->logger->error( + 'Could not exchange a shared secret with ' . $target . ' (HTTP ' . $status . ')', + ['app' => 'federation'] + ); } } catch (\Exception $e) { $status = Http::STATUS_INTERNAL_SERVER_ERROR; - $this->logger->logException($e, ['app' => 'federation']); + // NB: see above - the token must not reach the log via the request URI. + $this->logger->error( + 'Could not exchange a shared secret with ' . $target, + ['app' => 'federation'] + ); } // if we received a unexpected response we try again later diff --git a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php index 036ece63d8a..bd6928d0a3c 100644 --- a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php +++ b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php @@ -23,6 +23,9 @@ namespace OCA\Federation\Tests\BackgroundJob; +use GuzzleHttp\Exception\ClientException; +use GuzzleHttp\Psr7\Request; +use GuzzleHttp\Psr7\Response; use OCA\Federation\BackgroundJob\GetSharedSecret; use OCA\Federation\DbHandler; use OCA\Federation\TrustedServers; @@ -203,4 +206,51 @@ public function dataTestRun() { [Http::STATUS_CONFLICT], ]; } + + /** + * On a non-403 HTTP failure the shared secret is sent as a GET query + * parameter, so the Guzzle exception message embeds the full request URI + * including "?token=...". Logging that exception verbatim would leak the + * plaintext token, so the token value must never reach the logger. + */ + public function testRunDoesNotLogTokenOnClientException() { + $target = 'https://remote.example.com'; + $source = 'https://local.example.com'; + $token = 'super-secret-federation-token'; + + $argument = ['url' => $target, 'token' => $token]; + + $this->urlGenerator->expects($this->once())->method('getAbsoluteURL')->with('/') + ->willReturn($source); + + // Build a real Guzzle ClientException whose request URI carries the + // token in the query string, exactly as the live client would produce. + $request = new Request( + 'GET', + $target . '/ocs/v2.php/apps/federation/api/v1/shared-secret?url=' . $source . '&token=' . $token . '&format=json' + ); + $exception = new ClientException( + 'Client error: `' . $request->getUri() . '` resulted in a `409 Conflict` response', + $request, + new Response(Http::STATUS_CONFLICT) + ); + + $this->httpClient->expects($this->once())->method('get') + ->willThrowException($exception); + + // The token must not appear in any exception logged verbatim... + $this->logger->expects($this->never())->method('logException'); + // ...nor in any error/info message string. + $assertNoToken = function ($message) use ($token) { + $this->assertStringNotContainsString($token, $message); + return true; + }; + $this->logger->method('error')->with($this->callback($assertNoToken)); + $this->logger->method('info')->with($this->callback($assertNoToken)); + + self::invokePrivate($this->getSharedSecret, 'run', [$argument]); + + // A 409 is an unexpected response, so the job is retained for retry. + $this->assertTrue(self::invokePrivate($this->getSharedSecret, 'retainJob')); + } } diff --git a/changelog/unreleased/41578 b/changelog/unreleased/41578 index 5bca8c0d617..b4aff3142a9 100644 --- a/changelog/unreleased/41578 +++ b/changelog/unreleased/41578 @@ -4,7 +4,12 @@ When getSharedSecret received an invalid token it logged both the submitted value and the expected valid token in plaintext. Since the endpoint is public, any unauthenticated caller could trigger this log entry at will for any trusted server URL, exposing the valid token to -anyone with log-read access. The token values have been removed from -the log message. +anyone with log-read access. + +A second leak in the same code path has also been closed: the +GetSharedSecret background job sends the token as a GET query +parameter, so on an unexpected HTTP response the Guzzle exception - +whose message embeds the full request URI including "?token=..." - +was logged verbatim. Both log sites no longer emit the token value. https://github.com/owncloud/core/pull/41578