Hi, Thanks for the clear report, patch and follow-up.
On 20-05-2020 23:31, Jeremy Evans wrote: > On 05/20 09:33, Gert Doering wrote: >> On Wed, May 20, 2020 at 11:34:04AM -0700, Jeremy Evans wrote: >>> To give some background, we hit this assertion failure, with the >>> following log output: >> >> This should not happen, asserting out in "normal server use" is bad. >> >> (Neither should it ever reach that point without ks->authenticated being >> true) Agreed. I think it makes sense to first fix the urgent part (ie, not kill the server), then figure out how de code ends up at this ASSERT. Jeremy, can you determine from your logs whether this always happenedwhen --auth-user-pass-verify returns zero or non-zero? Ie, should the connections that trigger this succeed or fail? >>> @@ -1930,7 +1930,10 @@ tls_session_generate_data_channel_keys(struct >>> tls_session *session) >>> &ks->session_id_remote : >>> &session->session_id; >>> >>> - ASSERT(ks->authenticated); >>> + if (!ks->authenticated) { >>> + msg(D_TLS_ERRORS, "TLS Error: key_state not authenticated"); >>> + goto cleanup; >>> + } >>> >>> ks->crypto_options.flags = session->opt->crypto_flags; >>> if (!generate_key_expansion(&ks->crypto_options.key_ctx_bi, >> >> I'm not sure if that code is correct, though - it will erase key >> material (in cleanup) without actually having generated a session >> key. So "bad things might happen later". > > The same behavior would happen if generate_key_expansion fails a few > lines below, so my assumption was it was safe to do so. However, that's > just an assumption and not even an educated guess. This change correctly turns "kill the server" into "reject the connection", which I agree is a good thing. Acked-by: Steffan Karger <steffan.kar...@foxcrypto.com> -Steffan _______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel