Hi,
On Sun, Oct 08, 2023 at 12:53:16PM +0200, Frank Lichtenheld wrote:
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index d8ad0d1..66843b4 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1058,8 +1058,16 @@
> * and return false.
> */
> uint8_t opcode = *BPTR(&c->c2.buf) >> P_OPCODE_SHIFT;
> - if (tls_pre_decrypt(c->c2.tls_multi, &c->c2.from, &c->c2.buf,
> &co,
> - floated, &ad_start))
> +
> + if ((opcode == P_DATA_V1) && dco_enabled(&c->options))
> + {
> + msg(D_LINK_ERRORS,
> + "Data Channel Offload doesn't support DATA_V1 packets. "
> + "Upgrade your server to 2.4.5 or newer.");
> + c->c2.buf.len = 0;
> + }
> + else if (tls_pre_decrypt(c->c2.tls_multi, &c->c2.from,
> &c->c2.buf,
> + &co, floated, &ad_start))
I understand the reasoning behind this patch, and the new code is
doing what it should
I do not like some more "visual" aspects of this - the comment above
this all is not changed, so does not match the code anymore, and the
newly introduced "else" makes the tls_pre_decrypt() branch start to
look like "this is 'just the else branch'", no longer "the main thing
being done here".
I'd leave off the "else", as tls_pre_decrypt() will do nothing and
return FALSE on "c->c2.buf_len <= 0".
Also, I'd reshuffle the function a bit - move the "opcode" assignment
before the existing comment block, and add a new comment for the
DATA_V1 case - maybe like this:
if (c->c2.tls_multi)
{
uint8_t opcode = *BPTR(&c->c2.buf) >> P_OPCODE_SHIFT;
/*
* If DCO is enabled, the kernel drivers require that the
* other end only sends P_DATA_V2 packets. V1 are unknown
* to kernel and passed to userland, but we cannot handle them
* either because crypto context is missing - so drop the packet.
*
* This can only happen with particular old (2.4.0-2.4.4) servers.
*/
if ((opcode == P_DATA_V1) && dco_enabled(&c->options))
{
msg(D_LINK_ERRORS,
"Data Channel Offload doesn't support DATA_V1 packets. "
"Upgrade your server to 2.4.5 or newer.");
c->c2.buf.len = 0;
}
/*
* If tls_pre_decrypt returns true, it means the incoming
* packet was a good TLS control channel packet. If so, TLS code
* will deal with the packet and set buf.len to 0 so downstream
* stages ignore it.
*
* If the packet is a data channel packet, tls_pre_decrypt
* will load crypto_options with the correct encryption key
* and return false.
*/
if (tls_pre_decrypt(c->c2.tls_multi, &c->c2.from, &c->c2.buf, &co,
floated, &ad_start))
what do you think?
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
