From: mrbff (C. Review) <ge...@op...> - 2024-11-20 17:42:48
|
Attention is currently required from: flichtenheld, plaisthos. Hello plaisthos, flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to review the following change. Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 611 insertions(+), 82 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/1 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..9dd67bb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 61f6285..e772a54 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /** undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,54 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - bool apply_push_options(struct options *options, struct buffer *buf, @@ -5679,6 +5614,43 @@ { return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} static void set_user_script(struct options *options, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index ee39dbb..5125f79 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /** undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..87282c3 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,176 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '?') + { + *flags |= PUSH_OPT_OPTIONAL; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + else if (c == '-') + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + if (c == '-' && !(*flags & PUSH_OPT_TO_REMOVE)) + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..64ee0d6 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,10 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..71a0372 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No option found"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..52ca715 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,22 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 8040e7b..b006917 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..1b2414c 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,25 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + -ffunction-sections \ + @TEST_CFLAGS@ \ + @CMOCKA_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + @CMOCKA_LIBS@ \ + -L$(top_srcdir)/src/openvpn \ + $(OPTIONAL_CRYPTO_LIBS) + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..8574855 --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, ?-dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, -?dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-MessageType: newchange |
From: mrbff (C. Review) <ge...@op...> - 2024-11-21 11:21:09
|
Attention is currently required from: flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: mrbff <ma...@ma...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 21 Nov 2024 11:20:55 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2024-11-21 13:10:17
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 1: Code-Review-1 (2 comments) File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/f6e72862_6ae239a4 : PS1, Line 5472: if (!apply_pull_filter(options, line)) ``` options.c: In function ‘apply_push_options’: options.c:5472:14: error: too few arguments to function ‘apply_pull_filter’ 5472 | if (!apply_pull_filter(options, line)) | ^~~~~~~~~~~~~~~~~ In file included from options.c:66: options_util.h:34:1: note: declared here 34 | apply_pull_filter(const struct options *o, | ^~~~~~~~~~~~~~~~~ ``` http://gerrit.openvpn.net/c/openvpn/+/808/comment/09d1a5a6_ed8662d7 : PS1, Line 5618: apply_push_options(struct context *c, ``` options.c:5456:1: error: conflicting types for ‘apply_push_options’; have ‘_Bool(struct options *, struct buffer *, unsigned int, unsigned int *, struct env_set *)’ 5456 | apply_push_options(struct options *options, | ^~~~~~~~~~~~~~~~~~ In file included from ssl.h:40, from options.c:45: options.h:875:6: note: previous declaration of ‘apply_push_options’ with type ‘_Bool(struct context *, struct options *, struct buffer *, unsigned int, unsigned int *, struct env_set *, _Bool)’ 875 | bool apply_push_options(struct context *c, | ^~~~~~~~~~~~~~~~~~ ``` -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 1 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Thu, 21 Nov 2024 13:10:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-11-21 18:25:01
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Code-Review-1 by flichtenheld Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 612 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/2 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..9dd67bb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 61f6285..8dea578 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /** undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,84 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5680,6 +5585,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index ee39dbb..5125f79 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /** undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..87282c3 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,176 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '?') + { + *flags |= PUSH_OPT_OPTIONAL; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + else if (c == '-') + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + if (c == '-' && !(*flags & PUSH_OPT_TO_REMOVE)) + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..64ee0d6 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,10 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..71a0372 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No option found"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..52ca715 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,22 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 8040e7b..b006917 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..1b2414c 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,25 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + -ffunction-sections \ + @TEST_CFLAGS@ \ + @CMOCKA_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + @CMOCKA_LIBS@ \ + -L$(top_srcdir)/src/openvpn \ + $(OPTIONAL_CRYPTO_LIBS) + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..8574855 --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, ?-dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, -?dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 2 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-11-21 18:30:29
|
Attention is currently required from: flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 2: (2 comments) File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/63c77cf9_f80c96b7 : PS1, Line 5472: if (!apply_pull_filter(options, line)) > ``` […] Done http://gerrit.openvpn.net/c/openvpn/+/808/comment/b168825e_5aab5102 : PS1, Line 5618: apply_push_options(struct context *c, > ``` […] Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 2 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Thu, 21 Nov 2024 18:30:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2024-11-26 17:31:34
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 3: Code-Review-1 (1 comment) File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/0907cbe2_3aee602b : PS3, Line 932: #define PUF_TYPE_UNDEF 0 /** undefined filter type */ Needs rebase on top of 6c636f5387245ceb4809eac765c4b649709b4aa8 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 26 Nov 2024 17:31:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2024-11-26 17:40:18
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 3: (1 comment) File tests/unit_tests/openvpn/Makefile.am: http://gerrit.openvpn.net/c/openvpn/+/808/comment/09ca4a05_8807ae01 : PS3, Line 14: pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver Please also add this test to CMakeLists.txt -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 3 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 26 Nov 2024 17:40:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-11-27 10:44:05
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review-1 by flichtenheld Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 619 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/4 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..2077963 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c @@ -612,6 +613,7 @@ "test_provider" "test_ssl" "test_user_pass" + "test_push_update_msg" ) if (WIN32) @@ -807,6 +809,16 @@ src/openvpn/env_set.c src/openvpn/run_command.c ) + + target_sources(test_push_update_msg PRIVATE + tests/unit_tests/openvpn/mock_msg.c + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/buffer.c + src/openvpn/platform.c + src/openvpn/push_util.c + src/openvpn/options_util.c + src/openvpn/otime.c + ) if (TARGET test_argv) target_link_options(test_argv PRIVATE -Wl,--wrap=parse_line) diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 043b240..6f97fcd 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,84 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5680,6 +5585,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 6ab92e2..b4d486f 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /** undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..87282c3 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,176 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '?') + { + *flags |= PUSH_OPT_OPTIONAL; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + else if (c == '-') + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + if (c == '-' && !(*flags & PUSH_OPT_TO_REMOVE)) + { + *flags |= PUSH_OPT_TO_REMOVE; + if (str[1]) + { + c = *(++str); + } + else + { + return false; + } + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..64ee0d6 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,10 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..71a0372 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No option found"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..52ca715 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,22 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 1f8eb1e..4b621f1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..ac2241e 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + @TEST_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + -L$(top_srcdir)/src/openvpn + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..8574855 --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, ?-dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, -?dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 4 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-11-27 10:45:49
|
Attention is currently required from: flichtenheld, plaisthos. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 4: (2 comments) File src/openvpn/options.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/4631a9b8_d330a116 : PS3, Line 932: #define PUF_TYPE_UNDEF 0 /** undefined filter type */ > Needs rebase on top of 6c636f5387245ceb4809eac765c4b649709b4aa8 Done File tests/unit_tests/openvpn/Makefile.am: http://gerrit.openvpn.net/c/openvpn/+/808/comment/bbdd5cb5_f48d4c7c : PS3, Line 14: pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver > Please also add this test to CMakeLists. […] Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 4 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Comment-Date: Wed, 27 Nov 2024 10:45:33 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-11-27 11:41:34
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 4: (7 comments) File CMakeLists.txt: http://gerrit.openvpn.net/c/openvpn/+/808/comment/6208a664_8a387af8 : PS4, Line 812: nit: whitespace File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/4862d3bd_9b57ad9f : PS4, Line 2033: int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, nice catch! File src/openvpn/options_util.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/93ec136c_da93606f : PS4, Line 124: check_push_update_option_flags(char **line, unsigned int *flags) Add doxygen what does this function do. http://gerrit.openvpn.net/c/openvpn/+/808/comment/cacb7fd2_2e6844f7 : PS4, Line 138: if (str[1]) below 8 lines are duplicated 3 times. Maybe define a macro and reuse it, and later undefine? Like #define ADVANCE_OR_FAIL(ptr) \ do { \ if (!(ptr)[1]) \ return false; \ c = *(++(ptr)); \ } while (0) http://gerrit.openvpn.net/c/openvpn/+/808/comment/fc0ee113_1823490a : PS4, Line 160: if (c == '-' && !(*flags & PUSH_OPT_TO_REMOVE)) you could remove the "c == '-'" check here and move this whole block under the "if" condition above http://gerrit.openvpn.net/c/openvpn/+/808/comment/76877514_402d6e34 : PS4, Line 221: apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) Let's move doxygen to the header. File src/openvpn/push.h: http://gerrit.openvpn.net/c/openvpn/+/808/comment/811984a2_89dbae3b : PS4, Line 47: int process_incoming_push_update(struct context *c, Doxygen -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 4 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 27 Nov 2024 11:41:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2024-11-28 09:52:59
|
Attention is currently required from: mrbff, plaisthos. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 4: Code-Review-1 (2 comments) File CMakeLists.txt: http://gerrit.openvpn.net/c/openvpn/+/808/comment/cd014878_336b1231 : PS4, Line 817: src/openvpn/platform.c buffer.c and platform.c are always added by foreach loop above. So they are redundant here. File src/openvpn/options.h: http://gerrit.openvpn.net/c/openvpn/+/808/comment/654dd17f_91aed2e0 : PS4, Line 791: #define PUF_TYPE_UNDEF 0 /** undefined filter type */ You didn't resolve the conflict correctly when rebasing. Please copy the new code here. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 4 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Thu, 28 Nov 2024 09:52:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-11-28 15:22:05
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 5: (7 comments) File CMakeLists.txt: http://gerrit.openvpn.net/c/openvpn/+/808/comment/8f5a6f36_8362c01a : PS4, Line 812: > nit: whitespace Done File src/openvpn/init.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/bd8dd262_1158ed00 : PS4, Line 2033: int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, > nice catch! tnx! File src/openvpn/options_util.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/f8c35b78_8f40c7a2 : PS4, Line 124: check_push_update_option_flags(char **line, unsigned int *flags) > Add doxygen what does this function do. Done http://gerrit.openvpn.net/c/openvpn/+/808/comment/a0c306fd_1444ca21 : PS4, Line 138: if (str[1]) > below 8 lines are duplicated 3 times. Maybe define a macro and reuse it, and later undefine? Like […] Clever, i like it, I tried but when the compiler optimizes it, the code gets skipped. Anyway, i simplified and now i use it just 2 times and with a slightly different condition so i hope is good enough http://gerrit.openvpn.net/c/openvpn/+/808/comment/a517e1a9_3a38d88f : PS4, Line 160: if (c == '-' && !(*flags & PUSH_OPT_TO_REMOVE)) > you could remove the "c == '-'" check here and move this whole block under the "if" condition above I'm not entirely sure that's what you meant but I simplified http://gerrit.openvpn.net/c/openvpn/+/808/comment/ada8cdcc_81c709fb : PS4, Line 221: apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) > Let's move doxygen to the header. Done File src/openvpn/push.h: http://gerrit.openvpn.net/c/openvpn/+/808/comment/276f3131_7f2768f9 : PS4, Line 47: int process_incoming_push_update(struct context *c, > Doxygen Done -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 5 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Thu, 28 Nov 2024 15:21:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-11-28 15:28:48
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Code-Review-1 by flichtenheld Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 645 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/5 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..d9daeae 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c @@ -612,6 +613,7 @@ "test_provider" "test_ssl" "test_user_pass" + "test_push_update_msg" ) if (WIN32) @@ -808,6 +810,16 @@ src/openvpn/run_command.c ) + target_sources(test_push_update_msg PRIVATE + tests/unit_tests/openvpn/mock_msg.c + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/buffer.c + src/openvpn/platform.c + src/openvpn/push_util.c + src/openvpn/options_util.c + src/openvpn/otime.c + ) + if (TARGET test_argv) target_link_options(test_argv PRIVATE -Wl,--wrap=parse_line) target_sources(test_argv PRIVATE diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 043b240..6f97fcd 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,84 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5680,6 +5585,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 6ab92e2..b4d486f 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /** undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /** filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /** filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /** filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..405f023 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,174 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +/** + * @brief Checks the formatting and validity of options inside push-update messages. + * + * This function is used in `apply_pull_filter()` to validate and process options + * in push-update messages. It performs the following checks: + * - Determines if the options are updatable. + * - Checks for the presence of the `-` flag, which indicates that the option + * should be removed. + * - Checks for the `?` flag, which marks the option as optional and suppresses + * errors if the client cannot update it. + * - Modifies the original string (`*line`) by removing the `'-'` and `'?'` flags + * after validating them and updating the appropriate flags in the `flags` variable. + * - `-?option`, `-option`, `?option` are valid formats, `?-option` is not a valid format. + * + * @param line A pointer to an option string. This string is the option being validated. + * @param flags A pointer where flags will be stored: + * - `PUSH_OPT_TO_REMOVE`: Set if the `-` flag is present. + * - `PUSH_OPT_OPTIONAL`: Set if the `?` flag is present. + * + * @return true if the flags and option combination are valid. + * @return false if: + * - The `-` and `?` flags are not formatted correctly. + * - The `line` parameter is empty or `NULL`. + * - The `?` flag is absent and the option is not updatable. + */ +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '-') + { + if (!(str)[1]) + { + return false; + } + *flags |= PUSH_OPT_TO_REMOVE; + c = *(++(str)); + } + if (c == '?') + { + if (!(str)[1] || (str)[1] == '-') + { + return false; + } + *flags |= PUSH_OPT_OPTIONAL; + c = *(++(str)); + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..0825bfa 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,18 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..71a0372 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No option found"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..b930c6c 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,42 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +/** + * @brief Handles the receiving of a push-update message and applies updates to the specified options. + * + * This function processes a push-update message, validating its content and applying updates + * to the options specified in the message. It also handles split messages if the complete + * message has not yet been received. + * + * @param c The context for the operation. + * @param permission_mask The permission mask specifying which options are allowed to be pulled. + * @param option_types_found A pointer to a variable that will be filled with the types of options + * found in the message. + * @param buf A buffer containing the received message. + * + * @return + * - `PUSH_MSG_UPDATE`: The message was processed successfully, and the updates were applied. + * - `PUSH_MSG_CONTINUATION`: The message is a fragment of a larger message, and the program is + * waiting for the final part. + * - `PUSH_MSG_ERROR`: An error occurred during message processing, or the message is invalid. + */ + +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 1f8eb1e..4b621f1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..ac2241e 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + @TEST_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + -L$(top_srcdir)/src/openvpn + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..d0876bc --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -?dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, ?-dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 5 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-11-28 15:57:22
|
Attention is currently required from: flichtenheld, plaisthos, stipa. Hello flichtenheld, plaisthos, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#6). Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 643 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/6 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..cbce2a1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c @@ -612,6 +613,7 @@ "test_provider" "test_ssl" "test_user_pass" + "test_push_update_msg" ) if (WIN32) @@ -808,6 +810,14 @@ src/openvpn/run_command.c ) + target_sources(test_push_update_msg PRIVATE + tests/unit_tests/openvpn/mock_msg.c + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/push_util.c + src/openvpn/options_util.c + src/openvpn/otime.c + ) + if (TARGET test_argv) target_link_options(test_argv PRIVATE -Wl,--wrap=parse_line) target_sources(test_argv PRIVATE diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 043b240..6f97fcd 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,84 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5680,6 +5585,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 6ab92e2..253ddc3 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..405f023 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,174 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +/** + * @brief Checks the formatting and validity of options inside push-update messages. + * + * This function is used in `apply_pull_filter()` to validate and process options + * in push-update messages. It performs the following checks: + * - Determines if the options are updatable. + * - Checks for the presence of the `-` flag, which indicates that the option + * should be removed. + * - Checks for the `?` flag, which marks the option as optional and suppresses + * errors if the client cannot update it. + * - Modifies the original string (`*line`) by removing the `'-'` and `'?'` flags + * after validating them and updating the appropriate flags in the `flags` variable. + * - `-?option`, `-option`, `?option` are valid formats, `?-option` is not a valid format. + * + * @param line A pointer to an option string. This string is the option being validated. + * @param flags A pointer where flags will be stored: + * - `PUSH_OPT_TO_REMOVE`: Set if the `-` flag is present. + * - `PUSH_OPT_OPTIONAL`: Set if the `?` flag is present. + * + * @return true if the flags and option combination are valid. + * @return false if: + * - The `-` and `?` flags are not formatted correctly. + * - The `line` parameter is empty or `NULL`. + * - The `?` flag is absent and the option is not updatable. + */ +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '-') + { + if (!(str)[1]) + { + return false; + } + *flags |= PUSH_OPT_TO_REMOVE; + c = *(++(str)); + } + if (c == '?') + { + if (!(str)[1] || (str)[1] == '-') + { + return false; + } + *flags |= PUSH_OPT_OPTIONAL; + c = *(++(str)); + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..0825bfa 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,18 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..71a0372 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No option found"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..b930c6c 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,42 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +/** + * @brief Handles the receiving of a push-update message and applies updates to the specified options. + * + * This function processes a push-update message, validating its content and applying updates + * to the options specified in the message. It also handles split messages if the complete + * message has not yet been received. + * + * @param c The context for the operation. + * @param permission_mask The permission mask specifying which options are allowed to be pulled. + * @param option_types_found A pointer to a variable that will be filled with the types of options + * found in the message. + * @param buf A buffer containing the received message. + * + * @return + * - `PUSH_MSG_UPDATE`: The message was processed successfully, and the updates were applied. + * - `PUSH_MSG_CONTINUATION`: The message is a fragment of a larger message, and the program is + * waiting for the final part. + * - `PUSH_MSG_ERROR`: An error occurred during message processing, or the message is invalid. + */ + +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 1f8eb1e..4b621f1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..ac2241e 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + @TEST_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + -L$(top_srcdir)/src/openvpn + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..d0876bc --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -?dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, ?-dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 6 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-MessageType: newpatchset |
From: mrbff (C. Review) <ge...@op...> - 2024-11-28 15:57:45
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 5: (2 comments) File CMakeLists.txt: http://gerrit.openvpn.net/c/openvpn/+/808/comment/d57bba5b_c9eabd40 : PS4, Line 817: src/openvpn/platform.c > buffer.c and platform.c are always added by foreach loop above. So they are redundant here. Done File src/openvpn/options.h: http://gerrit.openvpn.net/c/openvpn/+/808/comment/17c2f5d7_5fb82784 : PS4, Line 791: #define PUF_TYPE_UNDEF 0 /** undefined filter type */ > You didn't resolve the conflict correctly when rebasing. Please copy the new code here. you are right, sorry! -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 5 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-CC: stipa <lst...@gm...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Thu, 28 Nov 2024 15:57:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: flichtenheld <fr...@li...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-11-29 10:40:11
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 6: Code-Review-1 (2 comments) Patchset: PS6: I did a quick test and following doesn't work: 2024-11-29 11:11:30 us=656000 PUSH: Received control message: 'PUSH_UPDATE,ifconfig 10.8.0.4 255.255.255.0' 2024-11-29 11:11:30 us=656000 No option found 2024-11-29 11:12:48 us=265000 Closing ovpn-dco interface 2024-11-29 11:12:48 us=265000 NETSH: C:\Windows\system32\netsh.exe interface ipv4 delete dns 20 all 2024-11-29 11:12:48 us=421000 NETSH: C:\Windows\system32\netsh.exe interface ipv4 delete address 20 10.8.0.2 store=active 2024-11-29 11:12:48 us=609000 SIGTERM[hard,] received, process exiting File src/openvpn/push.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/8d650f48_e00d63e5 : PS6, Line 543: msg(M_WARN, "No option found"); This warning is a bit confusing. 2024-11-29 10:58:57 us=140000 PUSH: Received control message: 'PUSH_UPDATE,-dns' 2024-11-29 10:58:57 us=140000 No option found It is unclear what exactly does it mean and what user/admin is supposed to do with it. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 6 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Fri, 29 Nov 2024 10:39:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-11-29 17:15:14
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 6: (1 comment) File src/openvpn/push.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/d1a9ecd3_e048bce9 : PS6, Line 543: msg(M_WARN, "No option found"); > This warning is a bit confusing. […] I put it to avoid that in case of a correct message but without updates (something like "PUSH_UPDATE,," or "PUSH_UPDATE,?something_not_updatable") there was a useless closing and reopening of the tun, but it is not a real error message, it is more of an info. I'm not even sure it is the correct way to handle it, what do you suggest? -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 6 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Fri, 29 Nov 2024 17:14:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: stipa (C. Review) <ge...@op...> - 2024-12-03 10:43:04
|
Attention is currently required from: flichtenheld, mrbff, plaisthos. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 6: (1 comment) File src/openvpn/push.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/b5d51f26_68cd5be8 : PS6, Line 543: msg(M_WARN, "No option found"); > I put it to avoid that in case of a correct message but without updates (something like "PUSH_UPDATE […] How about "No updatable options found in incoming PUSH_UPDATE message" - I think it is verbose enough to let user/admin know what has happened. -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 6 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Tue, 03 Dec 2024 10:42:48 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: mrbff <ma...@ma...> Comment-In-Reply-To: stipa <lst...@gm...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 11:06:48
|
Attention is currently required from: flichtenheld, plaisthos, stipa. mrbff has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 6: (1 comment) File src/openvpn/push.c: http://gerrit.openvpn.net/c/openvpn/+/808/comment/29711b30_ad673ff1 : PS6, Line 543: msg(M_WARN, "No option found"); > How about "No updatable options found in incoming PUSH_UPDATE message" - I think it is verbose enoug […] Ok then! -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 6 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: plaisthos <arn...@rf...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Comment-Date: Tue, 03 Dec 2024 11:06:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: stipa <lst...@gm...> Comment-In-Reply-To: mrbff <ma...@ma...> Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 11:12:38
|
mrbff has abandoned this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Abandoned -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: abandon |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 11:12:54
|
mrbff has restored this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Restored -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 7 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-MessageType: restore |
From: mrbff (C. Review) <ge...@op...> - 2024-12-03 11:17:57
|
Attention is currently required from: stipa. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#8). The following approvals got outdated and were removed: Code-Review-1 by stipa Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 643 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/8 diff --git a/CMakeLists.txt b/CMakeLists.txt index 5db207d..cbce2a1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -496,6 +496,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c @@ -612,6 +613,7 @@ "test_provider" "test_ssl" "test_user_pass" + "test_push_update_msg" ) if (WIN32) @@ -808,6 +810,14 @@ src/openvpn/run_command.c ) + target_sources(test_push_update_msg PRIVATE + tests/unit_tests/openvpn/mock_msg.c + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/push_util.c + src/openvpn/options_util.c + src/openvpn/otime.c + ) + if (TARGET test_argv) target_link_options(test_argv PRIVATE -Wl,--wrap=parse_line) target_sources(test_argv PRIVATE diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index ecb2bcf..55ec565 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -114,7 +114,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index d50b24c..2c75953 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2031,7 +2031,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 9371024..f8d9d06 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2030,8 +2030,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 043b240..6f97fcd 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -63,6 +63,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -927,24 +928,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5469,84 +5452,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5680,6 +5585,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 6ab92e2..253ddc3 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -777,6 +777,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -845,11 +872,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index a9e020a..405f023 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -98,3 +100,174 @@ gc_free(&gc); return message; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +/** + * @brief Checks the formatting and validity of options inside push-update messages. + * + * This function is used in `apply_pull_filter()` to validate and process options + * in push-update messages. It performs the following checks: + * - Determines if the options are updatable. + * - Checks for the presence of the `-` flag, which indicates that the option + * should be removed. + * - Checks for the `?` flag, which marks the option as optional and suppresses + * errors if the client cannot update it. + * - Modifies the original string (`*line`) by removing the `'-'` and `'?'` flags + * after validating them and updating the appropriate flags in the `flags` variable. + * - `-?option`, `-option`, `?option` are valid formats, `?-option` is not a valid format. + * + * @param line A pointer to an option string. This string is the option being validated. + * @param flags A pointer where flags will be stored: + * - `PUSH_OPT_TO_REMOVE`: Set if the `-` flag is present. + * - `PUSH_OPT_OPTIONAL`: Set if the `?` flag is present. + * + * @return true if the flags and option combination are valid. + * @return false if: + * - The `-` and `?` flags are not formatted correctly. + * - The `line` parameter is empty or `NULL`. + * - The `?` flag is absent and the option is not updatable. + */ +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '-') + { + if (!(str)[1]) + { + return false; + } + *flags |= PUSH_OPT_TO_REMOVE; + c = *(++(str)); + } + if (c == '?') + { + if (!(str)[1] || (str)[1] == '-') + { + return false; + } + *flags |= PUSH_OPT_OPTIONAL; + c = *(++(str)); + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index 29bc9dc..0825bfa 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -30,4 +30,18 @@ const char * parse_auth_failed_temp(struct options *o, const char *reason); +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 6c06374..2afd539 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1048,11 +1056,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1103,6 +1113,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 4a13327..b930c6c 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,42 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +/** + * @brief Handles the receiving of a push-update message and applies updates to the specified options. + * + * This function processes a push-update message, validating its content and applying updates + * to the options specified in the message. It also handles split messages if the complete + * message has not yet been received. + * + * @param c The context for the operation. + * @param permission_mask The permission mask specifying which options are allowed to be pulled. + * @param option_types_found A pointer to a variable that will be filled with the types of options + * found in the message. + * @param buf A buffer containing the received message. + * + * @return + * - `PUSH_MSG_UPDATE`: The message was processed successfully, and the updates were applied. + * - `PUSH_MSG_CONTINUATION`: The message is a fragment of a larger message, and the program is + * waiting for the final part. + * - `PUSH_MSG_ERROR`: An error occurred during message processing, or the message is invalid. + */ + +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 1f8eb1e..4b621f1 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1947,6 +1947,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index eea1323..53b7f2c 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -110,6 +110,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index a4e6235..ac2241e 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -307,3 +307,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + @TEST_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + -L$(top_srcdir)/src/openvpn + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..d0876bc --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -?dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, ?-dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 8 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-MessageType: newpatchset |
From: stipa (C. Review) <ge...@op...> - 2025-01-22 09:14:15
|
Attention is currently required from: mrbff. stipa has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 11: Code-Review+2 -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 11 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 22 Jan 2025 09:13:59 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: flichtenheld (C. Review) <ge...@op...> - 2025-03-05 13:24:25
|
Attention is currently required from: mrbff. flichtenheld has posted comments on this change. ( http://gerrit.openvpn.net/c/openvpn/+/808?usp=email ) Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... Patch Set 11: Code-Review-1 (1 comment) Patchset: PS11: Please rebase to solve conflict with 40518dc66d9d04e6ec7e04439d7a6bc7fd6ac20f in options_util.[ch] -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 11 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-Comment-Date: Wed, 05 Mar 2025 12:56:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment |
From: mrbff (C. Review) <ge...@op...> - 2025-03-21 04:57:57
|
Attention is currently required from: flichtenheld, mrbff, stipa. Hello flichtenheld, plaisthos, stipa, I'd like you to reexamine a change. Please visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email to look at the new patch set (#12). The following approvals got outdated and were removed: Code-Review+2 by stipa, Code-Review-1 by flichtenheld The change is no longer submittable: Code-Review and checks~ChecksSubmitRule are unsatisfied now. Change subject: PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. ...................................................................... PUSH_UPDATE: Allow OpenVPN in client mode to receive and handle PUSH UPDATE control messages to allow options updating at runtime. * Added IV_PROTO_PUSH_UPDATE flag bit to support push-updates. * Added process_incoming_push_update(), in a separate file to create tests more easily. * Modified incoming_push_message(), process_incoming_push_msg(), apply_push_options(), apply_pull_filter() to process also push-update messages. * Added the check_push_update_option_flags() function used in apply_pull_filter() to check options formatting inside push-update messages, if the options are updatables and to check for '?' and '-' flags that may be present in front of the options. The '-' flag is used to indicate that the option in question should be removed, while the '?' indicates that the option is optional and to do not generate errors if the client cannot update that option. For more info you can read the RFC at https://github.com/OpenVPN/openvpn-rfc . * Created some unit tests for the push-update message handling in test_push_update_msg.c. Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Signed-off-by: Marco Baffo <ma...@ma...> --- M CMakeLists.txt M src/openvpn/Makefile.am M src/openvpn/forward.c M src/openvpn/init.c M src/openvpn/options.c M src/openvpn/options.h M src/openvpn/options_util.c M src/openvpn/options_util.h M src/openvpn/push.c M src/openvpn/push.h A src/openvpn/push_util.c M src/openvpn/ssl.c M src/openvpn/ssl.h M tests/unit_tests/openvpn/Makefile.am A tests/unit_tests/openvpn/test_push_update_msg.c 15 files changed, 643 insertions(+), 112 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/08/808/12 diff --git a/CMakeLists.txt b/CMakeLists.txt index b04adce..f597a89 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -506,6 +506,7 @@ src/openvpn/ps.c src/openvpn/ps.h src/openvpn/push.c + src/openvpn/push_util.c src/openvpn/push.h src/openvpn/pushlist.h src/openvpn/reflect_filter.c @@ -622,6 +623,7 @@ "test_provider" "test_ssl" "test_user_pass" + "test_push_update_msg" ) if (WIN32) @@ -824,6 +826,14 @@ src/openvpn/run_command.c ) + target_sources(test_push_update_msg PRIVATE + tests/unit_tests/openvpn/mock_msg.c + tests/unit_tests/openvpn/mock_get_random.c + src/openvpn/push_util.c + src/openvpn/options_util.c + src/openvpn/otime.c + ) + if (TARGET test_argv) target_link_options(test_argv PRIVATE -Wl,--wrap=parse_line) target_sources(test_argv PRIVATE diff --git a/src/openvpn/Makefile.am b/src/openvpn/Makefile.am index 37af683..e349282 100644 --- a/src/openvpn/Makefile.am +++ b/src/openvpn/Makefile.am @@ -116,7 +116,7 @@ proto.c proto.h \ proxy.c proxy.h \ ps.c ps.h \ - push.c push.h \ + push.c push_util.c push.h \ pushlist.h \ reflect_filter.c reflect_filter.h \ reliable.c reliable.h \ diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index af1d008..59df966 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -2038,7 +2038,7 @@ } /* check for incoming control messages on the control channel like - * push request/reply, or authentication failure and 2FA messages */ + * push request/reply/update, or authentication failure and 2FA messages */ if (tls_test_payload_len(c->c2.tls_multi) > 0) { check_incoming_control_channel(c); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 1be205b..cd94e6e 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2048,8 +2048,8 @@ /* possibly add routes */ if ((route_order(c->c1.tuntap) == ROUTE_AFTER_TUN) && (!c->options.route_delay_defined)) { - int status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, - c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); + bool status = do_route(&c->options, c->c1.route_list, c->c1.route_ipv6_list, + c->c1.tuntap, c->plugins, c->c2.es, &c->net_ctx); *error_flags |= (status ? 0 : ISC_ROUTE_ERRORS); } diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 67ef55b..bf29c56 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -65,6 +65,7 @@ #include <ctype.h> #include "memdbg.h" +#include "options_util.h" const char title_string[] = PACKAGE_STRING @@ -941,24 +942,6 @@ } } -struct pull_filter -{ -#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ -#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ -#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ -#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ - int type; - int size; - char *pattern; - struct pull_filter *next; -}; - -struct pull_filter_list -{ - struct pull_filter *head; - struct pull_filter *tail; -}; - #ifndef ENABLE_SMALL static const char * @@ -5433,84 +5416,6 @@ } } -/** - * Filter an option line by all pull filters. - * - * If a match is found, the line is modified depending on - * the filter type, and returns true. If the filter type is - * reject, SIGUSR1 is triggered and the return value is false. - * In that case the caller must end the push processing. - */ -static bool -apply_pull_filter(const struct options *o, char *line) -{ - struct pull_filter *f; - - if (!o->pull_filter_list) - { - return true; - } - - /* skip leading spaces matching the behaviour of parse_line */ - while (isspace(*line)) - { - line++; - } - - for (f = o->pull_filter_list->head; f; f = f->next) - { - if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_LOW, "Pushed option accepted by filter: '%s'", line); - return true; - } - else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) - { - msg(D_PUSH, "Pushed option removed by filter: '%s'", line); - *line = '\0'; - return true; - } - else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) - { - msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); - *line = '\0'; - throw_signal_soft(SIGUSR1, "Offending option received from server"); - return false; - } - } - return true; -} - -bool -apply_push_options(struct options *options, - struct buffer *buf, - unsigned int permission_mask, - unsigned int *option_types_found, - struct env_set *es) -{ - char line[OPTION_PARM_SIZE]; - int line_num = 0; - const char *file = "[PUSH-OPTIONS]"; - const int msglevel = D_PUSH_ERRORS|M_OPTERR; - - while (buf_parse(buf, ',', line, sizeof(line))) - { - char *p[MAX_PARMS+1]; - CLEAR(p); - ++line_num; - if (!apply_pull_filter(options, line)) - { - return false; /* Cause push/pull error and stop push processing */ - } - if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) - { - add_option(options, p, false, file, line_num, 0, msglevel, - permission_mask, option_types_found, es); - } - } - return true; -} - void options_server_import(struct options *o, const char *filename, @@ -5644,6 +5549,44 @@ return options->forward_compatible ? M_WARN : msglevel; } +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + int line_num = 0; + const char *file = "[PUSH-OPTIONS]"; + const int msglevel = D_PUSH_ERRORS|M_OPTERR; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + char *p[MAX_PARMS+1]; + CLEAR(p); + ++line_num; + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + throw_signal_soft(SIGUSR1, "Offending option received from server"); + return false; /* Cause push/pull error and stop push processing */ + } + if (parse_line(line, p, SIZE(p)-1, file, line_num, msglevel, &options->gc)) + { + if (!is_update) + { + add_option(options, p, false, file, line_num, 0, msglevel, + permission_mask, option_types_found, es); + } + } + } + return true; +} + static void set_user_script(struct options *options, const char **script, diff --git a/src/openvpn/options.h b/src/openvpn/options.h index fa617c8..cb3a589 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -793,6 +793,33 @@ #define MAN_CLIENT_AUTH_ENABLED(opt) (false) #endif +/* + * some PUSH_UPDATE options + */ +#define OPT_P_U_ROUTE (1<<0) +#define OPT_P_U_ROUTE6 (1<<1) +#define OPT_P_U_DNS (1<<2) +#define OPT_P_U_DHCP (1<<3) +#define OPT_P_U_REDIR_GATEWAY (1<<4) + +struct pull_filter +{ +#define PUF_TYPE_UNDEF 0 /**< undefined filter type */ +#define PUF_TYPE_ACCEPT 1 /**< filter type to accept a matching option */ +#define PUF_TYPE_IGNORE 2 /**< filter type to ignore a matching option */ +#define PUF_TYPE_REJECT 3 /**< filter type to reject and trigger SIGUSR1 */ + int type; + int size; + char *pattern; + struct pull_filter *next; +}; + +struct pull_filter_list +{ + struct pull_filter *head; + struct pull_filter *tail; +}; + void parse_argv(struct options *options, const int argc, char *argv[], @@ -861,11 +888,13 @@ void pre_connect_restore(struct options *o, struct gc_arena *gc); -bool apply_push_options(struct options *options, +bool apply_push_options(struct context *c, + struct options *options, struct buffer *buf, unsigned int permission_mask, unsigned int *option_types_found, - struct env_set *es); + struct env_set *es, + bool is_update); void options_detach(struct options *o); diff --git a/src/openvpn/options_util.c b/src/openvpn/options_util.c index fea4c3d..48d53a4 100644 --- a/src/openvpn/options_util.c +++ b/src/openvpn/options_util.c @@ -30,6 +30,8 @@ #include "options_util.h" +#include "push.h" + const char * parse_auth_failed_temp(struct options *o, const char *reason) { @@ -145,3 +147,174 @@ return (int) i; } + +static const char *updatable_options[] = { + "block-ipv6", + "block-outside-dns", + "dhcp-option", + "dns", + "ifconfig", + "ifconfig-ipv6", + "push-continuation", + "redirect-gateway", + "redirect-private", + "route", + "route-gateway", + "route-ipv6", + "route-metric", + "topology", + "tun-mtu", + "keepalive" +}; + +/** + * @brief Checks the formatting and validity of options inside push-update messages. + * + * This function is used in `apply_pull_filter()` to validate and process options + * in push-update messages. It performs the following checks: + * - Determines if the options are updatable. + * - Checks for the presence of the `-` flag, which indicates that the option + * should be removed. + * - Checks for the `?` flag, which marks the option as optional and suppresses + * errors if the client cannot update it. + * - Modifies the original string (`*line`) by removing the `'-'` and `'?'` flags + * after validating them and updating the appropriate flags in the `flags` variable. + * - `-?option`, `-option`, `?option` are valid formats, `?-option` is not a valid format. + * + * @param line A pointer to an option string. This string is the option being validated. + * @param flags A pointer where flags will be stored: + * - `PUSH_OPT_TO_REMOVE`: Set if the `-` flag is present. + * - `PUSH_OPT_OPTIONAL`: Set if the `?` flag is present. + * + * @return true if the flags and option combination are valid. + * @return false if: + * - The `-` and `?` flags are not formatted correctly. + * - The `line` parameter is empty or `NULL`. + * - The `?` flag is absent and the option is not updatable. + */ +static bool +check_push_update_option_flags(char **line, unsigned int *flags) +{ + if (!line || !*line || !**line) + { + return false; + } + *flags = 0; + bool opt_is_updatable = false; + char *str = *line; + char c = **line; + + if (c == '-') + { + if (!(str)[1]) + { + return false; + } + *flags |= PUSH_OPT_TO_REMOVE; + c = *(++(str)); + } + if (c == '?') + { + if (!(str)[1] || (str)[1] == '-') + { + return false; + } + *flags |= PUSH_OPT_OPTIONAL; + c = *(++(str)); + } + + /* Skip '?' and '-' if they are present */ + size_t len = strlen(str); + memmove(*line, str, len); + (*line)[len] = '\0'; + str = *line; + + int count = sizeof(updatable_options)/sizeof(char *); + for (int i = 0; i < count; ++i) + { + size_t opt_len = strlen(updatable_options[i]); + if (len < opt_len) + { + continue; + } + if (!strncmp(str, updatable_options[i], opt_len) + && (!str[opt_len] || str[opt_len] == ' ')) + { + opt_is_updatable = true; + break; + } + } + + if (!opt_is_updatable) + { + if (*flags & PUSH_OPT_OPTIONAL) + { + msg(D_PUSH, "Pushed option is not updatable: '%s'", *line); + return true; + } + else + { + msg(M_WARN, "Pushed option is not updatable: '%s'. Restarting.", *line); + return false; + } + } + + return true; +} + +bool +apply_pull_filter(const struct options *o, char *line, bool is_update, unsigned int *push_update_option_flags) +{ + struct pull_filter *f; + + if (!o->pull_filter_list && !(is_update)) + { + return true; + } + + /* skip leading spaces matching the behaviour of parse_line */ + while (isspace(*line)) + { + line++; + } + + if (is_update) + { + if (!check_push_update_option_flags(&line, push_update_option_flags)) + { + return false; + } + if (!o->pull_filter_list) + { + return true; + } + } + + for (f = o->pull_filter_list->head; f; f = f->next) + { + if (f->type == PUF_TYPE_ACCEPT && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_LOW, "Pushed option accepted by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_IGNORE && strncmp(line, f->pattern, f->size) == 0) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else if (f->type == PUF_TYPE_REJECT && strncmp(line, f->pattern, f->size) == 0) + { + if (is_update && (*push_update_option_flags & PUSH_OPT_OPTIONAL)) + { + msg(D_PUSH, "Pushed option removed by filter: '%s'", line); + return true; + } + else + { + msg(M_WARN, "Pushed option rejected by filter: '%s'. Restarting.", line); + return false; + } + } + } + return true; +} diff --git a/src/openvpn/options_util.h b/src/openvpn/options_util.h index f34d997..c5f62db 100644 --- a/src/openvpn/options_util.h +++ b/src/openvpn/options_util.h @@ -50,4 +50,18 @@ int atoi_warn(const char *str, int msglevel); +/** + * Filter an option line by all pull filters. + * + * If a match is found, the line is modified depending on + * the filter type, and returns true. If the filter type is + * reject, SIGUSR1 is triggered and the return value is false. + * In that case the caller must end the push processing. + */ +bool +apply_pull_filter(const struct options *o, + char *line, + bool is_update, + unsigned int *push_update_option_flags); + #endif /* ifndef OPTIONS_UTIL_H_ */ diff --git a/src/openvpn/push.c b/src/openvpn/push.c index c97ce28..a40a28e 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -39,8 +39,6 @@ #include "ssl_util.h" #include "options_util.h" -static char push_reply_cmd[] = "PUSH_REPLY"; - /* * Auth username/password * @@ -519,21 +517,31 @@ { msg(D_PUSH_ERRORS, "WARNING: Received bad push/pull message: %s", sanitize_control_message(BSTR(buffer), &gc)); } - else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_CONTINUATION) + else if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE || status == PUSH_MSG_CONTINUATION) { c->options.push_option_types_found |= option_types_found; /* delay bringing tun/tap up until --push parms received from remote */ - if (status == PUSH_MSG_REPLY) + if (status == PUSH_MSG_REPLY || status == PUSH_MSG_UPDATE) { if (!options_postprocess_pull(&c->options, c->c2.es)) { goto error; } - if (!do_up(c, true, c->options.push_option_types_found)) + if (status == PUSH_MSG_REPLY) { - msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); - goto error; + if (!do_up(c, true, c->options.push_option_types_found)) + { + msg(D_PUSH_ERRORS, "Failed to open tun/tap interface"); + goto error; + } + } + else + { + if (!option_types_found) + { + msg(M_WARN, "No updatable options found in incoming PUSH_UPDATE message"); + } } } event_timeout_clear(&c->c2.push_request_interval); @@ -1058,11 +1066,13 @@ md_ctx_init(c->c2.pulled_options_state, "SHA256"); c->c2.pulled_options_digest_init_done = true; } - if (apply_push_options(&c->options, + if (apply_push_options(c, + &c->options, buf, permission_mask, option_types_found, - c->c2.es)) + c->c2.es, + false)) { push_update_digest(c->c2.pulled_options_state, &buf_orig, &c->options); @@ -1113,6 +1123,12 @@ return process_incoming_push_reply(c, permission_mask, option_types_found, &buf); } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } else { return PUSH_MSG_ERROR; diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 6af0853..4643c36 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -33,9 +33,42 @@ #define PUSH_MSG_AUTH_FAILURE 4 #define PUSH_MSG_CONTINUATION 5 #define PUSH_MSG_ALREADY_REPLIED 6 +#define PUSH_MSG_UPDATE 7 + +#define push_reply_cmd "PUSH_REPLY" +#define push_update_cmd "PUSH_UPDATE" + +/* Push-update options flags */ +#define PUSH_OPT_TO_REMOVE (1<<0) +#define PUSH_OPT_OPTIONAL (1<<1) int process_incoming_push_request(struct context *c); +/** + * @brief Handles the receiving of a push-update message and applies updates to the specified options. + * + * This function processes a push-update message, validating its content and applying updates + * to the options specified in the message. It also handles split messages if the complete + * message has not yet been received. + * + * @param c The context for the operation. + * @param permission_mask The permission mask specifying which options are allowed to be pulled. + * @param option_types_found A pointer to a variable that will be filled with the types of options + * found in the message. + * @param buf A buffer containing the received message. + * + * @return + * - `PUSH_MSG_UPDATE`: The message was processed successfully, and the updates were applied. + * - `PUSH_MSG_CONTINUATION`: The message is a fragment of a larger message, and the program is + * waiting for the final part. + * - `PUSH_MSG_ERROR`: An error occurred during message processing, or the message is invalid. + */ + +int process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf); + int process_incoming_push_msg(struct context *c, const struct buffer *buffer, bool honor_received_options, diff --git a/src/openvpn/push_util.c b/src/openvpn/push_util.c new file mode 100644 index 0000000..b4d1e8b --- /dev/null +++ b/src/openvpn/push_util.c @@ -0,0 +1,44 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include "push.h" + +int +process_incoming_push_update(struct context *c, + unsigned int permission_mask, + unsigned int *option_types_found, + struct buffer *buf) +{ + int ret = PUSH_MSG_ERROR; + const uint8_t ch = buf_read_u8(buf); + if (ch == ',') + { + if (apply_push_options(c, + &c->options, + buf, + permission_mask, + option_types_found, + c->c2.es, + true)) + { + switch (c->options.push_continuation) + { + case 0: + case 1: + ret = PUSH_MSG_UPDATE; + break; + + case 2: + ret = PUSH_MSG_CONTINUATION; + break; + } + } + } + else if (ch == '\0') + { + ret = PUSH_MSG_UPDATE; + } + + return ret; +} diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 99b5c07..1926691 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1990,6 +1990,9 @@ /* support for exit notify via control channel */ iv_proto |= IV_PROTO_CC_EXIT_NOTIFY; + /* support push-updates */ + iv_proto |= IV_PROTO_PUSH_UPDATE; + if (session->opt->pull) { /* support for receiving push_reply before sending diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index c32cb6c..334b068 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -114,6 +114,9 @@ /** Supports the --dns option after all the incompatible changes */ #define IV_PROTO_DNS_OPTION_V2 (1<<11) +/** Supports push-update */ +#define IV_PROTO_PUSH_UPDATE (1<<12) + /* Default field in X509 to be username */ #define X509_USERNAME_FIELD_DEFAULT "CN" diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 471389b..6fb7843 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -11,7 +11,7 @@ endif test_binaries += crypto_testdriver packet_id_testdriver auth_token_testdriver ncp_testdriver misc_testdriver \ - pkt_testdriver ssl_testdriver user_pass_testdriver + pkt_testdriver ssl_testdriver user_pass_testdriver push_update_msg_testdriver if HAVE_LD_WRAP_SUPPORT if !WIN32 @@ -314,3 +314,21 @@ $(top_srcdir)/src/openvpn/win32-util.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/list.c + +push_update_msg_testdriver_CFLAGS = -I$(top_srcdir)/src/openvpn \ + -I$(top_srcdir)/src/compat \ + -I$(top_srcdir)/tests/unit_tests/openvpn \ + @TEST_CFLAGS@ + +push_update_msg_testdriver_LDFLAGS = \ + @TEST_LDFLAGS@ \ + -L$(top_srcdir)/src/openvpn + +push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ + mock_msg.c \ + mock_get_random.c \ + $(top_srcdir)/src/openvpn/buffer.c \ + $(top_srcdir)/src/openvpn/platform.c \ + $(top_srcdir)/src/openvpn/push_util.c \ + $(top_srcdir)/src/openvpn/options_util.c \ + $(top_srcdir)/src/openvpn/otime.c \ No newline at end of file diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c new file mode 100644 index 0000000..d0876bc --- /dev/null +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -0,0 +1,245 @@ +#ifdef HAVE_CONFIG_H +#include "config.h" +#endif + +#include <stdlib.h> +#include <stdarg.h> +#include <setjmp.h> +#include <cmocka.h> +#include "push.h" +#include "options_util.h" + +/* mocks */ + +unsigned int +pull_permission_mask(const struct context *c) +{ + unsigned int flags = + OPT_P_UP + | OPT_P_ROUTE_EXTRAS + | OPT_P_SOCKBUF + | OPT_P_SOCKFLAGS + | OPT_P_SETENV + | OPT_P_SHAPER + | OPT_P_TIMER + | OPT_P_COMP + | OPT_P_PERSIST + | OPT_P_MESSAGES + | OPT_P_EXPLICIT_NOTIFY + | OPT_P_ECHO + | OPT_P_PULL_MODE + | OPT_P_PEER_ID + | OPT_P_NCP + | OPT_P_PUSH_MTU + | OPT_P_ROUTE + | OPT_P_DHCPDNS; + return flags; +} + +bool +apply_push_options(struct context *c, + struct options *options, + struct buffer *buf, + unsigned int permission_mask, + unsigned int *option_types_found, + struct env_set *es, + bool is_update) +{ + char line[OPTION_PARM_SIZE]; + + while (buf_parse(buf, ',', line, sizeof(line))) + { + unsigned int push_update_option_flags = 0; + + if (!apply_pull_filter(options, line, is_update, &push_update_option_flags)) + { + msg(M_WARN, "Offending option received from server"); + return false; + } + /* + * No need to test also the application part here + * (add_option/remove_option/update_option) + */ + } + return true; +} + +int +process_incoming_push_msg(struct context *c, + const struct buffer *buffer, + bool honor_received_options, + unsigned int permission_mask, + unsigned int *option_types_found) +{ + struct buffer buf = *buffer; + + if (buf_string_compare_advance(&buf, "PUSH_REQUEST")) + { + return PUSH_MSG_REQUEST; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_reply_cmd)) + { + return PUSH_MSG_REPLY; + } + else if (honor_received_options + && buf_string_compare_advance(&buf, push_update_cmd)) + { + return process_incoming_push_update(c, permission_mask, + option_types_found, &buf); + } + else + { + return PUSH_MSG_ERROR; + } +} + +/* tests */ + +static void +test_incoming_push_message_basic(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,dhcp-option DNS 8.8.8.8, route 0.0.0.0 0.0.0.0 10.10.10.1"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_error1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATEerr,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + + +static void +test_incoming_push_message_error2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE ,dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_1(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -?dns, route something, ?dhcp-option DNS 8.8.8.8"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_bad_format(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, -dhcp-option, ?-dns"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_not_updatable_option(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE, dev tun"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_ERROR); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option, route 10.10.10.0, dhcp-option DNS 1.1.1.1, route 10.11.12.0, dhcp-option DOMAIN corp.local, keepalive 10 60"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static void +test_incoming_push_message_mix2(void **state) +{ + struct context *c = *state; + struct buffer buf = alloc_buf(256); + const char *update_msg = "PUSH_UPDATE,-dhcp-option,dhcp-option DNS 8.8.8.8,redirect-gateway local,route 192.168.1.0 255.255.255.0"; + buf_write(&buf, update_msg, strlen(update_msg)); + unsigned int option_types_found = 0; + + assert_int_equal(process_incoming_push_msg(c, &buf, c->options.pull, pull_permission_mask(c), &option_types_found), PUSH_MSG_UPDATE); + + free_buf(&buf); +} + +static int +setup(void **state) +{ + struct context *c = calloc(1, sizeof(struct context)); + c->options.pull = true; + c->options.route_nopull = false; + *state = c; + return 0; +} + +static int +teardown(void **state) +{ + struct context *c = *state; + free(c); + return 0; +} + +int +main(void) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_incoming_push_message_basic, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error1, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_error2, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_not_updatable_option, setup, teardown), + cmocka_unit_test_setup_teardown(test_incoming_push_message_1, setup, teardown), + 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) + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +} -- To view, visit http://gerrit.openvpn.net/c/openvpn/+/808?usp=email To unsubscribe, or for help writing mail filters, visit http://gerrit.openvpn.net/settings Gerrit-Project: openvpn Gerrit-Branch: master Gerrit-Change-Id: I6ecd4cb47571cc8c20e46de8595c742aeec6064a Gerrit-Change-Number: 808 Gerrit-PatchSet: 12 Gerrit-Owner: mrbff <ma...@ma...> Gerrit-Reviewer: flichtenheld <fr...@li...> Gerrit-Reviewer: plaisthos <arn...@rf...> Gerrit-Reviewer: stipa <lst...@gm...> Gerrit-CC: openvpn-devel <ope...@li...> Gerrit-Attention: flichtenheld <fr...@li...> Gerrit-Attention: stipa <lst...@gm...> Gerrit-Attention: mrbff <ma...@ma...> Gerrit-MessageType: newpatchset |