Hi,
On 15/09/17 05: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;
> }
>
> @@ -755,14 +755,14 @@ create_temp_file(const char *directory, const char
> *prefix, struct gc_arena *gc)
> else if (fd == -1 && errno != EEXIST)
> {
> /* Something else went wrong, no need to retry. */
> - msg(M_FATAL | M_ERRNO, "Could not create temporary file '%s'",
> + msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
> retfname);
> return NULL;
> }
> }
> while (attempts < 6);
>
> - msg(M_FATAL, "Failed to create temporary file after %i attempts",
> attempts);
> + msg(M_WARN, "Failed to create temporary file after %i attempts",
> attempts);
> return NULL;
> }
>
> 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");
> - }
Why removing this chunk above? Isn't that supposed to be printed when
the init function of the plugin fails?
> + }
> + if (!c->c2.pf.enabled)
> + {
> + register_signal(c, SIGUSR1, "plugin-pf-init-failed");
> }
Maybe it's just me missing how the signaling part really works, but what
happens on the server when the SIGUSR1 is delivered? And why registering
the signal when PF is disabled? I am missing something I think...
> }
> #endif /* ifdef PLUGIN_PF */
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 9cd36d7..df2736c 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert,
> const char *tmp_dir, stru
> FILE *peercert_file;
> const char *peercert_filename = "";
>
> - if (!tmp_dir)
> + /* create tmp file to store peer cert */
> + if (!tmp_dir ||
> + !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
|| at the beginning of the next line (as mentioned in my previous email)
> {
> + msg (M_WARN, "Failed to create peer cert file");
> return NULL;
> }
>
> - /* create tmp file to store peer cert */
> - peercert_filename = create_temp_file(tmp_dir, "pcf", gc);
> -
> /* write peer-cert in tmp-file */
> peercert_file = fopen(peercert_filename, "w+");
> if (!peercert_file)
> @@ -589,10 +589,13 @@ verify_cert_call_command(const char *verify_command,
> struct env_set *es,
>
> if (verify_export_cert)
> {
> - if ((tmp_file = verify_cert_export_cert(cert, verify_export_cert,
> &gc)))
> + tmp_file = verify_cert_export_cert(cert, verify_export_cert, &gc);
> + if (!tmp_file)
> {
> - setenv_str(es, "peer_cert", tmp_file);
> + ret = false;
> + goto cleanup;
> }
> + setenv_str(es, "peer_cert", tmp_file);
> }
>
> argv_parse_cmd(&argv, verify_command);
> @@ -609,6 +612,7 @@ verify_cert_call_command(const char *verify_command,
> struct env_set *es,
> }
> }
>
> +cleanup:
> gc_free(&gc);
> argv_reset(&argv);
>
> @@ -879,21 +883,21 @@ key_state_rm_auth_control_file(struct key_state *ks)
> }
> }
>
> -static void
> +static bool
> key_state_gen_auth_control_file(struct key_state *ks, const struct
> tls_options *opt)
> {
> struct gc_arena gc = gc_new();
> - const char *acf;
>
> key_state_rm_auth_control_file(ks);
> - acf = create_temp_file(opt->tmp_dir, "acf", &gc);
> + const char *acf = create_temp_file(opt->tmp_dir, "acf", &gc);
> if (acf)
> {
> ks->auth_control_file = string_alloc(acf, NULL);
> setenv_str(opt->es, "auth_control_file", ks->auth_control_file);
> - } /* FIXME: Should have better error handling? */
> + }
>
> gc_free(&gc);
> + return acf;
> }
>
> static unsigned int
> @@ -1184,7 +1188,12 @@ verify_user_pass_plugin(struct tls_session *session,
> const struct user_pass *up,
>
> #ifdef PLUGIN_DEF_AUTH
> /* generate filename for deferred auth control file */
> - key_state_gen_auth_control_file(ks, session->opt);
> + if (!key_state_gen_auth_control_file(ks, session->opt))
> + {
> + msg (D_TLS_ERRORS, "TLS Auth Error (%s): "
> + "could not create deferred auth control file", __func__);
> + goto cleanup;
> + }
> #endif
>
> /* call command */
> @@ -1209,6 +1218,7 @@ verify_user_pass_plugin(struct tls_session *session,
> const struct user_pass *up,
> msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer
> provided a blank username");
> }
>
> +cleanup:
> return retval;
> }
>
>
The rest looks good to me.
Cheers,
--
Antonio Quartulli
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
