[PR #128] [CLOSED] fix: apply S4 transport padding to keepalive packets #153

New issue

Closed

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

bsakaguchi commented

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

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amneziawg-go/pull/128
Author: @wizier
Created: 3/31/2026
Status: ❌ Closed

Base: master ← Head: fix/s4-keepalive-padding

📝 Commits (1)

3ff8929 fix: apply S4 transport padding to keepalive packets

📊 Changes

1 file changed (+8 additions, -11 deletions)

View changed files

📝 device/send.go (+8 -11)

📄 Description

Bug

When S4 (transport padding) is non-zero, keepalive packets are sent without S4 padding, but the receiving side expects all transport packets to include it. This causes keepalives to be silently dropped, preventing the responder from ever calling timersHandshakeComplete().

Root cause

In device/send.go, RoutineSequentialSender only adds S4 padding inside the if len(elem.packet) != MessageKeepaliveSize branch:

// device/send.go:574-587
for _, elem := range elemsContainer.elems {
    if len(elem.packet) != MessageKeepaliveSize {
        dataSent = true

        if padding := device.paddings.transport; padding > 0 {
            // ... prepend S4 random bytes ...
        }
    }
    bufs = append(bufs, elem.packet)
}

Keepalive packets (len == MessageKeepaliveSize) skip the padding entirely.

On the receive side, DeterminePacketTypeAndPadding unconditionally expects S4 padding on all transport packets:

// device/receive.go:602-611
if size >= padding+MessageTransportHeaderSize {
    data := packet[padding:]
    if header.Validate(binary.LittleEndian.Uint32(data)) {
        return MessageTransportType, padding
    }
}

A keepalive without padding is smaller than padding + MessageTransportHeaderSize, so it fails the size check and is classified as MessageUnknownType → dropped.

Impact

After a handshake, the initiator immediately sends a keepalive to the responder. Since the keepalive is dropped, the responder never receives key confirmation and lastHandshakeNano stays at 0. The handshake only fully completes on the responder side when actual data traffic flows through the tunnel.

When S4 = 0 there is no issue — padding is 0, keepalive passes the size check normally.

Fix

Move the padding logic out of the dataSent guard so it applies to all transport packets including keepalives.

_{🔄 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/128 **Author:** [@wizier](https://github.com/wizier) **Created:** 3/31/2026 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix/s4-keepalive-padding` --- ### 📝 Commits (1) - [`3ff8929`](https://github.com/amnezia-vpn/amneziawg-go/commit/3ff8929137b576c8e3338d00822f37fb809e4827) fix: apply S4 transport padding to keepalive packets ### 📊 Changes **1 file changed** (+8 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `device/send.go` (+8 -11) </details> ### 📄 Description ## Bug When S4 (transport padding) is non-zero, keepalive packets are sent **without** S4 padding, but the receiving side expects **all** transport packets to include it. This causes keepalives to be silently dropped, preventing the responder from ever calling `timersHandshakeComplete()`. ## Root cause In `device/send.go`, `RoutineSequentialSender` only adds S4 padding inside the `if len(elem.packet) != MessageKeepaliveSize` branch: ```go // device/send.go:574-587 for _, elem := range elemsContainer.elems { if len(elem.packet) != MessageKeepaliveSize { dataSent = true if padding := device.paddings.transport; padding > 0 { // ... prepend S4 random bytes ... } } bufs = append(bufs, elem.packet) } ``` Keepalive packets (`len == MessageKeepaliveSize`) skip the padding entirely. On the receive side, `DeterminePacketTypeAndPadding` unconditionally expects S4 padding on all transport packets: ```go // device/receive.go:602-611 if size >= padding+MessageTransportHeaderSize { data := packet[padding:] if header.Validate(binary.LittleEndian.Uint32(data)) { return MessageTransportType, padding } } ``` A keepalive without padding is smaller than `padding + MessageTransportHeaderSize`, so it fails the size check and is classified as `MessageUnknownType` → dropped. ## Impact After a handshake, the **initiator** immediately sends a keepalive to the responder. Since the keepalive is dropped, the responder never receives key confirmation and `lastHandshakeNano` stays at 0. The handshake only fully completes on the responder side when actual data traffic flows through the tunnel. **When S4 = 0 there is no issue** — padding is 0, keepalive passes the size check normally. ## Fix Move the padding logic out of the `dataSent` guard so it applies to all transport packets including keepalives. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

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