From patchwork Mon Dec 1 14:04:18 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Moritz Fain X-Patchwork-Id: 4652 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:6c3:b0:7b1:439f:bdf with SMTP id j3csp7010768maw; Mon, 1 Dec 2025 06:35:01 -0800 (PST) X-Forwarded-Encrypted: i=2; AJvYcCWHQFD8/RTgPlD4IZ77GcKTQcRJPklpiDEsVK6KnACX0dXBVsvcwRLKtvcsnyuy/JhBpyfWdc+/2cE=@openvpn.net X-Google-Smtp-Source: AGHT+IGpQxSwzQKiD5dIxIC4pLamghMuDoi70efyPsZy7Ybwqoxpu7aFURp3OSMbJ845j4ML4cpC X-Received: by 2002:a05:6871:2b26:b0:3c9:adbd:aa37 with SMTP id 586e51a60fabf-3eca15e78f7mr20257277fac.2.1764599701440; Mon, 01 Dec 2025 06:35:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1764599701; cv=none; d=google.com; s=arc-20240605; b=TN7mvmURdikF+qmWflWHiebQIDQa4P9tGAy838ZH0OQLmvhCNA+/BAlpS/3Y6Rfw5q O5yAprmR/VJUfQuR7bAlaWdceMI10T1akUqIwC+ZfBc5Tkcy0b5NxHK45va78Q4x23k5 sFMo+hc+BfmcTSu9muu8EX9iqDFAA1/e/jc1qYP3uecbvrU6p6ZsxTGWc7ZsjIdGqu+J 9c1KEPK/hopgJP3Xh6tuilHTY74u812Yt1wMe3tsXrqfXNBh2eYH6UUnBZ1z5lQUrjlQ ++/X1Z9ZrjbcaBzolBG+2IO0hQ+kKjxXhwHJid0D7kQr0vP7pSIeihIIkrsnVlBg63wV ZmXw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :to:message-id:date:from:mime-version:dkim-signature:dkim-signature :dkim-signature; bh=t4vzQPSbfWrWWRFtLkqZdrJy1hdU6A0HLKT5E4cpACk=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=O/NIwZKR8ZiMcv0xJDCwSpJQ0+a6v6ZOGClZMZYYjBZ0SePN4b1V1rZLATkYhg4rUN vMZ+d4u69+ZNZkumOsYwuJSlpcTGsYgnU6pKk67+YPa6B7q0uYAO0MYU/sPGzyrK28sL c6v/j7IZjwC1DUHUy5dVsiomuY6w/1vLTEuQ783XO+cOvr2bZLOnSqepqOe5axQolid6 lhYJD7tYqogoANBaa+Y7C0MCnXW3HWQSXI89Ww3SjKO2BHEu50geG3FwU2H0CTaM/3Co zXcUfu6WxW/oAS7l8YqNNG/zTg3TDGSFqec7VJS3usIeydL29f0pJSizc/T6ZdgvJRU2 sYzA==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=LDZFz9eC; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=HjwM4W7C; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=ijaH6NM5; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 586e51a60fabf-3f0dd0431a0si2312142fac.989.2025.12.01.06.35.01 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Dec 2025 06:35:01 -0800 (PST) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=LDZFz9eC; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=HjwM4W7C; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=ijaH6NM5; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.sourceforge.net; s=beta; h=Content-Transfer-Encoding:Content-Type: List-Subscribe:List-Help:List-Post:List-Archive:List-Unsubscribe:List-Id: Subject:To:Message-ID:Date:From:MIME-Version:Sender:Reply-To:Cc:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Owner; bh=t4vzQPSbfWrWWRFtLkqZdrJy1hdU6A0HLKT5E4cpACk=; b=LDZFz9eCs7RixaLIEViJrteBgz KPWKVOt4WWha3WDnOYMySeYLuT+04bMVDZMCG0mcxvI8QYbmZNPUhn6LeLF7nFDQq6Sk0DFQuEmQL Pstwb25hFgvce7cCHRIUNDKwanNyS3OMWOpHleAcY9VoXT1E4t74ZDTHxklj+dTAIz6k=; Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1vQ4zQ-0003il-FL; Mon, 01 Dec 2025 14:34:56 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1vQ4zN-0003ic-Gi for openvpn-devel@lists.sourceforge.net; Mon, 01 Dec 2025 14:34:53 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:To:Subject:Message-ID:Date:From: MIME-Version:Sender:Reply-To:Cc:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=LTNugGGp9W8vlwBhGJmKO4FQMOn/k0to91/8+mW/H3s=; b=HjwM4W7CwycXNjNp7Fp83xPMqi PgehSiGhr2Rdy2PzE1TTUaI31HgmTlgvEAWUuteO9SBEWlu6E4cWeopBscq1+tnHb/g0Rct3tqPp0 13UrssYBcwj+c/ekuWvA6uIpbb6xmkhgm2OnDNwFVffM71kirx23k7A9L/pY80N0mH8g=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:To:Subject:Message-ID:Date:From:MIME-Version:Sender:Reply-To :Cc:Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To: References:List-Id:List-Help:List-Unsubscribe:List-Subscribe:List-Post: List-Owner:List-Archive; bh=LTNugGGp9W8vlwBhGJmKO4FQMOn/k0to91/8+mW/H3s=; b=i jaH6NM52wEQzZQ25rYbmcfP5TIW44nXoCf3Fdxmb8WuhuDI3q2ggeeTNYdF6m25Tn19yX84TtrCdn XkzxdSh1Cbyk270LR9uhfAA5SvPnuYSVznfAZQB1VwUnESVfazE0kA6SrfnaJRZ14moSCCkv6Cqxe a+xzpynzGPUz1F4A=; Received: from mail-qk1-f175.google.com ([209.85.222.175]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1vQ4zM-0008HE-Fn for openvpn-devel@lists.sourceforge.net; Mon, 01 Dec 2025 14:34:53 +0000 Received: by mail-qk1-f175.google.com with SMTP id af79cd13be357-8b2da83f721so437671785a.1 for ; Mon, 01 Dec 2025 06:34:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764599681; x=1765204481; h=to:subject:message-id:date:from:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=LTNugGGp9W8vlwBhGJmKO4FQMOn/k0to91/8+mW/H3s=; b=n0/iss+pmmVvie7v6AuX86lnCxb0hOQrJnUsywVIgTc5NTC2I1lp2+bLZXAMphPak1 nkOTXlP8c5qjC/tjF9exIR1HyLF2UpaildA1GkU8+Gv0tzbk5i6BkUthC2YkDTpob9d7 v/QdA2ZL+I3J92Xxn+LjQRBWV3kSzG/GeVPquxxtZrFjTkAWK9eoi/FAs/3j/DjMieQ4 mRivF7TnJFkEHyKznr1rxV+rZiAbArM53JI6k1w0f/sjxx80P0JsXzzjrvJke2av8z24 RTk97AphmCwmTpmxOKuA6oHlMuefY/ndOSngL0Xq5zdS9aOClC5ZullHiOWbWeLnUhMK FgCg== X-Gm-Message-State: AOJu0YyEj52szlKq+69xN1o0d9OTAF2yKAhOZ7eb0bO1lMK/G45rH6pR AryzKG2ghhHeSfAGgqdi7E4BPTXq7TMa1OArvDpyJ574uVKg0GQV4rkRfV5e+z01aaceFRVlA9C cD/jpks8apAv+dg9RHWfTljdI1W1ZJO+XVgdEpJGIj7KDbIfGCm10/g== X-Gm-Gg: ASbGncvqMh4ztXc+DI3FCFzJsMnYXlbs4m49su6qvrj/khrzBguTQdXEmgmaIP/zcM/ GJgmVU+9IXg4R7NLdbUmI/AO2M8eXuegF2ooT0hsCWFOBT3QlpLE+D5Po9hZDE4hHP42lnQG25j Xp9zyY6m5R2QPtKp0JbThg3aB+GL4cl/2/I3B/OdinHoD9S5jzrvyA/Jkovyq/MFbu7nwQd917k 2zMkeqIEW30VCQmnsKypR09BjzLrMReNv61Qo+cAsAN5HkYkbdBo7CjINhQfe2xZtAZCg== X-Received: by 2002:a05:6102:3707:b0:5dd:a0bf:8c89 with SMTP id ada2fe7eead31-5e1c3c1b82cmr17279168137.7.1764597884231; Mon, 01 Dec 2025 06:04:44 -0800 (PST) MIME-Version: 1.0 From: Moritz Fain Date: Mon, 1 Dec 2025 15:04:18 +0100 X-Gm-Features: AWmQ_blEF28peh7l-Xwuimj5gPyKlN-6aIRnatbot0avXoOGqd1dkcSdwYtXPfs Message-ID: To: openvpn-devel@lists.sourceforge.net X-Spam-Score: 0.2 (/) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-1.hosts.colo.sdot.me", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: https://github.com/OpenVPN/openvpn/pull/925 Previously, the logic for resetting push options (like 'route') was based on `update_options_found` which was local to `apply_push_options`. This meant that if a PUSH_UPDATE was split across multiple [...] Content analysis details: (0.2 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 RCVD_IN_DNSWL_BLOCKED RBL: ADMINISTRATOR NOTICE: The query to DNSWL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#DnsBlocklists-dnsbl-block for more information. [209.85.222.175 listed in list.dnswl.org] 0.2 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level mail domains are different 0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.222.175 listed in wl.mailspike.net] X-Headers-End: 1vQ4zM-0008HE-Fn Subject: [Openvpn-devel] [Bugfix v2]: PUSH_UPDATE: fix option reset logic in continuation messages X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1850316896100169227?= X-GMAIL-MSGID: =?utf-8?q?1850316896100169227?= https://github.com/OpenVPN/openvpn/pull/925 Previously, the logic for resetting push options (like 'route') was based on `update_options_found` which was local to `apply_push_options`. This meant that if a PUSH_UPDATE was split across multiple continuation messages, the state was lost, causing routes to be reset multiple times (once per message chunk) rather than once per update sequence. This patch moves the state tracking to `struct options` as `push_update_options_found`, allowing it to persist across the entire PUSH_UPDATE sequence. This fixes an issue where large route lists sent via PUSH_UPDATE would result in only the last chunk's routes being applied, or previous routes being continuously deleted and re-added. Added unit test `test_incoming_push_continuation_route_accumulation` to verify the fix. --- src/openvpn/options.c | 27 ++--- src/openvpn/options.h | 6 +- src/openvpn/options_parse.c | 8 +- src/openvpn/push.c | 8 +- tests/unit_tests/openvpn/test_options_parse.c | 2 +- .../unit_tests/openvpn/test_push_update_msg.c | 106 +++++++++++++++++- 6 files changed, 131 insertions(+), 26 deletions(-) buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, @@ -61,6 +75,12 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu { char line[OPTION_PARM_SIZE]; + /* + * Use persistent push_update_options_found from options struct to track + * which option types have been reset across continuation messages. + * This is the FIXED behavior - routes are only reset once per PUSH_UPDATE sequence. + */ + while (buf_parse(buf, ',', line, sizeof(line))) { unsigned int push_update_option_flags = 0; @@ -84,10 +104,27 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu return false; /* Cause push/pull error and stop push processing */ } } - /* - * No need to test also the application part here - * (add_option/remove_option/update_option) - */ + + /* Simulate route handling from update_option() in options.c */ + if (strncmp(&line[i], "route ", 6) == 0) + { + if (!(options->push_update_options_found & OPT_P_U_ROUTE)) + { + /* First route in entire PUSH_UPDATE sequence - reset routes once */ + route_reset_count++; + options->push_update_options_found |= OPT_P_U_ROUTE; + } + route_add_count++; + } + /* Simulate add_option() push-continuation logic */ + else if (!strcmp(&line[i], "push-continuation 2")) + { + options->push_continuation = 2; + } + else if (!strcmp(&line[i], "push-continuation 1")) + { + options->push_continuation = 1; + } } return true; } @@ -292,6 +329,65 @@ test_incoming_push_message_mix2(void **state) free_buf(&buf); } +/** + * Test that routes accumulate correctly across multiple continuation messages. + * This test exposes a bug where update_options_found is reset to 0 for each + * continuation message, causing routes to be reset on each message instead + * of accumulating. + * + * Expected behavior: routes should only be reset ONCE (when the first route is received), + * then all subsequent routes should accumulate. + * + * Current bug: routes are reset on the first route of EACH continuation message. + */ +static void +test_incoming_push_continuation_route_accumulation(void **state) +{ + struct context *c = *state; + unsigned int option_types_found = 0; + + reset_route_counters(); + + /* Message 1: first batch of routes, continuation 2 (more coming) */ + struct buffer buf1 = alloc_buf(512); + const char *msg1 = "PUSH_UPDATE, route 10.1.0.0 255.255.0.0, route 10.2.0.0 255.255.0.0, route 10.3.0.0 255.255.0.0,push-continuation 2"; + buf_write(&buf1, msg1, strlen(msg1)); + + assert_int_equal(process_incoming_push_msg(c, &buf1, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_CONTINUATION); + free_buf(&buf1); + + /* Message 2: more routes, continuation 2 (more coming) */ + struct buffer buf2 = alloc_buf(512); + const char *msg2 = "PUSH_UPDATE, route 10.4.0.0 255.255.0.0, route 10.5.0.0 255.255.0.0, route 10.6.0.0 255.255.0.0,push-continuation 2"; + buf_write(&buf2, msg2, strlen(msg2)); + + assert_int_equal(process_incoming_push_msg(c, &buf2, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_CONTINUATION); + free_buf(&buf2); + + /* Message 3: final batch of routes, continuation 1 (last message) */ + struct buffer buf3 = alloc_buf(512); + const char *msg3 = "PUSH_UPDATE, route 10.7.0.0 255.255.0.0, route 10.8.0.0 255.255.0.0, route 10.9.0.0 255.255.0.0,push-continuation 1"; + buf_write(&buf3, msg3, strlen(msg3)); + + assert_int_equal(process_incoming_push_msg(c, &buf3, c->options.pull, pull_permission_mask(c), + &option_types_found), + PUSH_MSG_UPDATE); + free_buf(&buf3); + + /* Verify: all 9 routes should have been added */ + assert_int_equal(route_add_count, 9); + + /* + * Verify: route option is reset only one time in the first message + * if a push-continuation is present. + */ + assert_int_equal(route_reset_count, 1); +} + #ifdef ENABLE_MANAGEMENT char *r0[] = { "PUSH_UPDATE,redirect-gateway local,route 192.168.1.0 255.255.255.0", @@ -603,6 +699,8 @@ main(void) cmocka_unit_test_setup_teardown(test_incoming_push_message_bad_format, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix, setup, teardown), cmocka_unit_test_setup_teardown(test_incoming_push_message_mix2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_continuation_route_accumulation, setup, + teardown), #ifdef ENABLE_MANAGEMENT cmocka_unit_test_setup_teardown(test_send_push_msg0, setup2, teardown2), diff --git a/src/openvpn/options.c b/src/openvpn/options.c index a3fc19d6..0988dfd0 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3234,6 +3234,7 @@ pre_connect_restore(struct options *o, struct gc_arena *gc) o->push_continuation = 0; o->push_option_types_found = 0; + o->push_update_options_found = 0; o->imported_protocol_flags = 0; } @@ -5409,14 +5410,14 @@ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found) + struct env_set *es) { const bool pull_mode = BOOL_CAST(permission_mask & OPT_P_PULL_MODE); ASSERT(MAX_PARMS >= 7); if (streq(p[0], "route") && p[1] && !p[5]) { - if (!(*update_options_found & OPT_P_U_ROUTE)) + if (!(options->push_update_options_found & OPT_P_U_ROUTE)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (!check_route_option(options, p, msglevel, pull_mode)) @@ -5429,12 +5430,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl es, &c->net_ctx); RESET_OPTION_ROUTES(options->routes, routes); } - *update_options_found |= OPT_P_U_ROUTE; + options->push_update_options_found |= OPT_P_U_ROUTE; } } else if (streq(p[0], "route-ipv6") && p[1] && !p[4]) { - if (!(*update_options_found & OPT_P_U_ROUTE6)) + if (!(options->push_update_options_found & OPT_P_U_ROUTE6)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (!check_route6_option(options, p, msglevel, pull_mode)) @@ -5447,12 +5448,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl ROUTE_OPTION_FLAGS(&c->options), es, &c->net_ctx); RESET_OPTION_ROUTES(options->routes_ipv6, routes_ipv6); } - *update_options_found |= OPT_P_U_ROUTE6; + options->push_update_options_found |= OPT_P_U_ROUTE6; } } else if (streq(p[0], "redirect-gateway") || streq(p[0], "redirect-private")) { - if (!(*update_options_found & OPT_P_U_REDIR_GATEWAY)) + if (!(options->push_update_options_found & OPT_P_U_REDIR_GATEWAY)) { VERIFY_PERMISSION(OPT_P_ROUTE); if (options->routes) @@ -5465,12 +5466,12 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl } env_set_del(es, "route_redirect_gateway_ipv4"); env_set_del(es, "route_redirect_gateway_ipv6"); - *update_options_found |= OPT_P_U_REDIR_GATEWAY; + options->push_update_options_found |= OPT_P_U_REDIR_GATEWAY; } } else if (streq(p[0], "dns") && p[1]) { - if (!(*update_options_found & OPT_P_U_DNS)) + if (!(options->push_update_options_found & OPT_P_U_DNS)) { VERIFY_PERMISSION(OPT_P_DHCPDNS); if (!check_dns_option(options, p, msglevel, pull_mode)) @@ -5479,13 +5480,13 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl } gc_free(&options->dns_options.gc); CLEAR(options->dns_options); - *update_options_found |= OPT_P_U_DNS; + options->push_update_options_found |= OPT_P_U_DNS; } } #if defined(_WIN32) || defined(TARGET_ANDROID) else if (streq(p[0], "dhcp-option") && p[1] && !p[3]) { - if (!(*update_options_found & OPT_P_U_DHCP)) + if (!(options->push_update_options_found & OPT_P_U_DHCP)) { struct tuntap_options *o = &options->tuntap_options; VERIFY_PERMISSION(OPT_P_DHCPDNS); @@ -5515,17 +5516,17 @@ update_option(struct context *c, struct options *options, char *p[], bool is_inl o->http_proxy_port = 0; o->http_proxy = NULL; #endif - *update_options_found |= OPT_P_U_DHCP; + options->push_update_options_found |= OPT_P_U_DHCP; } } #else /* if defined(_WIN32) || defined(TARGET_ANDROID) */ else if (streq(p[0], "dhcp-option") && p[1] && !p[3]) { - if (!(*update_options_found & OPT_P_U_DHCP)) + if (!(options->push_update_options_found & OPT_P_U_DHCP)) { VERIFY_PERMISSION(OPT_P_DHCPDNS); delete_all_dhcp_fo(options, &es->list); - *update_options_found |= OPT_P_U_DHCP; + options->push_update_options_found |= OPT_P_U_DHCP; } } #endif /* if defined(_WIN32) || defined(TARGET_ANDROID) */ diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 42db9cae..d3310332 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -558,6 +558,7 @@ struct options bool pull; /* client pull of config options from server */ int push_continuation; unsigned int push_option_types_found; + unsigned int push_update_options_found; /* tracks which option types have been reset in current PUSH_UPDATE sequence */ const char *auth_user_pass_file; bool auth_user_pass_file_inline; struct options_pre_connect *pre_connect; @@ -863,14 +864,11 @@ void remove_option(struct context *c, struct options *options, char *p[], bool i * @param option_types_found A pointer to the variable where the flags corresponding to the options * found are stored. * @param es The environment set structure. - * @param update_options_found A pointer to the variable where the flags corresponding to the update - * options found are stored, used to check if an option of the same type has already been processed - * by update_option() within the same push-update message. */ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found); + struct env_set *es); void parse_argv(struct options *options, const int argc, char *argv[], const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, diff --git a/src/openvpn/options_parse.c b/src/openvpn/options_parse.c index bb5b4049..4cbd903b 100644 --- a/src/openvpn/options_parse.c +++ b/src/openvpn/options_parse.c @@ -517,7 +517,6 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu int line_num = 0; const char *file = "[PUSH-OPTIONS]"; const msglvl_t msglevel = D_PUSH_ERRORS | M_OPTERR; - unsigned int update_options_found = 0; while (buf_parse(buf, ',', line, sizeof(line))) { @@ -564,8 +563,13 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu } else { + /* + * Use persistent push_update_options_found from options struct to track + * which option types have been reset across continuation messages. + * This prevents routes from being reset on each continuation message. + */ update_option(c, options, p, false, file, line_num, 0, msglevel, permission_mask, - option_types_found, es, &update_options_found); + option_types_found, es); } } } diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 7852d360..898d1585 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -540,11 +540,15 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } else { - if (!option_types_found) + /* Clear push_update_options_found for next PUSH_UPDATE sequence */ + c->options.push_update_options_found = 0; + + /* Use accumulated option_types_found for the entire PUSH_UPDATE sequence */ + if (!c->options.push_option_types_found) { msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); } - else if (!do_update(c, option_types_found)) + else if (!do_update(c, c->options.push_option_types_found)) { msg(D_PUSH_ERRORS, "Failed to update options"); goto error; diff --git a/tests/unit_tests/openvpn/test_options_parse.c b/tests/unit_tests/openvpn/test_options_parse.c index 59a3f6df..adf3e5ba 100644 --- a/tests/unit_tests/openvpn/test_options_parse.c +++ b/tests/unit_tests/openvpn/test_options_parse.c @@ -60,7 +60,7 @@ void update_option(struct context *c, struct options *options, char *p[], bool is_inline, const char *file, int line, const int level, const msglvl_t msglevel, const unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es, unsigned int *update_options_found) + struct env_set *es) { } diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 6ef12668..5c16001b 100644 --- a/tests/unit_tests/openvpn/test_push_update_msg.c +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -54,6 +54,20 @@ options_postprocess_pull(struct options *options, struct env_set *es) return true; } +/* + * Counters to track route accumulation across continuation messages. + * Used to verify the bug where update_options_found resets per message. + */ +static int route_reset_count = 0; +static int route_add_count = 0; + +static void +reset_route_counters(void) +{ + route_reset_count = 0; + route_add_count = 0; +} + bool apply_push_options(struct context *c, struct options *options, struct