Message ID | 20171101220342.14648-4-steffan@karger.me |
---|---|
State | Accepted |
Headers | show |
Series | [Openvpn-devel,1/4,v3] pf: clean up temporary files if plugin init fails | expand |
On 02/11/17 06:03, Steffan Karger wrote: > From: Steffan Karger <steffan.karger@fox-it.com> > > 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. > > 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.karger@fox-it.com> Acked-by: Antonio Quartulli <a@unstable.cc>
Your patch has been applied to the master branch. commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916 Author: Steffan Karger Date: Wed Nov 1 23:03:41 2017 +0100 Don't throw fatal errors from create_temp_file() Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net> Message-Id: <20171101220342.14648-4-steffan@karger.me> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15701.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, Steffan asked me to pull this into 2.4, which makes sense - it's somewhere between "code cleanup" and "bugfix", basically avoiding an ASSERT() when we could more gracefully handle a failure on an incoming client (... in a somewhat unlikely chain of events, but still, ASSERT() on the server side for recoverable things is bad). commit 77a0bdb77d4c5573fcb78f1e36c45d882a9923ba (HEAD -> release/2.4) Author: Steffan Karger <steffan.karger@fox-it.com> Date: Wed Nov 1 23:03:41 2017 +0100 Don't throw fatal errors from create_temp_file() Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> Acked-by: Antonio Quartulli <antonio@openvpn.net> Message-Id: <20171101220342.14648-4-steffan@karger.me> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15701.html Signed-off-by: Gert Doering <gert@greenie.muc.de> (cherry picked from commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916) gert On Fri, Nov 24, 2017 at 01:42:40PM +0100, Gert Doering wrote: > Your patch has been applied to the master branch. > > commit 3e0fd2b0471cf4e53959902ca10d88db7a1ef916 > Author: Steffan Karger > Date: Wed Nov 1 23:03:41 2017 +0100 > > Don't throw fatal errors from create_temp_file() > > Signed-off-by: Steffan Karger <steffan.karger@fox-it.com> > Acked-by: Antonio Quartulli <antonio@openvpn.net> > Message-Id: <20171101220342.14648-4-steffan@karger.me> > URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg15701.html > Signed-off-by: Gert Doering <gert@greenie.muc.de> > > > -- > kind regards, > > Gert Doering > > > ------------------------------------------------------------------------------ > 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 >
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 8c7f6116..25f38003 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/ssl_verify.c b/src/openvpn/ssl_verify.c index 9cd36d7a..de54fb74 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))) { + 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; }