From patchwork Thu Nov 27 08:51:34 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Moritz Fain X-Patchwork-Id: 4638 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:6c3:b0:7b1:439f:bdf with SMTP id j3csp3936733maw; Thu, 27 Nov 2025 00:52:11 -0800 (PST) X-Forwarded-Encrypted: i=2; AJvYcCVmyzhhoj+UWTJ5a8BN6umkb2rXJPNypRXyK1zlcmh+lyMbgLMaJwLElajj69R5DoNhqlEj8HbLbjg=@openvpn.net X-Google-Smtp-Source: AGHT+IGGI7H6wKvqf/cjQPFkyRntSwFXH2vEm9/KjEPpMtUumCF6YS4VHRZDRl22/LnPLVIdGMxR X-Received: by 2002:a05:6830:679f:b0:7c6:c891:5112 with SMTP id 46e09a7af769-7c798fcd59amr9972058a34.16.1764233531150; Thu, 27 Nov 2025 00:52:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1764233531; cv=none; d=google.com; s=arc-20240605; b=IYRqBfXhrhbpLoJ19Mj8MS7/muLMoaFD907URv44tFbuJfDUVfCMCLnWQsHAnQAa/e 4dp3zjJ/DHMGP4N9TjyBrHntUyWlpskm4f3ynMF6J2lk+5AmOoMNH/07WDcig05B6WTx 4XYXmCTE8EpqN+YF7dmERuxSMTLv1XhpFZn48y+2C7YDV5sBRJR5qNIAY8bA6ST/QLRX VQE/JUim5PzKU4fO82TSsVn4j3N0iCF69ixC4S/nHzIUgWocoVAr84drOBsMQdzSVBrP n0K2sy+kSc/o9taOtPQYJMvnXXqAmaE4UfwFxqLz7umDKRnsx2x/8VNFh0WgZBqu+V4R HRfA== 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=xS7LpnZlN3tNA837ZkjMBAAGEO7KN8j7nR32VwkRfHg=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=B4Yoo//sjn6dnqhnTAN6T0Jo/XJ186weKmmcMTsrsIBSxcPw+xiLQilPjD6TxGM44b /COXhAkfIMdg2kDJUSaQiowJJeU1WlfoEOhqUvOMnyFGhKXEXP+1+Jv0pv+FBpMPV42q CI5yuuZMH2ubJAgPjcmfAaSUyr2QGNZWLcxL41iBINXWnyuQTGJnrmLKCfxgBOhU7Xfc 5AvX/XDQJ9R2o/PlFZjJWAWbLvGkQL4rLG4GBt/n1a+CQYIF59l9p754pla+9AWEbNj8 5CndbFGu3yffi0/va1A6UdBelKsawIvFbpy2XIH15UPqHpn2FJ0O1KGe3G86J7EmTg/J RibA==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@lists.sourceforge.net header.s=beta header.b=P9PP9roL; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=PIk0U0+w; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=IZJW136s; 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 46e09a7af769-7c90fbea9e5si168259a34.166.2025.11.27.00.52.11 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Thu, 27 Nov 2025 00:52:11 -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=P9PP9roL; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=PIk0U0+w; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=IZJW136s; 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=xS7LpnZlN3tNA837ZkjMBAAGEO7KN8j7nR32VwkRfHg=; b=P9PP9roL4NwEJE9A6HHG4U+DfJ cuuj2SOkPti6tzxTsDTmIwGYUvl7uPEv+U7fI41PChD3VxLvhSCvYCG8M2hoYZz/0kxERH1WO6Ty9 v+Wxx/obgNnAuEQQLrdAOxfrwOiwPngHfpXnluDLbsl7IgOBe9Ii1v9dUx16uRNwi6HM=; Received: from [127.0.0.1] (helo=sfs-ml-3.v29.lw.sourceforge.com) by sfs-ml-3.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1vOXjV-0002W5-4x; Thu, 27 Nov 2025 08:52:09 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-3.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1vOXjT-0002Vw-Kg for openvpn-devel@lists.sourceforge.net; Thu, 27 Nov 2025 08:52:07 +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=t0Sq7xUuuzJQUU8ODNMDJNPxjoGVSH6LdGcqbLwbx/k=; b=PIk0U0+wM68bhk1RepwhETBkTE Dvd+J+SPH1E+GDe8HzuuAjEgLdSJCzaFOIzru2UAmR31XvVduMYAaZjqwRDJyXZufFUjIOQ0n9AUU FH4VE1dPJYJnQqoBMckKuw+aAFL1nYPEgNbdKEGfDH0bQ5rnPBOXBYolAoayKhvV7J8s=; 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=t0Sq7xUuuzJQUU8ODNMDJNPxjoGVSH6LdGcqbLwbx/k=; b=I ZJW136sXB++PI83iC47jM1S+Go6njyLdN57wpYNPzM+gxYbWnwysGwCyj0kA+Fv3THLM+Nh3YO3ow GZlUXVxH6jSh8juSrGnDqcIS7ifYHgweOKWAuYsCKSoVllGhescKL5+CsFma2GtQ7T/EMqnIiY0fV XDjKQ8tUHc6laI6A=; Received: from mail-ua1-f54.google.com ([209.85.222.54]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1vOXjS-0005ZO-Qk for openvpn-devel@lists.sourceforge.net; Thu, 27 Nov 2025 08:52:07 +0000 Received: by mail-ua1-f54.google.com with SMTP id a1e0cc1a2514c-93518a78d0aso338233241.3 for ; Thu, 27 Nov 2025 00:52:06 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1764233521; x=1764838321; 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=t0Sq7xUuuzJQUU8ODNMDJNPxjoGVSH6LdGcqbLwbx/k=; b=EUoX22C6A0B7k/njR9S8VpvSLhRyaj7h2QrxwQNXypoox78cF/iLaFYvaidH/vLOtx mfETtWR+p4DrQXQZQP2EhKpJgWkR8hzmMkL4/iNsWckx8Y9S7aJG9Fku3PCQXeDxNZIK Xg3VQPJ9mfk+48G6VEFy1BKLGj2kLwru63596HlfSboKPlzkeRSDA8J6/K6XeMEQ3WJ+ o9FkYHL+mBkwOjSt0tCVSQMfq0TR6JuOR4HLdyC8K+zylAECv+7B6kwaFVy0gkzzKHmN zz4U18i7bc2nAl4rvSmgzeIwpnuwQ4XzAjhUn8EDNd1C/phiFzFn7yqTkOx/U+rSpBdE syqA== X-Gm-Message-State: AOJu0YwX7U8IWJJdkPlyGJKSwhGt5esBNiewgBED6Lx/hNLrrwCcq0C2 zMRrQ95m8suQwCIBBl5Lh4ICPC+NGysSAnGIvuRCnCTBBycmU8zwFl5IciwtzIkTCaf70X5CuHl 6FC91F2nwe88SyuOuxtfpuYnB3nUSbkHQDs6B7nhPWdsqeB9G/sIAUQ== X-Gm-Gg: ASbGncuRB7jh9yr/CW3UanrPPu7CQounShzW+AyXQwdTsfRcLJu2s2z5c+jJTJA20zR loHK3FKRdH44rwURZxoQn62CAS7L/z7n9YrZ3F/WMw6GGPzIzlFPdOdD0LspAKvHiPRENYFiPRI Y0oWbHO4/MCmwMM66I2LTX7cxMF3uipewm/RwxGRd7cQ217wwj/TTYG6CWiNmrboCHqHFZqlJ5B Dpi6ytwc2MSpbShYFGq0zOCzrGvh42VHUBB1NwAIuZa5qRuma58hP50scL/fIFL1moXJg== X-Received: by 2002:a05:6102:3a06:b0:5db:2828:c133 with SMTP id ada2fe7eead31-5e1de1c9734mr7618700137.10.1764233520586; Thu, 27 Nov 2025 00:52:00 -0800 (PST) MIME-Version: 1.0 From: Moritz Fain Date: Thu, 27 Nov 2025 09:51:34 +0100 X-Gm-Features: AWmQ_bkqhsFPub_x4j2o8T3-I3NtJqobikoCNFVzKnVebASYaNUCH3uRdG0Z9GY Message-ID: To: openvpn-devel@lists.sourceforge.net X-Spam-Score: 0.0 (/) X-Spam-Report: Spam detection software, running on the system "sfi-spamd-2.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: 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.0 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- 0.0 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.54 listed in wl.mailspike.net] X-Headers-End: 1vOXjS-0005ZO-Qk Subject: [Openvpn-devel] [PATCH 2/2] Push: 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?1849932938921208434?= X-GMAIL-MSGID: =?utf-8?q?1849932938921208434?= 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 | 1 + src/openvpn/options.h | 1 + src/openvpn/options_parse.c | 9 +- src/openvpn/push.c | 7 +- .../unit_tests/openvpn/test_push_update_msg.c | 101 +++++++++++++++++- 5 files changed, 111 insertions(+), 8 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,18 @@ 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 (is_update && 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++; + } } return true; } @@ -292,6 +320,69 @@ 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 -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: removal + first batch of routes, continuation 2 (more coming) */ + struct buffer buf1 = alloc_buf(512); + const char *msg1 = "PUSH_UPDATE,-route, 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); + + /* + * BUG CHECK: Routes should only be "reset" once (for the entire update sequence). + * Due to the bug where update_options_found is local to each message, + * routes get reset on the first route of EACH message (3 times instead of 1). + * + * When the bug is fixed, this assertion should pass with route_reset_count == 1. + * Currently it will show route_reset_count == 3, exposing the bug. + */ + 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 +694,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 8a97374e..fe4f5814 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -3238,6 +3238,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; } diff --git a/src/openvpn/options.h b/src/openvpn/options.h index bd013dba..ca94b004 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -559,6 +559,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; diff --git a/src/openvpn/options_parse.c b/src/openvpn/options_parse.c index bb5b4049..99d096b0 100644 --- a/src/openvpn/options_parse.c +++ b/src/openvpn/options_parse.c @@ -517,7 +517,12 @@ 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; + + /* + * 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. + */ while (buf_parse(buf, ',', line, sizeof(line))) { @@ -565,7 +570,7 @@ apply_push_options(struct context *c, struct options *options, struct buffer *bu else { update_option(c, options, p, false, file, line_num, 0, msglevel, permission_mask, - option_types_found, es, &update_options_found); + option_types_found, es, &options->push_update_options_found); } } } diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 7852d360..d0601871 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -540,16 +540,19 @@ incoming_push_message(struct context *c, const struct buffer *buffer) } else { - if (!option_types_found) + /* 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; } } + /* Clear push_update_options_found for next PUSH_UPDATE sequence */ + c->options.push_update_options_found = 0; } event_timeout_clear(&c->c2.push_request_interval); event_timeout_clear(&c->c2.wait_for_connect); diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 6ef12668..1a8b1542 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