I just did a quick skim for now and I only have a codestyle comment below:
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");
> - }
> + }
> + if (!c->c2.pf.enabled)
> + {
> + register_signal(c, SIGUSR1, "plugin-pf-init-failed");
> }
> }
> #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)))
The '||' should go at the beginning of the second line.
> {
> + 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;
> }
>
>
--
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
