diff mbox series

[Openvpn-devel,v1] clinat: do not adjust UDP checksum if zero

Message ID 20260821182442.13542-1-gert@greenie.muc.de
State New
Headers
Series [Openvpn-devel,v1] clinat: do not adjust UDP checksum if zero |

Commit Message

Gert Doering Aug. 21, 2026, 6:24 p.m. UTC 
  From: Antonio Quartulli <antonio@mandelbit.com>

As per RFC768, when the UDP checksum is zero, it means
it was not computed by the source, therefore any NAT
processing along the way should leave the checksum alone
and not update it.
Failing to do so would result in computing a bogus value.

At the same time, if the result of updating a non-zero
checksum ends up being zero, as per the same RFC, we must
store its one-complement (0xFFFF) as zero is reserved
for "checksum not computed", as mentioned above.

Ensure our Client NAT code follows both rules.

Github: closes OpenVPN/openvpn#1037
Reported-by: Jeff Salee <jeff@samjackson.com>
Change-Id: I4068bf8175c23151298d142dc920ab89f861a411
Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1681
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1681
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Razvan Cojocaru <razvanc@mailbox.org>

Comments

Gert Doering Aug. 22, 2026, 3:54 p.m. UTC | #1 
This is technically correct.  There are caveats for IPv6 (namely, 
zero is not allowed and a receiver is expected to drop packets with
such a checksum, RFC8200 8.1).  If we don't drop, but also do not
modify the field, we're not violating this rule.  On transmission,
the 0x0000->0xffff still needs to happen, so this code is correct
for IPv4 and IPv6.

I have not actually tested this, just stared at the code :-) - and we
have +2 from Razvan (thanks).

I'm applying this to release/2.7, as it's clearly a bug fix.  I am not
backporting to 2.6, 2.5, 2.4 etc as it's not a security issue, and the
first report we ever had was in May this year - so it seems to hit
infrequently enough (plus --client-nat is an edge case anyway).

Your patch has been applied to the master and release/2.7 branch (bugfix).

commit c797db6225755e6d7144d433ed55d3992d8ff141 (master)
commit e1d0005ec1a15fffbebc14fcf42ad36332e3a36a (release/2.7)
Author: Antonio Quartulli
Date:   Fri Aug 21 20:24:34 2026 +0200

     clinat: do not adjust UDP checksum if zero

     Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
     Acked-by: Razvan Cojocaru <razvanc@mailbox.org>
     Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1681
     Message-Id: <20260821182442.13542-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38581.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/clinat.c b/src/openvpn/clinat.c
index 32c1325..3724022 100644
--- a/src/openvpn/clinat.c
+++ b/src/openvpn/clinat.c
@@ -258,7 +258,23 @@ 
         {
             if (BLENZ(ipbuf) >= sizeof(struct openvpn_iphdr) + sizeof(struct openvpn_udphdr))
             {
-                ADJUST_CHECKSUM(accumulate, h->u.udp.check);
+                /* RFC 768: a UDP checksum of 0 means "no checksum computed".
+                 * Do not run the incremental adjustment over a non-checksum,
+                 * or we will write a bogus non-zero value into the field.
+                 */
+                if (h->u.udp.check)
+                {
+                    ADJUST_CHECKSUM(accumulate, h->u.udp.check);
+
+                    if (!h->u.udp.check)
+                    {
+                        /* RFC 768: a computed checksum of 0 must be transmitted
+                         * as 0xFFFF (one-complement), because 0 is reserved for
+                         * "no checksum computed"
+                         */
+                        h->u.udp.check = 0xFFFF;
+                    }
+                }
             }
         }
     }