Frame overhead does not account for the `aead-epoch` 8-byte packet id in `calc_packet_id_size_dc`

[raphaelthomas]

Describe the bug
On any 2.7 tunnel with an AEAD data cipher (e.g., the default AES-256-GCM), the
aead-epoch data-channel format is negotiated automatically: each 2.7 peer
advertises IV_PROTO_DATA_EPOCH and the server enables it for AEAD ciphers.
The on-wire packet id is then 8 bytes, but calc_packet_id_size_dc() in
mtu.c still reports 4. The overhead is undercounted by 4 bytes, so mssfix N mtu emits an outer datagram of N + 4. Packets sized to the path or link MTU
exactly then overshoot it by 4 bytes and are dropped by any strict next hop,
leading to large TCP flows (TLS/SSH handshakes, bulk transfer) stalling.

To Reproduce

• 2.7 on both ends, --proto udp, AEAD data cipher (AES-256-GCM), mssfix M mtu in effect (the default is M=1492).
• Send a full-size TCP segment through the tunnel; capture the outer UDP
  datagram on the link.
• Its IP length is M + 4 (see e.g. with tcpdump -ni IFACE -v)

Expected behavior
The outer UDP datagram should be <= M: with mssfix M mtu the overhead
calculation should reserve the true on-wire packet id size (8 bytes for
aead-epoch), so encapsulated packets never exceed the configured MTU.

Version information (please complete the following information):

• OS: Linux From Scratch, kernel 6.18
• OpenVPN version: 2.7.4 (reproduced); mtu.c unchanged in 2.7.5 / master
• Both server and client run the same version.

Additional context
packet_id_write_epoch() writes an 8-byte id (uint64_t: 16-bit epoch +
48-bit counter), but calc_packet_id_size_dc() returns packet_id_size(false)
= 4 for any TLS+AEAD tunnel and never consults CO_EPOCH_DATA_KEY_FORMAT. So
the AEAD framing is computed as 4 + 4 + 16 = 24 while the wire is 4 + 8 + 16 = 28 (opcode+peerid | packet id | GCM tag).

Proposed fix: make calc_packet_id_size_dc() epoch-aware, with the epoch size
named packet_id_size_epoch() next to packet_id_size(). The gate is the
negotiated flag options->imported_protocol_flags & CO_EPOCH_DATA_KEY_FORMAT
rather than cipher_kt_mode_aead().

 // src/openvpn/mtu.c
 unsigned int
 calc_packet_id_size_dc(const struct options *options, const struct key_type *kt)
 {
     bool tlsmode = options->tls_server || options->tls_client;

+    if (tlsmode && (options->imported_protocol_flags & CO_EPOCH_DATA_KEY_FORMAT))
+    {
+        return packet_id_size_epoch();
+    }
+
     bool packet_id_long_form = !tlsmode || cipher_kt_mode_ofb_cfb(kt->cipher);

     return packet_id_size(packet_id_long_form);
 }

 // src/openvpn/packet_id.h - beside packet_id_size()
+static inline int
+packet_id_size_epoch(void)
+{
+    return sizeof(uint64_t);
+}

Workaround: reserve >= 4 bytes headroom below the true MTU in mssfix N mtu.

[ordex]

Hi @raphaelthomas thanks a lot for your report!
From what I am reading, this indeed makes a lot of sense. I wonder why we haven't seen this issue as it's quite impactful (as you say).
Maybe @schwabe can offer his opinion when he's back.

This said, please be aware that we prefer to receive patches directly on the openvpn-devel mailing list, so they can happily enter our workflow (gerrit/buildbot testing/etc..)

[raphaelthomas]

Hi @ordex, thanks a lot for your fast response!

A contributing factor for not spotting this is probably the default mssfix 1492 mtu whilst various MTUs in mtu.h default to 1500. So there is a "extra" margin of 8 bytes by default, which in turn means that an overrun of 4 bytes is still below the 1500 MTU and thus nothing gets dropped. In practice this might therefore limit the blast radius at least for somewhat well-behaved systems running on defaults.

I'm not overly proficient with C, but I'll happily try get a patch submitted in the coming days.

[raphaelthomas]

0001-mtu.c-account-for-8-byte-packet-id-with-aead-epoch-f.patch