fix(federation): replace strcmp token oracle with hash-based comparison in requestSharedSecret

[DeepDiver1975]

Summary

• requestSharedSecret (@PublicPage, unauthenticated) used strcmp($localToken, $submitted) to decide 403 vs 200, leaking token ordering to any caller
• Binary search converges on the exact 16-char stored token in ~96 requests; token can then be replayed to getSharedSecret to steal the federation shared secret
• Fix: compare SHA-256 hashes instead — ordering is preserved (tiebreaking still works) but responses are now independent of the plaintext token

Security Impact

High — unauthenticated callers can recover federation shared secrets when they know a trusted server URL

Test plan

• New regression test testRequestSharedSecretNoOracleLeakage uses a token pair where old strcmp returns 403 but hash-based returns 200; fails without fix
• Existing dataTestRequestSharedSecret updated with hash-ordered token pairs
• Run make test TEST_PHP_SUITE=apps/federation

🤖 Generated with Claude Code

[update-docs]

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

[DeepDiver1975]

Code Review — fix(federation): replace strcmp token oracle with hash-based comparison in requestSharedSecret

Overview: requestSharedSecret (@PublicPage, unauthenticated) used strcmp($localToken, $submitted) to determine which server's token wins a simultaneous federation exchange. Because different responses (200 vs 403) are returned for >0 vs <=0, an attacker can binary-search the stored $localToken in ~96 requests. The fix hashes both values with SHA-256 before comparing, so the response reveals nothing about the plaintext token.

Correctness of the fix

if (\strcmp(\hash('sha256', $localToken), \hash('sha256', $token)) > 0) {

The tiebreak semantics are preserved: both sides hash deterministically, the same side still "wins" for any given pair (assuming neither server has the other's real token), and no plaintext ordering is revealed. ✅

Is SHA-256 appropriate here? Yes — the goal is to eliminate the ordering oracle, not to provide cryptographic secrecy of the token itself. Both tokens are hashed identically; keyed HMAC would be overkill since the comparison is symmetric and the attacker doesn't get the hash value directly (only the 200/403 response). ✅

Does the tiebreak still resolve correctly? For the legitimate two-server exchange scenario, both servers hold the same $localToken and receive the other's token. After hashing, the same lexicographic comparison runs, so whichever side has the higher-hashing token still wins. ✅

Test analysis

dataTestRequestSharedSecret: Updated to use concrete token pairs where the hash-ordering matches the expected status code. The comment block explaining Case 1 vs Case 2 is unusually thorough for a test file but is genuinely useful here since the tokens are opaque — it makes the intent reviewable. ✅

testRequestSharedSecretNoOracleLeakage: This is the critical regression test. It uses a pair where strcmp(localToken, token) > 0 (old code: 403) but strcmp(hash(localToken), hash(token)) < 0 (new code: 200). This is exactly the distinguishing case that proves the fix changes behavior for the exploitable scenario. ✅

The test verifies:

• jobList->add(GetSharedSecret) is called (remote wins, we schedule fetch)
• jobList->remove(RequestSharedSecret) is called
• Status is 200 ✅

Changelog

changelog/unreleased/41579 accurately describes the oracle attack and the fix, including the "~96 requests" estimate. ✅

Summary

Aspect	Assessment
Oracle eliminated	✅ Hash comparison reveals no plaintext ordering
Tiebreak semantics	✅ Preserved
Regression test	✅ Directly demonstrates old code fails, new code passes
Data provider	✅ Updated with hash-ordered token pairs + explanatory comments
Changelog	✅ Accurate

Verdict: Ready to merge. High-impact security fix with solid test coverage.

[dj4oC]

Replaces strcmp($localToken, $token) with strcmp(hash('sha256', $localToken), hash('sha256', $token)) in OCSAuthAPIController::requestSharedSecret(), closing a binary-search oracle that let an unauthenticated caller recover the stored federation shared secret via HTTP-status-code responses (~96 requests).

Findings

• Security — CWE-203/208-class oracle correctly eliminated (OWASP A02:2021, Cryptographic Failures): independently re-derived SHA-256 hashes and strcmp signs for the PR's data-provider pairs and the dedicated oracle test; confirmed old code would return 403 for the chosen pair while the new hash-based comparison returns 200, i.e. the test demonstrates the specific divergence the fix causes, not just incidental output.
• Non-blocking hygiene note: the comparison still uses strcmp() on the two hex digests rather than hash_equals(). Not practically exploitable (SHA-256's avalanche effect means no timing signal on the digest carries information about the original tokens), but hash_equals() is the standard idiom for anything secret-adjacent and removes any doubt.
• Stability: no findings — both federation peers compute the hash deterministically, so the tie-break outcome each side agrees on is unchanged in kind, only the oracle is closed.
• Performance: negligible — two SHA-256 hashes of short strings on an infrequent federation-handshake endpoint.
• Test coverage: strong — the new test proves the fix via a concrete input pair where old vs. new code diverge.

Verdict

Approved. Optional non-blocking follow-up: swap the final strcmp() for hash_equals().

🤖 Automated review by Claude Code (security · stability · performance · coverage)

---

Generated by Claude Code

[jvillafanez]

crc32 might be a better choice instead of sha256 for this particular case because it's faster and we don't really need security properties