ClickHouse / ClickHouse/clickhouse-cpp

Blocking socket during handshake allows denial of service via partial TCP message

Ouverte
#486 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
Langage dominant
C
Étoiles
382
Forks
209
Merge moyen
1 j 22 h
PR mergées (30 j)
13

Description

# Blocking socket during handshake allows denial of service via partial TCP message

## Summary

The clickhouse-cpp client uses blocking sockets with no receive timeout during the TCP handshake phase. A malicious server, man-in-the-middle, or network fault that delivers a partial handshake response can cause the client thread to hang **indefinitely** on `recv()`, effectively denying service.

## Environment

- **Library**: clickhouse-cpp (all current versions)
- **Affected platforms**: All (Linux, macOS, Windows)
- **Affected component**: `Client::Impl::Handshake()` → `ReceiveHello()`

## Root Cause

Three factors combine to create the vulnerability:

1. **`connection_recv_timeout` defaults to 0** ([client.h, line 120](https://github.com/ClickHouse/clickhouse-cpp/blob/master/clickhouse/client.h#L120)), which means `SO_RCVTIMEO` is never effectively set — `recv()` blocks forever.

2. **Socket is switched to blocking mode after connect** ([socket.cpp, `SocketConnect()`](https://github.com/ClickHouse/clickhouse-cpp/blob/master/clickhouse/base/socket.cpp)): `SetNonBlock(*s, false)` is called once the connection succeeds.

3. **`ReceiveHello()` performs multiple sequential blocking reads** ([client.cpp, `ReceiveHello()`](https://github.com/ClickHouse/clickhouse-cpp/blob/master/clickhouse/client.cpp)): 8 `WireFormat::Read*()` calls, each of which can individually block on `recv()` if data stops arriving mid-message.

## Why `SetConnectionRecvTimeout` is not a viable workaround

`SO_RCVTIMEO` is set **once** on the socket during `SocketConnect()` and applies to **all** `recv()` calls for the socket's entire lifetime. Setting it to a short value (e.g. 10s) would protect the handshake, but would also kill long-running queries — a `SELECT` that takes minutes or hours to compute on the server side would timeout on the client before any data arrives.

There is currently **no way to set a different timeout for the handshake vs. query execution**.

## Concrete attack / failure scenarios

Any of the following byte sequences sent by a rogue server (or injected by a MITM) after the client sends its `Hello` packet will hang the client thread forever:

### Scenario 1: Single continuation byte (1 byte total)

```
0x80
```

The `ReadVarint64()` loop reads byte `0x80`: continuation bit is set (meaning "more bytes follow"), value bits = 0. It loops to call `ReadByte()` again → `recv()` → **blocks forever**.

### Scenario 2: Valid packet type + truncated string length (2 bytes total)

```
0x00 0x85
```

- `0x00` = varint 0 = `ServerCodes::Hello` ✓
- `0x85` = start of string length varint, continuation bit set → `ReadByte()` → `recv()` → **blocks forever**

### Scenario 3: Valid header + truncated string body (7 bytes total)

```
0x00 0x0A 'C' 'l' 'i' 'c' 'k'
```

- `0x00` = `ServerCodes::Hello`
- `0x0A` = string length 10
- 5 of 10 expected bytes arrive

`ReadAll()` reads the 5 available bytes, loops back for the remaining 5 → `recv()` → **blocks forever**.

### Scenario 4: Network fault mid-handshake

Server sends a valid Hello response, but TCP connection drops (RST lost, half-open state) between any two of the 8 read operations in `ReceiveHello()`. With no `SO_RCVTIMEO`, the client hangs indefinitely. Even TCP keepalive (disabled by default, 75s detection time when enabled) doesn't help against a server that keeps the connection open but stops sending.

## Affected code path

```
Client::Impl::ResetConnection()
└─ Handshake() # client.cpp
├─ SendHello() # succeeds
└─ ReceiveHello() # client.cpp:1065
├─ ReadVarint64 (packet_type) ← BLOCK POINT 1
├─ ReadString (server_name) ← BLOCK POINT 2 (varint len + body)
├─ ReadUInt64 (version_major) ← BLOCK POINT 3
├─ ReadUInt64 (version_minor) ← BLOCK POINT 4
├─ ReadUInt64 (revision) ← BLOCK POINT 5
├─ ReadString (timezone) ← BLOCK POINT 6
├─ ReadString (display_name) ← BLOCK POINT 7
└─ ReadUInt64 (version_patch) ← BLOCK POINT 8
```

Each `Read*` call eventually reaches:
```
WireFormat::ReadVarint64 / ReadAll
→ BufferedInput::DoNext
→ SocketInput::DoRead
→ ::recv(fd, buf, len, 0) // blocks forever when SO_RCVTIMEO=0
```

## Proposed fix

Add a **handshake-scoped timeout** that is independent of the query recv timeout. Two options:

### Option A: New `connection_handshake_timeout` option (recommended)

Add a dedicated option with a safe default:

```cpp
DECLARE_FIELD(connection_handshake_timeout, std::chrono::milliseconds,
SetConnectionHandshakeTimeout, std::chrono::seconds(30));
```

In `ResetConnection()`, temporarily apply it to the socket before `Handshake()`, then restore the user's `connection_recv_timeout` after:

```cpp
void Client::Impl::ResetConnection() {
auto socket = socket_factory_->connect(options_, current_endpoint_.value());

// Apply handshake timeout
SetSocketTimeout(socket->fd(), options_.connection_handshake_timeout);

InitializeStreams(std::move(socket));
inserting_ = false;

if (!Handshake()) {
throw ProtocolError("fail to connect to " + options_.host);
}

// Restore user's recv timeout (0 = infinite for long queries)
SetSocketTimeout(socket->fd(), options_.connection_recv_timeout);
}
```

### Option B: Reuse `connection_connect_timeout` for handshake

Apply the existing `connection_connect_timeout` (default 5s) as a temporary `SO_RCVTIMEO` during `Handshake()` only. No new options needed, but conflates two different operations.

## Impact

- **Severity**: Medium-High (denial of service / thread hang)
- **Attack vector**: Network (MITM, rogue server, network fault)
- **User impact**: Client thread hangs permanently, no recovery possible without process kill
- **Workaround**: None that doesn't also break long-running queries

Guide de contribution

Aucun guide de contribution indexé pour ce dépôt

Piste de recherche

Commencez dans client.cpp avec Client::Impl::ResetConnection(), Handshake() et ReceiveHello(), puis examinez client.h pour les options de délai d’expiration de connexion et base/socket.cpp pour SocketConnect(). Suivez la manière dont le délai d’expiration de réception du socket est appliqué et restauré ; le travail est terminé lorsqu’un handshake partiel ne peut pas bloquer indéfiniment, tandis que les requêtes de longue durée conservent le comportement configuré du délai d’expiration de réception. Aucun fichier de test n’est indiqué dans l’issue.

Rédigé par le modèle d'indexation à partir du texte de l'issue.

Évaluation

Stack technique
cpp
Domaine
networking, security
Type d'issue
Bug
Difficulté
4/5
Temps estimé
3-5 jours
Activité
Calme
Clarté
Plutôt claire
Accessibilité débutants
45/100

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.