diff --git a/apps/federation/lib/Controller/OCSAuthAPIController.php b/apps/federation/lib/Controller/OCSAuthAPIController.php index d79415f03b95..548bcde27e9e 100644 --- a/apps/federation/lib/Controller/OCSAuthAPIController.php +++ b/apps/federation/lib/Controller/OCSAuthAPIController.php @@ -105,9 +105,15 @@ public function requestSharedSecret($url, $token) { } // if both server initiated the exchange of the shared secret the greater - // token wins + // token wins. + // Compare hashes of the tokens to prevent an oracle attack: comparing + // raw token strings via strcmp() would leak ordering information about + // the stored localToken to unauthenticated callers (binary search oracle). + // Hashing both values with SHA-256 before comparison preserves the + // deterministic tie-breaking property while revealing nothing about the + // plaintext localToken value. $localToken = $this->dbHandler->getToken($url); - if (\strcmp($localToken, $token) > 0) { + if (\strcmp(\hash('sha256', $localToken), \hash('sha256', $token)) > 0) { $this->logger->info( 'remote server (' . $url . ') presented lower token. We will initiate the exchange of the shared secret.', ['app' => 'federation'] diff --git a/apps/federation/tests/API/OCSAuthAPITest.php b/apps/federation/tests/API/OCSAuthAPITest.php index e07c6ce23c3c..02eb9563eb41 100644 --- a/apps/federation/tests/API/OCSAuthAPITest.php +++ b/apps/federation/tests/API/OCSAuthAPITest.php @@ -112,13 +112,65 @@ public function testRequestSharedSecret($token, $localToken, $isTrustedServer, $ } public function dataTestRequestSharedSecret() { + // Token pairs are chosen so that hash('sha256', $token) vs hash('sha256', $localToken) + // ordering matches the expected response. Using raw strcmp() on the tokens + // would give different results for the first two cases, which is exactly the + // behaviour that the oracle-prevention fix changes. + // + // Case 1: hash(token) > hash(localToken) => remote wins, we schedule GetSharedSecret => 200 + // token=0eihx/zCxwyV localToken=hgSXvmcFnOp+ + // strcmp(token, localToken) < 0 (old code would return 403, NEW code returns 200) + // + // Case 2: hash(token) < hash(localToken) => we win, remote backs off => 403 + // token=Dvu0e+f+cC5F localToken=FBv96rzfQZB0 + // strcmp(token, localToken) < 0 (both old and new code agree here) + // + // Case 3: server not trusted => always 403 regardless of tokens return [ - ['token2', 'token1', true, Http::STATUS_OK], - ['token1', 'token2', false, Http::STATUS_FORBIDDEN], - ['token1', 'token2', true, Http::STATUS_FORBIDDEN], + ['0eihx/zCxwyV', 'hgSXvmcFnOp+', true, Http::STATUS_OK], + ['Dvu0e+f+cC5F', 'FBv96rzfQZB0', false, Http::STATUS_FORBIDDEN], + ['Dvu0e+f+cC5F', 'FBv96rzfQZB0', true, Http::STATUS_FORBIDDEN], ]; } + /** + * Regression test: the old strcmp()-based comparison leaked ordering information + * about the stored localToken to unauthenticated callers (binary search oracle). + * + * This test uses a concrete token pair where strcmp(localToken, token) > 0 + * (old code would return 403) but strcmp(hash(localToken), hash(token)) <= 0 + * (new code returns 200), demonstrating that the fix changes the behaviour + * for cases that would have been exploitable as an oracle while still + * producing the correct deterministic tiebreak result. + * + * Without the fix this test would return STATUS_FORBIDDEN for a case where + * the hash-based comparison says the remote server's token wins. + */ + public function testRequestSharedSecretNoOracleLeakage() { + // strcmp('DD7EOAhPjsziDBGa', '/Ewru/vr674cUicB') = 21 (localToken > token, old code: 403) + // strcmp(hash256('DD7EOAhPjsziDBGa'), hash256('/Ewru/vr674cUicB')) = -48 (new code: 200) + $url = 'https://remote.example.com'; + $localToken = 'DD7EOAhPjsziDBGa'; + $token = '/Ewru/vr674cUicB'; + + $this->trustedServers + ->expects($this->once()) + ->method('isTrustedServer')->with($url)->willReturn(true); + $this->dbHandler + ->expects($this->any()) + ->method('getToken')->with($url)->willReturn($localToken); + + // With the fix the hash of $token > hash of $localToken, so the remote server + // wins the tiebreak and we must schedule GetSharedSecret (=> 200). + $this->jobList->expects($this->once())->method('add') + ->with('OCA\Federation\BackgroundJob\GetSharedSecret', ['url' => $url, 'token' => $token]); + $this->jobList->expects($this->once())->method('remove') + ->with('OCA\Federation\BackgroundJob\RequestSharedSecret', ['url' => $url, 'token' => $localToken]); + + $result = $this->ocsAuthApi->requestSharedSecret($url, $token); + $this->assertSame(Http::STATUS_OK, $result['statuscode']); + } + /** * @dataProvider dataTestGetSharedSecret * diff --git a/changelog/unreleased/41579 b/changelog/unreleased/41579 new file mode 100644 index 000000000000..3469a7e7c8b7 --- /dev/null +++ b/changelog/unreleased/41579 @@ -0,0 +1,10 @@ +Security: Replace strcmp token oracle with hash-based comparison in federation + +The requestSharedSecret endpoint used strcmp() to compare caller-supplied +and stored federation tokens, returning different HTTP responses based on +lexicographic ordering. This allowed an unauthenticated attacker to recover +the stored token via binary search in approximately 96 requests. Tokens are +now compared by their SHA-256 hashes, removing the plaintext oracle while +preserving the tiebreaking behaviour. + +https://github.com/owncloud/core/pull/41579