On 14/09/17 23:34, Steffan Karger wrote:
> This function is called in response to connecting clients, and can fail
> when I/O fails for some (possibly temporary) reason. In such cases we
> should not exit the process, but just reject the connecting client.
>
> This commit changes the function to actually return NULL on errors, and
> (where needed) changes the callers to check for and handle errors.
>
> Note that this changes the behavior for pf plugins: instead of just not
> initializing the firewall rules and happily continuing, this now rejects
> the client in the case of an (unlikely) failure to initialize the pf.
>
> Since the tls-crypt-v2 metadata code also calls create_temp_file() when
> clients connect, I consider this a prerequisite for tls-crypt-v2.
>
> Signed-off-by: Steffan Karger <steffan.kar...@fox-it.com>
> ---
> src/openvpn/misc.c | 6 +++---
> src/openvpn/pf.c | 8 ++++----
> src/openvpn/ssl_verify.c | 32 +++++++++++++++++++++-----------
> 3 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 8c7f611..25f3800 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -740,7 +740,7 @@ create_temp_file(const char *directory, const char
> *prefix, struct gc_arena *gc)
> retfname = gen_path(directory, BSTR(&fname), gc);
> if (!retfname)
> {
> - msg(M_FATAL, "Failed to create temporary filename and path");
> + msg(M_WARN, "Failed to create temporary filename and path");
> return NULL;
All these changes to create_temp_file() are good. The use of M_FATAL
must have been an oversight when this part was overhauled approx 8 years
ago. M_WARN is the proper log tag. And all places calling
create_temp_file() have proper checks if this function returns NULL. So
this part of the patch gets an ACK.
[...snip...]
> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
> index 5cb002b..5fe1734 100644
> --- a/src/openvpn/pf.c
> +++ b/src/openvpn/pf.c
> @@ -639,10 +639,10 @@ pf_init_context(struct context *c)
> }
> #endif
> }
> - else
> - {
> - msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
> - }
> + }
> + if (!c->c2.pf.enabled)
> + {
> + register_signal(c, SIGUSR1, "plugin-pf-init-failed");
> }
This change scares me. Unless I overlooked something, pf_init_context()
is called on each new connection happens on the server side, through
multi_create_instance(). Is it truly safe to call SIGUSR1 on the server
process, considering other clients may be connected at the same time?
On a client process, this would be fine though.
Just noticed Antonio are thinking along the same paths, so either we're
missing some details ... or this is risky.
--
kind regards,
David Sommerseth
OpenVPN Technologies, Inc
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
