Fix unauthenticated TablesStatusRequest in interserver mode - #99854
Conversation
|
Workflow [PR], commit [01797bc] Summary: ✅
AI ReviewSummaryThis PR closes the table-status disclosure itself by authenticating interserver Findings
Final Verdict❌ Changes requested |
clickhouse-gh
Bot
added
pr-critical-bugfix
pr-must-backport
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
The test uses `CLICKHOUSE_DATABASE` for table isolation and operates at the TCP connection level; parallel execution poses no hazard. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@tiandiwonder The changelog entry is for end users and database administorators but these people won't be able to decipher what |
|
|
||
| python3 "$CURDIR"/04036_interserver_tables_status_auth.python | ||
|
|
||
| $CLICKHOUSE_CLIENT -q "DROP TABLE IF EXISTS t_interserver_auth_check" |
There was a problem hiding this comment.
This stateless test was later removed in favor of the integration test test_interserver_tables_status_auth (needed for Bugfix validation and the two-nodes wrong-secret case), so this no longer applies.
| read_ws(sock); read_ws(sock); read_vu(sock) | ||
| sock.sendall(tsr) | ||
| data = sock.recv(4096) | ||
| print("FAIL: server returned data" if data else "OK: connection closed without leaking table status") |
There was a problem hiding this comment.
I wonder why there can be two different success paths (l. 56 and l. 58). Should the server not respond in a deterministic way?
There was a problem hiding this comment.
The server behavior is deterministic — it always closes the connection without sending data.
The two paths handle OS-level TCP delivery differences: an empty recv() result indicates a graceful close (FIN), while ConnectionResetError/EOFError indicates an abrupt close (RST). Both mean no data was leaked. Added a comment explaining this.
There was a problem hiding this comment.
This raw-socket stateless test was later replaced by the integration test test_interserver_tables_status_auth, which asserts the same property (connection closed with no data leaked) through the client library instead of raw recv, so the two-path handling is gone.
This comment was marked as resolved.
This comment was marked as resolved.
- Move `!is_interserver_authenticated` check into `processTablesStatusRequest()` inside the `if (is_interserver_mode)` block, as suggested by reviewer - Remove unnecessary `IF NOT EXISTS` / `IF EXISTS` from CREATE/DROP TABLE in test - Rename `vu`/`ws`/`read_vu`/`read_ws` to `varbyte`/`varstring`/`read_varbyte`/`read_varstring` - Replace terse AI-generated comments with protocol-level documentation explaining the Hello packet layout, TablesStatusRequest layout, and expected outcome - Rewrite chained server-Hello reads as one call per line with field annotations - Add comment explaining why two TCP close paths (graceful FIN vs RST) both count as success Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ed_table_existence_request
| ContextPtr context_to_resolve_table_names; | ||
| if (is_interserver_mode) | ||
| { | ||
| if (!is_interserver_authenticated) |
There was a problem hiding this comment.
TablesStatusRequest is parsed before the interserver-auth check, so an unauthenticated client can still force the server to decode and allocate request payloads before being rejected.
Since this branch closes the connection on auth failure anyway, please move the is_interserver_authenticated check before request.read(*in, client_tcp_protocol_version) to fail fast and avoid pre-auth parsing work.
There was a problem hiding this comment.
Fixed in 4155290 — the secret hash is now sent (client) and validated (server) before TablesStatusRequest::read, so an unauthenticated peer can no longer force the server to decode the request body.
There was a problem hiding this comment.
This is still present in the current head. In the new-protocol branch, processTablesStatusRequest still deserializes the full body at src/Server/TCPHandler.cpp:1617 before comparing the hash at src/Server/TCPHandler.cpp:1644, and TablesStatusRequest::read still fills plain std::string table names at src/Interpreters/TablesStatus.cpp:100-102.
That means a spoofed interserver client can announce revision 54487, send any 32-byte hash, and still force pre-auth allocations / CPU for an arbitrarily large table list before the connection is dropped. If we want the hash bound to the body without reopening that exposure, the digest needs to be computed while streaming or from an explicitly size-limited representation, not by materializing the whole unauthenticated request first.
tiandiwonder
added
the
can be tested
`processUnexpectedTablesStatusRequest` read the request body directly, so on an interserver connection with protocol revision >= 54486 an out-of-place `TablesStatusRequest` would decode the 32-byte hash prefix as the request body and fail with unrelated parse errors instead of `UNEXPECTED_PACKET_FROM_CLIENT`. Consume the same prefix as `processTablesStatusRequest`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@rschu1ze > Can you please check if this PR is a duplicate of #99675 ? Not a duplicate — the two PRs address complementary halves of #99512. This PR is exactly that tracked-separately part. |
# Conflicts: # src/Core/ProtocolDefines.h
The cluster-secret hash proving a peer may issue a `TablesStatusRequest` covered only `salt + nonce + cluster_secret + "TablesStatusRequest"`, not the requested table list, so a relayed hash could be reused to read the status of arbitrary tables. Fold an order-independent, length-prefixed digest of `request.tables` into the hash on both client and server, mirroring the per-query secret hash that `processQuery` computes over the query text. The hash precedes the body on the wire, so the server deserializes the body to recompute the digest before validating (as `processQuery` reads the query before validating), using `StringWithMemoryTracking` for the hash duplicate. Table resolution still happens only after validation, and an old-protocol unauthenticated request rejected by `interserver_tables_status_require_auth` is refused before its body is read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 105/111 (94.59%) · Uncovered code |
b361f3f
robot-ch-test-poll4
added
the
pr-must-backport-synced
Backport decision: master-onlyAfter analysis, this fix will not be backported to any release branch (OSS or Cloud). It stays on Why it can't be cleanly backportedThe fix authenticates the interserver
Mitigation for supported release branchesSupported release branches rely on network isolation of the interserver port. The vulnerability discloses only table existence / replication delay / readonly status to a party that can reach the interserver TCP port and present the interserver user marker — it reads no data and writes nothing — and the interserver port is normally not exposed outside the cluster. The bot-created cherry-pick PRs for all release branches are closed with a pointer here. |
robot-ch-test-poll
added
the
pr-backports-created
|
Our organization attempted an upgrade from 26.6 to 26.7 and started seeing errors which we suspect are caused by the new rejection path introduced in this PR. This caused queries to fail and made us unable to upgrade the cluster in a rolling manner. Does that sound correct? If so, we believe this should be marked as a backward-incompatible change. |
Yes, that's correct. Workaround: set
And yes — this should have been marked as a backward-incompatible change instead of being mentioned only in the setting's description. I'll add a changelog and upgrade-notes entry for it, and we'll revisit whether the default should be inverted so the hardening is opt-in until a cluster is fully upgraded. |
…ion test Follow-up to the interserver TablesStatusRequest authentication, addressing review comments on ClickHouse#99854: - Authenticate before decoding the request body. The secret hash is now sent (client) and validated (server) *before* `TablesStatusRequest::read`, so an unauthenticated peer can no longer force the server to decode an unauthenticated request. - Default `interserver_tables_status_require_auth` to `true` (secure by default): old-protocol peers that send no hash are rejected unless an operator opts out for a mixed-version rolling upgrade. Documented the upgrade caveat in the setting description. - Extend the integration test to also cover the new-protocol rejection path: two nodes configure the same cluster with different secrets, so a peer that signs the request with the wrong secret is rejected by hash validation during connection establishment (asserted via node_b's log), in addition to the old-protocol no-hash rejection. Verified locally: authenticated path still succeeds with the hash sent first and the setting defaulting on; old-protocol peer rejected by default.
…ated_table_existence_request Fix unauthenticated TablesStatusRequest in interserver mode
TCPHandler::receivePacketsExpectQuery() handles the packets that arrive
before any Query has been received. Its Data/Scalar case called
processUnexpectedData() and only then threw UNEXPECTED_PACKET_FROM_CLIENT.
That call builds a headerless NativeReader and reads a block, so the
column type name comes off the wire and is handed to DataTypeFactory.
Entering interserver mode requires only the name of a cluster that has a
non-empty <secret>, never the secret itself, and the secret is verified
only inside processQuery(), which this loop never reaches. So an
unauthenticated peer decided which type the server constructed and which
deserializer ran.
The read had no consumer for any peer either: its return value was
discarded, and the exception thrown right after is caught where
"if (!query_state) return;" exits runImpl, so the exception is never
sent, the connection is closed, and no further byte is read from the
socket. runImpl's exception handler already documents that policy ("the
server should not try to skip (parse, decompress) the remaining packets
sent by the client, as it will lead to additional work and unneeded
exposure to unauthenticated connections"); the parse it forbids had
already happened one frame earlier. The adjacent case of the same
switch, processObsoleteIgnoredPartUUIDs(), was given exactly this shape
in ClickHouse#99398.
No setting gates the rejection, deliberately. The TablesStatusRequest
path needed an operator opt-out when it was hardened in ClickHouse#99854 because
it was a served path, and rejecting it broke rolling upgrades (ClickHouse#113602).
Data before Query has always thrown, so there is no working flow to
preserve, and nothing a client can observe changes: the connection
closes with nothing written back, before and after.
The regression test completes an accepted interserver handshake over a
raw socket and then sends a Data packet declaring a column type that
does not exist. Without this change the server logs "Unknown data type
family: NoSuchTypeGroeneAI", which is the proof that a wire-supplied
type name reached DataTypeFactory before authentication. It is extended
into the existing pre-authentication interserver test module rather than
added as a new one, because that module already provides the cluster
with a <secret> and a handshake pinned below the chunked-packets
revision, which is what makes a hand-built packet feasible.
Requested by @ nikitamikhaylov, relayed via OranjeAI.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Fix a security issue where an unauthenticated TCP client could probe table existence and replication status via the interserver port.
Documentation entry for user-facing changes
Version info
26.7.1.975(included in26.7and later)