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
