amnezia-vpn/amneziawg-go

Fork 0

mirror of https://github.com/amnezia-vpn/amneziawg-go.git synced 2026-10-13 21:34:25 +00:00

[PR #183] fix: random trailers cause ~3.5% of transport packets to be misclassified as handshakes #191

New issue

Open

opened 2026-10-13 21:04:26 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-13 21:04:26 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amneziawg-go/pull/183
Author: @AYastrebov
Created: 8/25/2026
Status: 🔄 Open

Base: master ← Head: fix/random-trailers-transport-misclassification

📝 Commits (3)

afe2fc5 fix: stop random trailers stealing ~3.5% of transport packets
67a2b96 fix: keep exact-size handshakes ahead of the transport test
2b855b9 test: pin that equal S1-S4 avoids the classification collision

📊 Changes

2 files changed (+344 additions, -6 deletions)

View changed files

📝 device/receive.go (+60 -6)
➕ device/receive_awg_test.go (+284 -0)

📄 Description

Summary

With RandomTrailers enabled and S1–S4 set to different values, roughly 3.5% of all transport packets are silently dropped as unparseable handshake messages. That is enough to collapse a TCP sender across the tunnel — I measured a real link fall from ~30 Mbit/s to ~0.4 Mbit/s, with the loss disappearing entirely when RandomTrailers was turned off and nothing else changed.

The config dependency matters, and I want to put it up front rather than bury it: setting S1 = S2 = S3 = S4 avoids the collision completely, and upstream documentation already recommends exactly that when random trailers are in use, "to reduce the risk of incorrect packet type detection."

So this is not a claim that the protocol is broken. It is a claim that the classifier silently depends on a configuration invariant it never checks, never warns about, and that generators in the wild do violate — and that when the invariant is broken the failure mode is 3.5% packet loss with no counter and no log line.

The bug

DeterminePacketTypeAndPadding tests the four message types in order: init, response, cookie, then transport. Random trailers relax the three handshake size tests from == to >, which is necessary — a trailer makes a handshake message longer than its fixed size:

if size == expectedSize || randomTrailers && size > expectedSize {

A transport packet is longer than any handshake message, so that relaxed test matches every full-size data packet too. The only thing then separating a data packet from a handshake misparse is the type-range check.

Each branch reads its candidate type field at its own S offset:

applyHash(headerBytes[:], packet[padding:padding+4], typeHash)

That is the crux. In a transport packet the real type field sits at S4. When S1 != S4, the initiation branch reads four bytes from somewhere else — junk prefix or ciphertext — which are uniformly random, and a header range spanning n of the 2^32 values claims n / 2^32 of them. The client config generator emits H1–H4 ranges 50,000,000 wide, which is 1.164% each, and three handshake ranges are tried before transport:

3 × 1.164% ≈ 3.5% of every transport packet

When all four S values are equal, every branch reads the real type field instead. That value was drawn from H4, and since H1–H4 must not overlap it cannot match H1, H2 or H3 — the collision rate drops to exactly zero.

Measured against unpatched master, 200,000 synthetic transport packets, 50M-wide ranges:

`S1`/`S2`/`S3`/`S4`	misclassified
49 / 57 / 37 / 27 (unequal)	3.431%
12 / 12 / 12 / 12 (equal)	0.000%

Those packets fail their MAC and are dropped with no counter and no log line. By the Mathis model, 3.5% loss over a 170 ms path at a 1373-byte MSS caps a flow at about 0.35 Mbit/s, which matches what I measured.

Why fix it in code if the docs already say to use equal values

Nothing enforces or warns. A config that violates the invariant looks completely normal, connects, passes small traffic, and loses 3.5% of bulk data in one direction.
Generators do violate it. The docker-amneziawg container emitted independently randomised S1–S4 alongside RandomTrailers = on for every AWG_VERSION=3.1 deployment until I fixed it this week; it is unlikely to be the only one.
The failure presents as "my upload is broken", not as packet loss, because the drop happens on the receive path — so it does not lead anyone to the S values.
The fix costs nothing in the compliant case, as the numbers below show.

The fix

Test transport first when random trailers are on.

Classification is a purely local decision that never appears on the wire, so the order costs no interoperability — a patched peer and an unpatched peer interoperate. It moves the same collision onto handshake messages (~1.16% via H4), which are rare and retried, instead of onto every data packet.

With the patch applied, both configurations are clean:

`S1`/`S2`/`S3`/`S4`	master	with this PR
49 / 57 / 37 / 27	3.431%	0.000%
12 / 12 / 12 / 12	0.000%	0.000%

A stronger fix would verify MAC1 before accepting a handshake match that only passed because of the relaxed size test, which would remove both collisions rather than trading one for the other. That is a larger change and I am happy to take this in that direction if you prefer it.

Tests

TestDeterminePacketTypeTransportNotStolenByRelaxedHandshakeSizes feeds 200,000 synthetic transport packets (S4 padding prefix, a type value from H4, ciphertext-shaped body) through DeterminePacketTypeAndPadding with realistic 50M-wide ranges and counts how many come back as anything other than transport.

On master:

--- FAIL: TestDeterminePacketTypeTransportNotStolenByRelaxedHandshakeSizes
    receive_awg_test.go:118: random trailers on: 6926/200000 transport packets
    misclassified as handshakes (3.463%)

With this change: PASS, 0 misclassified.

TestDeterminePacketTypeEqualPaddingsNeverCollide pins the configuration dependency in code rather than leaving it in prose. It passes both before and after the fix — it is a guard, not a reproducer, documenting why some deployments never saw this and failing if a future change makes classification depend on something other than the type ranges.

TestDeterminePacketTypeTransportExactWithoutRandomTrailers pins the RandomTrailers = off path, which was never affected and must stay that way.

Notes

The same ordering exists in the Linux kernel module (awg_determine_type_and_padding in src/receive.c, the random_trailers ? skb->len >= expected_len : skb->len == expected_len tests), so it needs the same change there. Happy to open that PR too.
TestAWGDevicePing fails on unmodified master, before and after this change. It is unrelated to this PR — #184 fixes it.

Verification

Measured end to end against a real AmneziaWG 3.1 server, changing one parameter at a time on both ends. The RandomTrailers rows are the ones that carry the argument; the other rows are single runs on a ~170 ms link and are indicative only.

Config	Down	Up
All 3.1 features	17.6 Mbit/s	0.44 Mbit/s
All 3.1 features except `RandomTrailers`	29.8 Mbit/s	30.8 Mbit/s
`RandomTrailers` alone	17.0 Mbit/s	0.73 Mbit/s

RandomTrailers is the only parameter that reproduces the collapse, and it reproduces it alone. Fixing the classifier on the receiving side restored that side's throughput to parity with an AmneziaWG 2.0 tunnel to the same host.

Independently reproduced on a second setup — kernel module 3.1.20260812 server, amneziawg-go 3.1.20260814 client, 22 ms path — where the same misconfiguration measured 1.7 Mbit/s up against a 100 Mbit/s baseline, and setting S1 = S2 = S3 = S4 with no other change restored it to 95–99 Mbit/s.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/amnezia-vpn/amneziawg-go/pull/183 **Author:** [@AYastrebov](https://github.com/AYastrebov) **Created:** 8/25/2026 **Status:** 🔄 Open **Base:** `master` ← **Head:** `fix/random-trailers-transport-misclassification` --- ### 📝 Commits (3) - [`afe2fc5`](https://github.com/amnezia-vpn/amneziawg-go/commit/afe2fc5a699e8d40627a612db7270f537a9744dd) fix: stop random trailers stealing ~3.5% of transport packets - [`67a2b96`](https://github.com/amnezia-vpn/amneziawg-go/commit/67a2b9608673274257d380ed1fb2cce88b2a4bb1) fix: keep exact-size handshakes ahead of the transport test - [`2b855b9`](https://github.com/amnezia-vpn/amneziawg-go/commit/2b855b97b02f87b5a71d4ab21413ecc632ab520d) test: pin that equal S1-S4 avoids the classification collision ### 📊 Changes **2 files changed** (+344 additions, -6 deletions) <details> <summary>View changed files</summary> 📝 `device/receive.go` (+60 -6) ➕ `device/receive_awg_test.go` (+284 -0) </details> ### 📄 Description ## Summary With `RandomTrailers` enabled **and `S1`–`S4` set to different values**, roughly **3.5% of all transport packets are silently dropped** as unparseable handshake messages. That is enough to collapse a TCP sender across the tunnel — I measured a real link fall from ~30 Mbit/s to ~0.4 Mbit/s, with the loss disappearing entirely when `RandomTrailers` was turned off and nothing else changed. The config dependency matters, and I want to put it up front rather than bury it: **setting `S1 = S2 = S3 = S4` avoids the collision completely**, and [upstream documentation already recommends exactly that](https://docs.amnezia.org/documentation/amnezia-wg) when random trailers are in use, "to reduce the risk of incorrect packet type detection." So this is not a claim that the protocol is broken. It is a claim that the classifier silently depends on a configuration invariant it never checks, never warns about, and that generators in the wild do violate — and that when the invariant is broken the failure mode is 3.5% packet loss with no counter and no log line. ## The bug `DeterminePacketTypeAndPadding` tests the four message types in order: init, response, cookie, then transport. Random trailers relax the three handshake size tests from `==` to `>`, which is necessary — a trailer makes a handshake message longer than its fixed size: ```go if size == expectedSize || randomTrailers && size > expectedSize { ``` A transport packet is longer than *any* handshake message, so that relaxed test matches every full-size data packet too. The only thing then separating a data packet from a handshake misparse is the type-range check. Each branch reads its candidate type field at **its own** `S` offset: ```go applyHash(headerBytes[:], packet[padding:padding+4], typeHash) ``` That is the crux. In a transport packet the real type field sits at `S4`. When `S1 != S4`, the initiation branch reads four bytes from somewhere else — junk prefix or ciphertext — which are uniformly random, and a header range spanning `n` of the 2^32 values claims `n / 2^32` of them. The client config generator emits H1–H4 ranges 50,000,000 wide, which is 1.164% each, and three handshake ranges are tried before transport: ``` 3 × 1.164% ≈ 3.5% of every transport packet ``` When all four `S` values are equal, every branch reads the *real* type field instead. That value was drawn from H4, and since H1–H4 must not overlap it cannot match H1, H2 or H3 — the collision rate drops to exactly zero. Measured against unpatched `master`, 200,000 synthetic transport packets, 50M-wide ranges: | `S1`/`S2`/`S3`/`S4` | misclassified | |---|---| | 49 / 57 / 37 / 27 (unequal) | **3.431%** | | 12 / 12 / 12 / 12 (equal) | **0.000%** | Those packets fail their MAC and are dropped with no counter and no log line. By the Mathis model, 3.5% loss over a 170 ms path at a 1373-byte MSS caps a flow at about 0.35 Mbit/s, which matches what I measured. ## Why fix it in code if the docs already say to use equal values - Nothing enforces or warns. A config that violates the invariant looks completely normal, connects, passes small traffic, and loses 3.5% of bulk data in one direction. - Generators do violate it. The `docker-amneziawg` container emitted independently randomised `S1`–`S4` alongside `RandomTrailers = on` for every `AWG_VERSION=3.1` deployment until I fixed it this week; it is unlikely to be the only one. - The failure presents as "my upload is broken", not as packet loss, because the drop happens on the receive path — so it does not lead anyone to the S values. - The fix costs nothing in the compliant case, as the numbers below show. ## The fix Test transport first when random trailers are on. Classification is a purely local decision that never appears on the wire, so the order costs no interoperability — a patched peer and an unpatched peer interoperate. It moves the same collision onto handshake messages (~1.16% via H4), which are rare and retried, instead of onto every data packet. With the patch applied, both configurations are clean: | `S1`/`S2`/`S3`/`S4` | master | with this PR | |---|---|---| | 49 / 57 / 37 / 27 | 3.431% | **0.000%** | | 12 / 12 / 12 / 12 | 0.000% | **0.000%** | A stronger fix would verify MAC1 before accepting a handshake match that only passed because of the relaxed size test, which would remove both collisions rather than trading one for the other. That is a larger change and I am happy to take this in that direction if you prefer it. ## Tests `TestDeterminePacketTypeTransportNotStolenByRelaxedHandshakeSizes` feeds 200,000 synthetic transport packets (S4 padding prefix, a type value from H4, ciphertext-shaped body) through `DeterminePacketTypeAndPadding` with realistic 50M-wide ranges and counts how many come back as anything other than transport. On master: ``` --- FAIL: TestDeterminePacketTypeTransportNotStolenByRelaxedHandshakeSizes receive_awg_test.go:118: random trailers on: 6926/200000 transport packets misclassified as handshakes (3.463%) ``` With this change: `PASS`, 0 misclassified. `TestDeterminePacketTypeEqualPaddingsNeverCollide` pins the configuration dependency in code rather than leaving it in prose. It passes both before and after the fix — it is a guard, not a reproducer, documenting why some deployments never saw this and failing if a future change makes classification depend on something other than the type ranges. `TestDeterminePacketTypeTransportExactWithoutRandomTrailers` pins the `RandomTrailers = off` path, which was never affected and must stay that way. ## Notes - The same ordering exists in the Linux kernel module (`awg_determine_type_and_padding` in `src/receive.c`, the `random_trailers ? skb->len >= expected_len : skb->len == expected_len` tests), so it needs the same change there. Happy to open that PR too. - `TestAWGDevicePing` fails on unmodified `master`, before and after this change. It is unrelated to this PR — #184 fixes it. ## Verification Measured end to end against a real AmneziaWG 3.1 server, changing one parameter at a time on both ends. The `RandomTrailers` rows are the ones that carry the argument; the other rows are single runs on a ~170 ms link and are indicative only. | Config | Down | Up | |---|---|---| | All 3.1 features | 17.6 Mbit/s | 0.44 Mbit/s | | All 3.1 features except `RandomTrailers` | 29.8 Mbit/s | 30.8 Mbit/s | | `RandomTrailers` alone | 17.0 Mbit/s | 0.73 Mbit/s | `RandomTrailers` is the only parameter that reproduces the collapse, and it reproduces it alone. Fixing the classifier on the receiving side restored that side's throughput to parity with an AmneziaWG 2.0 tunnel to the same host. Independently reproduced on a second setup — kernel module 3.1.20260812 server, `amneziawg-go` 3.1.20260814 client, 22 ms path — where the same misconfiguration measured 1.7 Mbit/s up against a 100 Mbit/s baseline, and setting `S1 = S2 = S3 = S4` with no other change restored it to 95–99 Mbit/s. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-13 21:04:26 +00:00

bsakaguchi referenced this issue

2026-10-13 21:04:28 +00:00

[PR #191] fix: reject jmin > jmax instead of underflowing the junk packet size #195