Skip to content

Commit bfdf515

Browse files
DeepDiver1975DeepDiver1975
authored
fix(federation): replace strcmp token oracle with hash-based comparison in requestSharedSecret (#41579)
* fix(federation): replace strcmp token oracle with hash-based comparison The requestSharedSecret endpoint (@publicpage, unauthenticated) used strcmp() to compare the caller-supplied token against the stored local token, returning 403 when localToken > submitted_token and 200 otherwise. This binary oracle allows an attacker to binary-search the stored token in ~96 requests and then use it to obtain the federation shared secret. Replace strcmp($a, $b) with strcmp(hash("sha256",$a), hash("sha256",$b)). The deterministic tiebreaking property is preserved while the response reveals nothing about the plaintext token value. Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> * chore: add changelog entry for #41579 Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com> --------- Signed-off-by: Thomas Müller <thomas.mueller@owncloud.com> Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
1 parent aea1c28 commit bfdf515

3 files changed

Lines changed: 73 additions & 5 deletions

File tree

‎apps/federation/lib/Controller/OCSAuthAPIController.php‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,15 @@ public function requestSharedSecret($url, $token) {
105105
}
106106

107107
// if both server initiated the exchange of the shared secret the greater
108-
// token wins
108+
// token wins.
109+
// Compare hashes of the tokens to prevent an oracle attack: comparing
110+
// raw token strings via strcmp() would leak ordering information about
111+
// the stored localToken to unauthenticated callers (binary search oracle).
112+
// Hashing both values with SHA-256 before comparison preserves the
113+
// deterministic tie-breaking property while revealing nothing about the
114+
// plaintext localToken value.
109115
$localToken = $this->dbHandler->getToken($url);
110-
if (\strcmp($localToken, $token) > 0) {
116+
if (\strcmp(\hash('sha256', $localToken), \hash('sha256', $token)) > 0) {
111117
$this->logger->info(
112118
'remote server (' . $url . ') presented lower token. We will initiate the exchange of the shared secret.',
113119
['app' => 'federation']

‎apps/federation/tests/API/OCSAuthAPITest.php‎

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,13 +112,65 @@ public function testRequestSharedSecret($token, $localToken, $isTrustedServer, $
112112
}
113113

114114
public function dataTestRequestSharedSecret() {
115+
// Token pairs are chosen so that hash('sha256', $token) vs hash('sha256', $localToken)
116+
// ordering matches the expected response. Using raw strcmp() on the tokens
117+
// would give different results for the first two cases, which is exactly the
118+
// behaviour that the oracle-prevention fix changes.
119+
//
120+
// Case 1: hash(token) > hash(localToken) => remote wins, we schedule GetSharedSecret => 200
121+
// token=0eihx/zCxwyV localToken=hgSXvmcFnOp+
122+
// strcmp(token, localToken) < 0 (old code would return 403, NEW code returns 200)
123+
//
124+
// Case 2: hash(token) < hash(localToken) => we win, remote backs off => 403
125+
// token=Dvu0e+f+cC5F localToken=FBv96rzfQZB0
126+
// strcmp(token, localToken) < 0 (both old and new code agree here)
127+
//
128+
// Case 3: server not trusted => always 403 regardless of tokens
115129
return [
116-
['token2', 'token1', true, Http::STATUS_OK],
117-
['token1', 'token2', false, Http::STATUS_FORBIDDEN],
118-
['token1', 'token2', true, Http::STATUS_FORBIDDEN],
130+
['0eihx/zCxwyV', 'hgSXvmcFnOp+', true, Http::STATUS_OK],
131+
['Dvu0e+f+cC5F', 'FBv96rzfQZB0', false, Http::STATUS_FORBIDDEN],
132+
['Dvu0e+f+cC5F', 'FBv96rzfQZB0', true, Http::STATUS_FORBIDDEN],
119133
];
120134
}
121135

136+
/**
137+
* Regression test: the old strcmp()-based comparison leaked ordering information
138+
* about the stored localToken to unauthenticated callers (binary search oracle).
139+
*
140+
* This test uses a concrete token pair where strcmp(localToken, token) > 0
141+
* (old code would return 403) but strcmp(hash(localToken), hash(token)) <= 0
142+
* (new code returns 200), demonstrating that the fix changes the behaviour
143+
* for cases that would have been exploitable as an oracle while still
144+
* producing the correct deterministic tiebreak result.
145+
*
146+
* Without the fix this test would return STATUS_FORBIDDEN for a case where
147+
* the hash-based comparison says the remote server's token wins.
148+
*/
149+
public function testRequestSharedSecretNoOracleLeakage() {
150+
// strcmp('DD7EOAhPjsziDBGa', '/Ewru/vr674cUicB') = 21 (localToken > token, old code: 403)
151+
// strcmp(hash256('DD7EOAhPjsziDBGa'), hash256('/Ewru/vr674cUicB')) = -48 (new code: 200)
152+
$url = 'https://remote.example.com';
153+
$localToken = 'DD7EOAhPjsziDBGa';
154+
$token = '/Ewru/vr674cUicB';
155+
156+
$this->trustedServers
157+
->expects($this->once())
158+
->method('isTrustedServer')->with($url)->willReturn(true);
159+
$this->dbHandler
160+
->expects($this->any())
161+
->method('getToken')->with($url)->willReturn($localToken);
162+
163+
// With the fix the hash of $token > hash of $localToken, so the remote server
164+
// wins the tiebreak and we must schedule GetSharedSecret (=> 200).
165+
$this->jobList->expects($this->once())->method('add')
166+
->with('OCA\Federation\BackgroundJob\GetSharedSecret', ['url' => $url, 'token' => $token]);
167+
$this->jobList->expects($this->once())->method('remove')
168+
->with('OCA\Federation\BackgroundJob\RequestSharedSecret', ['url' => $url, 'token' => $localToken]);
169+
170+
$result = $this->ocsAuthApi->requestSharedSecret($url, $token);
171+
$this->assertSame(Http::STATUS_OK, $result['statuscode']);
172+
}
173+
122174
/**
123175
* @dataProvider dataTestGetSharedSecret
124176
*

‎changelog/unreleased/41579‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
Security: Replace strcmp token oracle with hash-based comparison in federation
2+
3+
The requestSharedSecret endpoint used strcmp() to compare caller-supplied
4+
and stored federation tokens, returning different HTTP responses based on
5+
lexicographic ordering. This allowed an unauthenticated attacker to recover
6+
the stored token via binary search in approximately 96 requests. Tokens are
7+
now compared by their SHA-256 hashes, removing the plaintext oracle while
8+
preserving the tiebreaking behaviour.
9+
10+
https://github.com/owncloud/core/pull/41579

0 commit comments

Comments
 (0)