AdguardTeam / AdguardTeam/dnsproxy

TCP: readPrefixed uses conn.Read for the 2-byte length prefix and ignores the byte count

Đang mở
#523 0 bình luận 0 reaction 1 người được giao Được @Chinaski1 nhận Xem trên GitHub
Priority: P4
Ngôn ngữ chính
Go
Star
3.3k
Fork
343
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

## What happens

`readPrefixed` in `proxy/servertcp.go` reads the 2-byte DNS-over-TCP length
prefix with a single `conn.Read` and discards the returned count:

```go
func readPrefixed(conn net.Conn) (b []byte, err error) {
l := make([]byte, 2)
_, err = conn.Read(l) // <-- returns n == 1 if only one byte has arrived
if err != nil {
return nil, fmt.Errorf("reading len: %w", err)
}

packetLen := binary.BigEndian.Uint16(l)
...
b = make([]byte, packetLen)
_, err = io.ReadFull(conn, b) // <-- the payload read gets this right
...
}
```

TCP is a byte stream with no message boundaries, so a client is free to deliver
those two bytes in two segments. When that happens, `conn.Read` returns after
the first byte, `l` is `[0x00, 0x00]`, `packetLen` becomes `0`, the following
`io.ReadFull` returns immediately with an empty buffer, and `req.Unpack` fails
on the empty message:

```
[error] dnsproxy: handling tcp; unpacking msg err="bad header id: dns: overflow unpacking uint16"
```

The connection is then closed. Whether this happens is a **race**: if the
reading goroutine is scheduled before the second segment arrives, `conn.Read`
returns 1 byte and the query fails; if both bytes are already in the socket
buffer, it returns 2 and the very same query is served normally.

I caught both outcomes 2 ms apart, same client, same byte pattern:

```
# connection A - one prefix byte, then the payload -> failure
12:19:07.079750 192.168.2.1.55647 > .210.53: P. length 1 [DNS over TCP: length 1 < 2]
12:19:07.080203 log: handling tcp; unpacking msg err="bad header id: dns: overflow unpacking uint16"
12:19:07.080285 192.168.2.1.55647 > .210.53: P. length 36
12:19:07.080323 .210.53 > 192.168.2.1.55647: F. connection closed

# connection B, 0.9 ms later - identical split -> served
12:19:07.081939 192.168.2.1.48233 > .210.53: P. length 1 [DNS over TCP: length 1 < 2]
12:19:07.082285 192.168.2.1.48233 > .210.53: P. length 36
12:19:07.084082 .210.53 > 192.168.2.1.48233: P. length 53 1/0/0 A 0.0.0.0
```

Note that the payload read two lines further down already uses `io.ReadFull` —
only the prefix read does not. (Incidentally, tcpdump's own dissector makes the
same assumption per segment, which is why it prints `prefix length(9175) !=
length(34)` for the second segment.)

## Reproducer

The `sleep` is what makes it deterministic — it guarantees losing the race
described above. Without it the same script fails only intermittently. The
query itself is valid and identical in both runs. Against AdGuard Home
v0.107.79 (which vendors dnsproxy), plain DNS on port 53:

```python
import socket, struct, time

qname = b"\x06events\x06mapbox\x03com\x00"
msg = struct.pack(">HHHHHH", 0x1234, 0x0100, 1, 0, 0, 0) + qname + b"\x00\x01\x00\x01"
prefix = struct.pack(">H", len(msg))

s = socket.create_connection(("192.0.2.1", 53), timeout=5)
s.setsockopt(socket.IPPROTO_TCP, socket.TCP_NODELAY, 1)
s.sendall(prefix[:1]) # first prefix byte on its own
time.sleep(0.05) # force a separate segment
s.sendall(prefix[1:] + msg) # second prefix byte + payload
print(s.recv(2))
```

```
split prefix -> b'' connection closed, one "bad header id" line in the log
single sendall -> 51-byte answer, rcode=0
```

## Suggested fix

`io.ReadFull(conn, l)` instead of `conn.Read(l)`, matching what the payload
read already does.

## How this shows up in practice

This is not a synthetic edge case. My consumer router — the DNS forwarder for
the whole LAN — writes the prefix separately as a matter of course, so every
one of its TCP queries enters the race. Losing it means a closed connection and
a retry, and since the retry usually goes through, nothing user-visible ever
breaks. What it does produce is log volume: on bad days tens of thousands of
lines, 56,415 in a single day at a steady ~5,000/hour around the clock, with
quiet weeks in between. The bursts line up with load, which fits a scheduling
race rather than a client that intermittently sends garbage.

Because nothing visibly breaks, this reads as harmless log noise, which I
suspect is why it tends to be attributed to the client. Some data points, in
case they save someone else the tcpdump session:

- Not an attack or a scan: 53/tcp, 853/tcp and 784/udp are not reachable from
the internet on my setup; the source is a LAN device.
- Resolution is not broken, so "does DNS still work?" does not catch it.
- `#198` reports the same code path (`handling tcp; unpacking msg`) with
`dns: buffer size too small` instead — that is the same bug with the prefix
split at a different offset, giving a different bogus `packetLen`. It was
answered with "it just means that an invalid query is written by the client".
I think that is worth revisiting: the query is well-formed, only its
segmentation is unusual — and segmentation is not something a client controls
or a receiver may assume.
- Related-looking issues such as AdGuardHome#8312, dnsproxy#282 and
dnsproxy#299 are about the UDP path and are, as far as I can tell, unrelated.

## Environment

- dnsproxy as vendored in AdGuard Home v0.107.79 (also reproduced on v0.107.76)
- Linux arm64, Docker, Raspberry Pi
- Client: consumer router acting as a DNS forwarder, using DNS-0x20 case
randomisation (visible as `eVEnts.MaPBox.COM` in the capture)

Hướng dẫn đóng góp

Chưa lập chỉ mục được hướng dẫn đóng góp cho kho mã nguồn này

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.