diff --git a/apps/federation/lib/BackgroundJob/GetSharedSecret.php b/apps/federation/lib/BackgroundJob/GetSharedSecret.php index e8b8a9b40dea..9a67d1e969bf 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/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php index 548bcde27e9e..766b724b7e82 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 02eb9563eb41..b37de7246a98 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 */ diff --git a/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php b/apps/federation/tests/BackgroundJob/GetSharedSecretTest.php index 036ece63d8ab..bd6928d0a3c1 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 new file mode 100644 index 000000000000..b4aff3142a9d --- /dev/null +++ b/changelog/unreleased/41578 @@ -0,0 +1,15 @@ +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. + +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