[PR #188] [CLOSED] fix: unchecked allocations and a TOCTOU race on handshake paths #225

New issue

Closed

opened 2026-10-13 21:07:47 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-13 21:07:47 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/amnezia-vpn/amneziawg-linux-kernel-module/pull/188
Author: @pattyboi
Created: 7/20/2026
Status: ❌ Closed

Base: master ← Head: fix/unchecked-allocations-noise-send

📝 Commits (2)

ec6e0d3 fix: two unchecked kzalloc()s on handshake paths crash under memory pressure
6d84680 fix: two unchecked kzalloc()s on handshake paths crash under memory pressure

📊 Changes

2 files changed (+37 additions, -13 deletions)

View changed files

📝 src/noise.c (+18 -4)
📝 src/send.c (+19 -9)

📄 Description

Two allocations added for the advanced-security features had no NULL
check, each introducing a new crash mode not present in upstream
WireGuard (which does no allocation on either of these paths):

wg_noise_handshake_consume_initiation() (noise.c) unconditionally
kzalloc'd a struct endpoint on every received handshake-initiation
packet, authenticated or not, then dereferenced it unconditionally
a few lines later in wg_socket_endpoint_from_skb() (an immediate
memset through the pointer). Under memory pressure this is a remote,
pre-authentication NULL-pointer dereference: anyone who can reach
the UDP port and knows the server's public key (to pass the outer
MAC1 check) can trigger it.

Fix: only allocate the endpoint when it's actually needed (the
unknown-peer branch), and check the result before using it. This
also means known-peer traffic -- the common case -- no longer pays
for an allocation it never used.

While in there: the unknown-peer branch also unconditionally built
and broadcast a genl multicast notification containing attacker-
influenced bytes (the unauthenticated claimed peer pubkey) for every
such packet, a new pre-auth allocation+broadcast cost on a path
upstream just silently drops. Added a 10/sec ratelimit around it,
checked before the allocation -- so a flood of unknown-peer
packets that's mostly being rate-limited away doesn't still pay for
the allocation on every rejected packet.
wg_packet_send_handshake_initiation() (send.c) allocates a
wg->jmax-sized buffer for dummy junk packets (used when Jc/Jmax are
configured) with no NULL check, then writes into it via
get_random_bytes() unconditionally. This runs on the local transmit
path for every handshake attempt when junk packets are enabled --
an allocation failure would crash the box on its own outbound
traffic, not an attacker-controlled path, but still a real
robustness bug for anyone running with Jc/Jmax set (a fairly common
'advanced security' configuration).

Fix: skip sending dummy junk for that handshake attempt if the
allocation fails, instead of writing through a NULL pointer.
Same function, found by an independent review pass before this was
submitted upstream: wg->jc gets snapshotted into a local
(junk_packet_count) before the send loop, but wg->jmin/wg->jmax did
not -- the loop re-read the live wg->jmax on every iteration while
using the value read once at kzalloc() time to size the buffer.
This function runs without device_update_lock, so a concurrent
'wg set jmax N' (raising jmax) landing mid-loop lets
get_random_bytes() write past the end of a buffer sized for the
old, smaller jmax -- a heap buffer overflow triggered by two racing
CAP_NET_ADMIN operations. Pre-existing bug, not introduced by fix
#2 above, but same function and same buffer-safety concern, so
fixed alongside it: snapshot jmin/jmax into locals once, same as jc,
and use those consistently for both the allocation size and the
loop's random range.

_{🔄 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-linux-kernel-module/pull/188 **Author:** [@pattyboi](https://github.com/pattyboi) **Created:** 7/20/2026 **Status:** ❌ Closed **Base:** `master` ← **Head:** `fix/unchecked-allocations-noise-send` --- ### 📝 Commits (2) - [`ec6e0d3`](https://github.com/amnezia-vpn/amneziawg-linux-kernel-module/commit/ec6e0d34a7156f7cda320f99c9684a01ae56b28a) fix: two unchecked kzalloc()s on handshake paths crash under memory pressure - [`6d84680`](https://github.com/amnezia-vpn/amneziawg-linux-kernel-module/commit/6d84680b06ac033e1c16c46518310fc5b37c9468) fix: two unchecked kzalloc()s on handshake paths crash under memory pressure ### 📊 Changes **2 files changed** (+37 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `src/noise.c` (+18 -4) 📝 `src/send.c` (+19 -9) </details> ### 📄 Description Two allocations added for the advanced-security features had no NULL check, each introducing a new crash mode not present in upstream WireGuard (which does no allocation on either of these paths): 1. wg_noise_handshake_consume_initiation() (noise.c) unconditionally kzalloc'd a struct endpoint on *every* received handshake-initiation packet, authenticated or not, then dereferenced it unconditionally a few lines later in wg_socket_endpoint_from_skb() (an immediate memset through the pointer). Under memory pressure this is a remote, pre-authentication NULL-pointer dereference: anyone who can reach the UDP port and knows the server's public key (to pass the outer MAC1 check) can trigger it. Fix: only allocate the endpoint when it's actually needed (the unknown-peer branch), and check the result before using it. This also means known-peer traffic -- the common case -- no longer pays for an allocation it never used. While in there: the unknown-peer branch also unconditionally built and broadcast a genl multicast notification containing attacker- influenced bytes (the unauthenticated claimed peer pubkey) for every such packet, a new pre-auth allocation+broadcast cost on a path upstream just silently drops. Added a 10/sec ratelimit around it, checked *before* the allocation -- so a flood of unknown-peer packets that's mostly being rate-limited away doesn't still pay for the allocation on every rejected packet. 2. wg_packet_send_handshake_initiation() (send.c) allocates a wg->jmax-sized buffer for dummy junk packets (used when Jc/Jmax are configured) with no NULL check, then writes into it via get_random_bytes() unconditionally. This runs on the local transmit path for every handshake attempt when junk packets are enabled -- an allocation failure would crash the box on its own outbound traffic, not an attacker-controlled path, but still a real robustness bug for anyone running with Jc/Jmax set (a fairly common 'advanced security' configuration). Fix: skip sending dummy junk for that handshake attempt if the allocation fails, instead of writing through a NULL pointer. 3. Same function, found by an independent review pass before this was submitted upstream: wg->jc gets snapshotted into a local (junk_packet_count) before the send loop, but wg->jmin/wg->jmax did not -- the loop re-read the live wg->jmax on every iteration while using the value read *once* at kzalloc() time to size the buffer. This function runs without device_update_lock, so a concurrent 'wg set <if> jmax N' (raising jmax) landing mid-loop lets get_random_bytes() write past the end of a buffer sized for the old, smaller jmax -- a heap buffer overflow triggered by two racing CAP_NET_ADMIN operations. Pre-existing bug, not introduced by fix #2 above, but same function and same buffer-safety concern, so fixed alongside it: snapshot jmin/jmax into locals once, same as jc, and use those consistently for both the allocation size and the loop's random range. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-13 21:07:47 +00:00