diff mbox series

[Openvpn-devel] dco: warn if DATA_V1 packets are sent to userspace

Message ID 20231008105316.21010-1-frank@lichtenheld.com
State Superseded
Headers
Series [Openvpn-devel] dco: warn if DATA_V1 packets are sent to userspace |

Commit Message

Frank Lichtenheld Oct. 8, 2023, 10:53 a.m. UTC 
  From: Lev Stipakov <lev@openvpn.net>

Servers 2.4.0 - 2.4.4 support peer-id and AEAD ciphers,
but only send DATA_V1 packets. With DCO enabled on the
client, connection is established but not working.

This is because DCO driver(s) are unable to handle
DATA_V1 packets and forwards them to userspace, where
they silently disappear since crypto context is in
DCO and not in userspace.

Starting from 2.4.5 server sends DATA_V2 so problem
doesn't happen.

We cannot switch to non-DCO on the fly, so we log this
and advice user to upgrade the server to 2.4.5 or newer.

This fixes https://github.com/OpenVPN/openvpn/issues/422

Change-Id: I8cb2cb083e3cdadf187b7874979d79af3974e759
Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to release/2.6.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/368
This mail reflects revision 1 of this Change.
Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>
Frank Lichtenheld <frank@lichtenheld.com>

Comments

Gert Doering Oct. 15, 2023, 2:34 p.m. UTC | #1 
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
diff mbox series

Patch

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))
             {
                 /* Restore pre-NCP frame parameters */
                 if (is_hard_reset_method2(opcode))