Re: [Openvpn-devel] [PATCH] Make OPENVPN_PLUGIN_ENABLE_PF failures FATAL
feed back: On 22/01/2021 07:02, Arne Schwabe wrote: Am 21.01.21 um 14:39 schrieb Gert Doering: Without this patch, if openpn is using a plugin that provides OPENVPN_PLUGIN_ENABLE_PF but then fails (returns OPENVPN_PLUGIN_FUNC_ERROR), OpenVPN will crash on a NULL pointer reference. The underlying cause is (likely) the refactoring work regarding CAS_SUCCEEDED etc., and that nobody adjusted the pf.c code accordingly (it tries to sent itself a SIGUSR1, which tries to tear down the client MI instance, but since it is not fully set up yet at this point, things explode). Full details on the call chain in Trac... Since we intend to remove pf in 2.6, but we still do not want OpenVPN to ever SIGSEGV, change the requirements for the plugins to "MUST SUCCEED", so if the plugin ENABLE_PF call fails, abort openvpn with a M_FATAL message. Trac: #1377 Signed-off-by: Gert Doering --- src/openvpn/pf.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c index f9bbfb50..3f472ef4 100644 --- a/src/openvpn/pf.c +++ b/src/openvpn/pf.c @@ -639,8 +639,17 @@ pf_init_context(struct context *c) } if (!c->c2.pf.enabled) { -msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client."); -register_signal(c, SIGUSR1, "plugin-pf-init-failed"); +/* At some point in openvpn history, this code just printed a + * warning and signalled itself (SIGUSR1, "plugin-pf-init-failed") + * to terminate the client instance. This got broken at one of + * the client auth state refactorings (leading to SIGSEGV crashes) + * and due to "pf will be removed anyway" reasons the easiest way + * to prevent crashes is to REQUIRE that plugins succeed - so if + * the plugin fails, we cleanly abort OpenVPN + * + * see also: https://community.openvpn.net/openvpn/ticket/1377 + */ +msg(M_FATAL, "FATAL: failed to init PF plugin, must succeed."); return; } } Acked-By: Arne Schwabe I agree to make this "fixed" in a way that doesn't involve refactoring of pf code that is later removed anyway. I don't think the refactoring early affects this. It has been probably broken even earlier. Arne I agree that a VPN should focus on its task and not try to be a firewall. I do use the PF plugin but it is of little, if any, actual use, which is not handled better elsewhere. I do not pretend to understand the intricacies of the code but if removing the packet filter plugin is relatively simple and clean then, from a user point-of-view, it makes more sense to drop it. Less complication overall. No Ack: Just feed back. -- tct ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Make OPENVPN_PLUGIN_ENABLE_PF failures FATAL
Am 21.01.21 um 14:39 schrieb Gert Doering: > Without this patch, if openpn is using a plugin that provides > OPENVPN_PLUGIN_ENABLE_PF but then fails (returns OPENVPN_PLUGIN_FUNC_ERROR), > OpenVPN will crash on a NULL pointer reference. > > The underlying cause is (likely) the refactoring work regarding > CAS_SUCCEEDED etc., and that nobody adjusted the pf.c code accordingly > (it tries to sent itself a SIGUSR1, which tries to tear down the > client MI instance, but since it is not fully set up yet at this > point, things explode). Full details on the call chain in Trac... > > Since we intend to remove pf in 2.6, but we still do not want OpenVPN > to ever SIGSEGV, change the requirements for the plugins to "MUST SUCCEED", > so if the plugin ENABLE_PF call fails, abort openvpn with a M_FATAL > message. > > Trac: #1377 > > Signed-off-by: Gert Doering > --- > src/openvpn/pf.c | 13 +++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c > index f9bbfb50..3f472ef4 100644 > --- a/src/openvpn/pf.c > +++ b/src/openvpn/pf.c > @@ -639,8 +639,17 @@ pf_init_context(struct context *c) > } > if (!c->c2.pf.enabled) > { > -msg(M_WARN, "WARNING: failed to init PF plugin, rejecting > client."); > -register_signal(c, SIGUSR1, "plugin-pf-init-failed"); > +/* At some point in openvpn history, this code just printed a > + * warning and signalled itself (SIGUSR1, > "plugin-pf-init-failed") > + * to terminate the client instance. This got broken at one of > + * the client auth state refactorings (leading to SIGSEGV > crashes) > + * and due to "pf will be removed anyway" reasons the easiest way > + * to prevent crashes is to REQUIRE that plugins succeed - so if > + * the plugin fails, we cleanly abort OpenVPN > + * > + * see also: https://community.openvpn.net/openvpn/ticket/1377 > + */ > +msg(M_FATAL, "FATAL: failed to init PF plugin, must succeed."); > return; > } > } > Acked-By: Arne Schwabe I agree to make this "fixed" in a way that doesn't involve refactoring of pf code that is later removed anyway. I don't think the refactoring early affects this. It has been probably broken even earlier. Arne ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 3/5] Allow running a default configuration with TLS libraries without BF-CBC
Hi, On 07/09/2020 18:22, Arne Schwabe wrote: > Modern TLS libraries might drop Blowfish by default or distributions > might disable Blowfish in OpenSSL/mbed TLS. We still signal OCC > options with BF-CBC compatible strings. To avoid requiring BF-CBC > for this, special case this one usage of BF-CBC enough to avoid a hard > requirement on Blowfish in the default configuration. > > Signed-off-by: Arne Schwabe > --- > src/openvpn/init.c| 18 +-- > src/openvpn/options.c | 51 --- > 2 files changed, 54 insertions(+), 15 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index dff090b1..1e0baf2a 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -2752,14 +2752,18 @@ do_init_crypto_tls_c1(struct context *c) > #endif /* if P2MP */ > } > > -/* Do not warn if we only have BF-CBC in options->ciphername > - * because it is still the default cipher */ > -bool warn = !streq(options->ciphername, "BF-CBC") > - || options->enable_ncp_fallback; > -/* Get cipher & hash algorithms */ > -init_key_type(>c1.ks.key_type, options->ciphername, > options->authname, > - options->keysize, true, warn); > > +if (!options->ncp_enabled || options->enable_ncp_fallback > +|| !streq(options->ciphername, "BF-CBC")) > +{ > +/* Get cipher & hash algorithms > + * skip BF-CBC for NCP setups when cipher as this is the default > + * and is also special cased later to allow it to be not > available > + * as we need to construct a fake BF-CBC occ string > + */ After our discussion I believe I understood what this part is about. However, I think the comment could be made a bit more explicit. I would like to propose the following: /* * BF-CBC is allowed to be used only when explicitly configured * as NCP-fallback or when NCP has been disabled. * In all other cases don't attempt to initialize BF-CBC as it * may not even be supported by the underlying SSL library. * * Therefore, the key structure has to be initialized when: * - any non-BF-CBC cipher was selected; or * - BF-CBC is selected and NCP is disabled (explicit request to * use the BF-CBC cipher); or * - BF-CBC is selected, NCP is enabled and fallback is enabled * (BF-CBC will be the fallback). * * Note that BF-CBC will still be part of the OCC string to retain * backwards compatibility with older clients. */ This comment should be placed above the if-block. At the same time I would like to propose a switch within the if-block conditions as follows: if (!streq(options->ciphername, "BF-CBC") || !options->ncp_enabled || options->enable_ncp_fallback) > +init_key_type(>c1.ks.key_type, options->ciphername, > options->authname, > + options->keysize, true, true); > +} Why do you always want to warn the user in this context? By passing warn=true all the time (last argument) we will have openvpn always warning the user about "weak cipher selected", but ciphername could be anything at this point. > /* Initialize PRNG with config-specified digest */ > prng_init(options->prng_hash, options->prng_nonce_secret_len); > > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 90e78a7b..01da88ad 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -3640,11 +3640,32 @@ calc_options_string_link_mtu(const struct options *o, > const struct frame *frame) > { > struct frame fake_frame = *frame; > struct key_type fake_kt; > -init_key_type(_kt, o->ciphername, o->authname, o->keysize, true, > - false); > + > frame_remove_from_extra_frame(_frame, crypto_max_overhead()); > -crypto_adjust_frame_parameters(_frame, _kt, o->replay, > - > cipher_kt_mode_ofb_cfb(fake_kt.cipher)); > + > +/* o->ciphername can be still BF-CBC and our SSL library might not > like > + * like it, workaround this important corner case in the name of > + * compatibility and not stopping openvpn on our default > configuration > + */ I would rephrase a bit this comment to make it more explicit for the casual reader. See below. > +if ((strcmp(o->ciphername, "BF-CBC") == 0) > +&& cipher_kt_get(o->ciphername) == NULL) > +{ > +init_key_type(_kt, "none", o->authname, o->keysize, true, > + false); > + > +crypto_adjust_frame_parameters(_frame, _kt, o->replay, > + > cipher_kt_mode_ofb_cfb(fake_kt.cipher)); > +/* 64 bit block size, 64 bit IV size */ > +frame_add_to_extra_frame(_frame, 64/8 + 64/8); > +} > +else > +{ > +init_key_type(_kt,
Re: [Openvpn-devel] [PATCH 09/11] Implement deferred auth for scripts
On 30/09/2020 15:13, Arne Schwabe wrote: Signed-off-by: Arne Schwabe --- Changes.rst | 9 + doc/man-sections/script-options.rst | 14 +++- src/openvpn/ssl_verify.c| 56 - 3 files changed, 70 insertions(+), 9 deletions(-) Only glared at the code here too. In addition to prior merge conflicts, commit bbcada8abb410 seems to add even more confusion when applying this patch. diff --git a/Changes.rst b/Changes.rst index f67e1d76..7e60eb64 100644 --- a/Changes.rst +++ b/Changes.rst Ignoring merge conflicts here though. Not important in this round. diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index fc3a1116..5e15fa32 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1189,14 +1189,14 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con /* * Verify the user name and password using a script */ -static bool +static int verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, const struct user_pass *up) { struct gc_arena gc = gc_new(); struct argv argv = argv_new(); const char *tmp_file = ""; -bool ret = OPENVPN_PLUGIN_FUNC_ERROR; +int retval = OPENVPN_PLUGIN_FUNC_ERROR; Good fixing this mistake from previous round, but incorrect indenting. /* Is username defined? */ if ((session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL) || strlen(up->username)) @@ -1247,10 +1247,45 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, argv_parse_cmd(, session->opt->auth_user_pass_verify_script); argv_printf_cat(, "%s", tmp_file); +/* Add files needed for deferred auth */ +key_state_gen_auth_control_files(>key[KS_PRIMARY], + session->opt); + /* call command */ -ret = openvpn_run_script(, session->opt->es, 0, - "--auth-user-pass-verify"); +int script_ret = openvpn_run_script(, session->opt->es, S_EXITCODE, +"--auth-user-pass-verify"); Perhaps move the retval declaration down here, as we're touching it anyhow? This switch() is the first place we use it, unless I'm overlooking something. +switch (script_ret) +{ + case 0: + retval = OPENVPN_PLUGIN_FUNC_SUCCESS; + break; + case 2: + retval = OPENVPN_PLUGIN_FUNC_DEFERRED; + break; + default: + retval = OPENVPN_PLUGIN_FUNC_ERROR; + break; + } [...snip...] /* @@ -1406,7 +1441,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, struct tls_session *session) { int s1 = OPENVPN_PLUGIN_FUNC_SUCCESS; -bool s2 = true; +int s2 = OPENVPN_PLUGIN_FUNC_SUCCESS; Indenting issues? [...snip...] @@ -1499,7 +1534,11 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, #ifdef PLUGIN_DEF_AUTH || s1 == OPENVPN_PLUGIN_FUNC_DEFERRED #endif - ) && s2 + ) && +((s2 == OPENVPN_PLUGIN_FUNC_SUCCESS) +#ifdef ENABLE_DEF_AUTH + || s2 == OPENVPN_PLUGIN_FUNC_DEFERRED) +#endif #ifdef MANAGEMENT_DEF_AUTH && man_def_auth != KMDA_ERROR #endif Yikes! This if() statement is unreadable. Since you are doing changes here, could we improve this whole logic. Perhaps using a helper macro like this (incorrect code-style, for e-mail readability) #define PLUGIN_AUTH_RESULT_PASS(s) \ (OPENVPN_PLUGIN_FUNC_SUCCESS == s\ || OPENVPN_PLUGIN_FUNC_DEFERRED == s) [...] if (PLUGIN_AUTH_RESULT_PASS(s1) && PLUGIN_AUTH_RESULT_PASS(s2) && tls_lock_username(multi, up->username)) { #ifdef ENABLE_MANGEMENT if (KMDA_ERROR == man_def_auth) { /* ... error handling ... */ goto error; } if (KMDA_UNDEF == man_def_auth) { ks->authenticated = KS_AUTH_DEFERRED; } #endif // ENABLE_MANAGEMENT /* ... rest of the if block content ... */ return; // Success } error: /* ... existing error handling from if-else block... */ This is just a quick draft skeleton. Right now the code is pretty messy, and we should improve the code quality on such critical code paths such as user authentication. -- kind regards, David Sommerseth OpenVPN Inc ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 08/11] Allow pending auth to be send from a auth plugin
On 30/09/2020 15:13, Arne Schwabe wrote: Signed-off-by: Arne Schwabe --- doc/man-sections/generic-options.rst | 3 +- include/openvpn-plugin.h.in | 8 ++ src/openvpn/ssl.c| 2 +- src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_verify.c | 165 --- src/openvpn/ssl_verify.h | 2 +- 6 files changed, 165 insertions(+), 16 deletions(-) So far just glared at the code, but the change below needs to be fixed first. This patchset has also aged so much it does no longer apply on top of latest git master. [...snip...] diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index e7e62afa..fc3a1116 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c[...snip...] @@ -1067,7 +1196,7 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, struct gc_arena gc = gc_new(); struct argv argv = argv_new(); const char *tmp_file = ""; -bool ret = false; +bool ret = OPENVPN_PLUGIN_FUNC_ERROR; This is wrong. OPENVPN_PLUGIN_FUNC_ERROR is 1, which means "true". I see this is being corrected again in the next patch; but lets make it correct from the beginning to avoid making a potential bisect in the future more confusing than needed. The rest of the code looks reasonable. I've not tested it yet, as there are some merge conflicts now. Since the surrounding code has changed a bit since this patch series , I consider it a bit risky to conclude on testing this on a older code base without many of the fixes in between in place. Most of the merge conflicts is probably related to commit 99d217b20064 (removing --disable-def-auth), but there are other AUTH related changes as well. This needs to be carefully tested with all these auth changes in place too. -- kind regards, David Sommerseth OpenVPN Inc ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] clean up / rewrite sample-plugins/defer/simple.c
Am 21.01.21 um 18:25 schrieb Gert Doering: > If we ship something that we consider a form of documentation > "this is how to write an OpenVPN plugin" it should meet our standards > for secure and modern code. This plugin did neither. > > - get rid of system() calls, especially those that enabled a > remote-root exploit if this code was used "as is" > > - change logging from printf() to OpenVPN's plugin_log() > > - this requires changing to openvpn_plugin_open_v3() to get > to the function pointers > > - change wacky "background and sleep in the shell call" to the > double-fork/waitpid model we use in plugins/auth-pam > (copy-paste code reuse) > > - OpenVPN 2.5 and later react badly to OPENVPN_PLUGIN_FUNC_ERROR > returns to OPENVPN_PLUGIN_ENABLE_PF calls (SIGSEGV crash), so > always return SUCCESS. Only hook ENABLE_PF if that functionality > is actually requested ("setenv test_packet_filter NN"). > > - change deeply-nested functions auth_user_pass_verify() and > tls_final() to use early-return style > > - actually make defered PF setup *work* with recent OpenVPNs > (pre-creating temp files broke this, so unlink() the pre-created > file in the ENABLE_PF hook, and re-create asyncronously later) > > - add lots of comments explaining why we do things this way > > +while( r>0 ); style But overall it improves the sample plugin so massive that I think we should merge it because it fixes several big issues. Acked-By: Arne Schwabe ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 10/11] Implement --client-crresponse script options and plugin interface
On 30/09/2020 15:13, Arne Schwabe wrote: This is allows scripts and pluginsto parse/react to a CR_RESPONSE message Signed-off-by: Arne Schwabe --- Changes.rst | 7 doc/man-sections/script-options.rst | 28 - include/openvpn-plugin.h.in | 7 +++- src/openvpn/init.c | 1 + src/openvpn/options.c | 15 +++ src/openvpn/options.h | 1 + src/openvpn/push.c | 4 ++ src/openvpn/ssl_common.h| 1 + src/openvpn/ssl_verify.c| 63 + src/openvpn/ssl_verify.h| 23 +++ 10 files changed, 147 insertions(+), 3 deletions(-) Only glared at the code here too. [...snip...] diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst index e0cc10c2..66bf3662 100644 --- a/doc/man-sections/script-options.rst +++ b/doc/man-sections/script-options.rst [...snip...] @@ -123,6 +128,25 @@ SCRIPT HOOKS For a sample script that performs PAM authentication, see :code:`sample-scripts/auth-pam.pl` in the OpenVPN source distribution. +--client-crresponse +Executed when the client sends a text based challenge response. + +Valid syntax: +:: + +client-crresponse cmd method + [...snip...] diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 3df803db..703927da 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c [...snip...] @@ -7070,6 +7075,16 @@ add_option(struct options *options, set_user_script(options, >client_connect_script, p[1], "client-connect", true); } +else if (streq(p[0], "client-crresponse") && p[1]) +{ +VERIFY_PERMISSION(OPT_P_SCRIPT); +if (!no_more_than_n_args(msglevel, p, 2, NM_QUOTE_HINT)) +{ +goto err; +} +set_user_script(options, >client_crresponse_script, +p[1], "client-crresponse", true); +} Either the doc is wrong, or the option parser is lacking parsing of "method". [...snip...] diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 877e9396..a63a1967 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -440,6 +440,7 @@ struct options const char *client_connect_script; const char *client_disconnect_script; const char *learn_address_script; +const char *client_crresponse_script; Indentation. [...snip...] diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 58e20baa..e5c92e17 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -227,6 +227,10 @@ receive_cr_response(struct context *c, const struct buffer *buffer) management_notify_client_cr_response(key_id, mda, es, m); +#endif +#if ENABLE_PLUGIN +verify_crresponse_plugin(c->c2.tls_multi, m); +verify_crresponse_script(c->c2.tls_multi, m); Any reason the script feature is insdie the ENABLE_PLUGIN fence? [...snip...] diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 98afc88c..87877c88 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -314,6 +314,7 @@ struct tls_options /* used for username/password authentication */ const char *auth_user_pass_verify_script; +const char *client_crresponse_script; Indentation. I've not looked that carefully at the rest of the code, as I would like to test those code paths when completing the review. It looks reasonable though at a first glance, but might be I stumble across something during testing. -- kind regards, David Sommerseth OpenVPN Inc ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] clean up / rewrite sample-plugins/defer/simple.c
If we ship something that we consider a form of documentation "this is how to write an OpenVPN plugin" it should meet our standards for secure and modern code. This plugin did neither. - get rid of system() calls, especially those that enabled a remote-root exploit if this code was used "as is" - change logging from printf() to OpenVPN's plugin_log() - this requires changing to openvpn_plugin_open_v3() to get to the function pointers - change wacky "background and sleep in the shell call" to the double-fork/waitpid model we use in plugins/auth-pam (copy-paste code reuse) - OpenVPN 2.5 and later react badly to OPENVPN_PLUGIN_FUNC_ERROR returns to OPENVPN_PLUGIN_ENABLE_PF calls (SIGSEGV crash), so always return SUCCESS. Only hook ENABLE_PF if that functionality is actually requested ("setenv test_packet_filter NN"). - change deeply-nested functions auth_user_pass_verify() and tls_final() to use early-return style - actually make defered PF setup *work* with recent OpenVPNs (pre-creating temp files broke this, so unlink() the pre-created file in the ENABLE_PF hook, and re-create asyncronously later) - add lots of comments explaining why we do things this way Security issue reported by "oxr463" on HackerOne. Signed-off-by: Gert Doering --- sample/sample-plugins/defer/simple.c | 357 --- 1 file changed, 265 insertions(+), 92 deletions(-) diff --git a/sample/sample-plugins/defer/simple.c b/sample/sample-plugins/defer/simple.c index 64338b4a..7be1c0a0 100644 --- a/sample/sample-plugins/defer/simple.c +++ b/sample/sample-plugins/defer/simple.c @@ -54,13 +54,16 @@ #include #include #include +#include +#include +#include +#include +#include #include "openvpn-plugin.h" -/* bool definitions */ -#define bool int -#define true 1 -#define false 0 +/* Pointers to functions exported from openvpn */ +static plugin_log_t plugin_log = NULL; /* * Our context, where we keep our state. @@ -76,6 +79,9 @@ struct plugin_per_client_context { bool generated_pf_file; }; +/* module name for plugin_log() */ +static char *MODULE = "defer/simple"; + /* * Given an environmental variable name, search * the envp array for its value, returning it @@ -130,33 +136,46 @@ atoi_null0(const char *str) } } -OPENVPN_EXPORT openvpn_plugin_handle_t -openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char *envp[]) +/* use v3 functions so we can use openvpn's logging and base64 etc. */ +OPENVPN_EXPORT int +openvpn_plugin_open_v3(const int v3structver, + struct openvpn_plugin_args_open_in const *args, + struct openvpn_plugin_args_open_return *ret) { +const char **envp = args->envp; /* environment variables */ struct plugin_context *context; -printf("FUNC: openvpn_plugin_open_v1\n"); +/* Check API compatibility -- struct version 5 or higher needed */ +if (v3structver < 5) +{ +fprintf(stderr, "sample-client-connect: this plugin is incompatible with the running version of OpenVPN\n"); +return OPENVPN_PLUGIN_FUNC_ERROR; +} + +/* Save global pointers to functions exported from openvpn */ +plugin_log = args->callbacks->plugin_log; + +plugin_log(PLOG_NOTE, MODULE, "FUNC: openvpn_plugin_open_v3"); /* * Allocate our context */ context = (struct plugin_context *) calloc(1, sizeof(struct plugin_context)); -if (context == NULL) +if (!context) { -printf("PLUGIN: allocating memory for context failed\n"); -return NULL; +goto error; } context->test_deferred_auth = atoi_null0(get_env("test_deferred_auth", envp)); -printf("TEST_DEFERRED_AUTH %d\n", context->test_deferred_auth); +plugin_log(PLOG_NOTE, MODULE, "TEST_DEFERRED_AUTH %d", context->test_deferred_auth); context->test_packet_filter = atoi_null0(get_env("test_packet_filter", envp)); -printf("TEST_PACKET_FILTER %d\n", context->test_packet_filter); +plugin_log(PLOG_NOTE, MODULE, "TEST_PACKET_FILTER %d", context->test_packet_filter); /* * Which callbacks to intercept. */ -*type_mask = +ret->type_mask = OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_UP) |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_DOWN) |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_ROUTE_UP) @@ -166,90 +185,241 @@ openvpn_plugin_open_v1(unsigned int *type_mask, const char *argv[], const char * |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_CONNECT_V2) |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_CLIENT_DISCONNECT) |OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_LEARN_ADDRESS) -|OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_TLS_FINAL) -|OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_ENABLE_PF); +|OPENVPN_PLUGIN_MASK(OPENVPN_PLUGIN_TLS_FINAL); -return (openvpn_plugin_handle_t) context; +/* ENABLE_PF should only be called if we're actually willing to
Re: [Openvpn-devel] [PATCH 07/11] Refactor extract_var_peer_info into standalone function and add ssl_util.c
Hi, Both new files have > +} > \ No newline at end of file Can probably be fixed by the committer. Stared at the come, compiled with MSVC. No "brand new" code added, just existing one factored out into a separate function and generalized. Acked-by: Lev Stipakov Acked with distinction for updating MSVC project files. ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Make OPENVPN_PLUGIN_ENABLE_PF failures FATAL
Without this patch, if openpn is using a plugin that provides OPENVPN_PLUGIN_ENABLE_PF but then fails (returns OPENVPN_PLUGIN_FUNC_ERROR), OpenVPN will crash on a NULL pointer reference. The underlying cause is (likely) the refactoring work regarding CAS_SUCCEEDED etc., and that nobody adjusted the pf.c code accordingly (it tries to sent itself a SIGUSR1, which tries to tear down the client MI instance, but since it is not fully set up yet at this point, things explode). Full details on the call chain in Trac... Since we intend to remove pf in 2.6, but we still do not want OpenVPN to ever SIGSEGV, change the requirements for the plugins to "MUST SUCCEED", so if the plugin ENABLE_PF call fails, abort openvpn with a M_FATAL message. Trac: #1377 Signed-off-by: Gert Doering --- src/openvpn/pf.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c index f9bbfb50..3f472ef4 100644 --- a/src/openvpn/pf.c +++ b/src/openvpn/pf.c @@ -639,8 +639,17 @@ pf_init_context(struct context *c) } if (!c->c2.pf.enabled) { -msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client."); -register_signal(c, SIGUSR1, "plugin-pf-init-failed"); +/* At some point in openvpn history, this code just printed a + * warning and signalled itself (SIGUSR1, "plugin-pf-init-failed") + * to terminate the client instance. This got broken at one of + * the client auth state refactorings (leading to SIGSEGV crashes) + * and due to "pf will be removed anyway" reasons the easiest way + * to prevent crashes is to REQUIRE that plugins succeed - so if + * the plugin fails, we cleanly abort OpenVPN + * + * see also: https://community.openvpn.net/openvpn/ticket/1377 + */ +msg(M_FATAL, "FATAL: failed to init PF plugin, must succeed."); return; } } -- 2.26.2 ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 06/11] Add S_EXITCODE flag for openvpn_run_script to report exit code
Hi, > +platform_ret_code(int stat) > +{ > + > +if (stat >= 0 && stat < 255) Unneeded line break. > -/* interpret the status code returned by execve() */ > +/** interpret the status code returned by execve() */ > bool platform_system_ok(int stat); > > +/** Return a return code if valid and between 0 and 255, -1 otherwise */ > +int platform_ret_code(int stat); "Return a return code" looks odd. Maybe "status code" or "exit code" ? > +if(flags & S_EXITCODE) Missing whitespace after if. > +/** > + * Will run a script and return the exit code if it > + */ "If it" what? Assuming committer will fix those nit-picks (especially last one), Acked-by: Lev Stipakov ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 05/11] Change parameter of send_auth_pending_messages from context to tls_multi
Hi, > +/** > + * Reschedule tls_multi_process. > + * NOTE: in multi-client mode, usually the below two statements are I realize that this comment is copied from existing code, but "below two statements" is bit misleading here in the context of function definition. Maybe just "this function" ? Stared at the code, compiled with MSVC. Acked-by: Lev Stipakov -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 04/11] Introduce management client state for AUTH_PENDING notifications
Stared at the code, compiled with MSVC. Acked-by: Lev Stipakov ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 03/11] Implement server side of AUTH_PENDING with extending timeout
Hi, Note that I didn't manage to apply this patch on the latest master so I had to apply commit from https://github.com/schwabe/openvpn/commit/42ae41d812668c4c00badaf592825684fa387d9d > +static bool > +parse_kid(const char *str, unsigned int *kid) > +&& parse_uint(timeout_str, "TIMEOUT", )) Since you've added wrapper "parse_kid", why not add "parse_timeout" for consistency? > +bool ret = send_auth_pending_messages(>context, extra, timeout); >C:\Users\lev\Projects\openvpn\src\openvpn\multi.c(3907,68): error C2220: the >following warning is treated as an error >C:\Users\lev\Projects\openvpn\src\openvpn\multi.c(3907,68): warning C4020: >'send_auth_pending_messages': too many actual parameters > -send_auth_pending_messages(struct context *c, const char *extra) > +send_auth_pending_messages(struct context *c, const char *extra, > + unsigned int timeout) > { > -send_control_channel_string(c, "AUTH_PENDING", D_PUSH); > +struct key_state *ks = _multi->session[TM_ACTIVE].key[KS_PRIMARY]; >C:\Users\lev\Projects\openvpn\src\openvpn\push.c(354,1): error C2220: the >following warning is treated as an error >C:\Users\lev\Projects\openvpn\src\openvpn\push.c(354,1): warning C4029: >declared formal parameter list different from definition >C:\Users\lev\Projects\openvpn\src\openvpn\push.c(355,38): error C2065: >'tls_multi': undeclared identifier >C:\Users\lev\Projects\openvpn\src\openvpn\push.c(355,40): error C2223: left of >'->session' must point to struct/union > +size_t len = 20 + 1 + sizeof(auth_pre); > size_t len = strlen(extra)+1 + sizeof(info_pre); Whitespace inconsistency. > +unsigned int > +extract_iv_proto(const char *peer_info) > +{ > + > +const char *optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; Unnecessary line break. -- -Lev ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] HEADS UP: fate of the built-in packet filter (PF)
Hi, OpenVPN has a built-in packet filter, which has a couple of issues - it is IPv4 only (though IPv6 patches existed at some point, but nobody reviewed them, so they did not get merged) - it can only be configured by a plugin or the management interface (so actually *using* it is not very straightforward) - it is not tested in any automated way today - none of the core developers uses it, or knows any deployment where it is used - so if we break it, we might not even notice (this was actually what brought up the discussion today - if a plugin returns OPENVPN_PLUGIN_FUNC_ERROR on OPENVPN_PLUGIN_ENABLE_PF, openvpn will crash with a NULL pointer access...) - not even OpenVPN AS, which usually uses "those interesting features that nobody else knows about" uses PF (compiles with --disable-pf) Based on this, we consider ripping all the PF stuff *out* of OpenVPN for the 2.6 release ("hopefully later this year"). This is your chance to speak up and tell us "I use OpenVPN pf for this totally cool thing, and there is no way to do this with the firewalling layer the operating system provides, because..." :-) So - surprise us! gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel