Use P_DATA_V2 for server->client packets too

PATCH Superseded Steffan Karger 4 participants 7 messages Nov 11, 2017 – Nov 24, 2017

Share & tools

Steffan Karger

author

2017-11-11 16:03 UTC #0 lore raw

PATCH 1/1 Use P_DATA_V2 for server->client packets too

P_DATA_V2 introduced the peer-id. This allows clients to float, but as a side-effect 32-bit aligns the encrypted data. That alignment improves performance particularly on cheaper/older CPUs. So although servers don't actually have a peer-id, still use the V2 packet format (with a zero-id) for server->client traffic too.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

src/openvpn/forward.c +1 -1

index 1b7455bb..20e89d34 100644
@@ -496,7 +496,7 @@ encrypt_sign(struct context *c, bool comp_frag)
         /* If using P_DATA_V2, prepend the 1-byte opcode and 3-byte peer-id to the
          * packet before openvpn_encrypt(), so we can authenticate the opcode too.
          */
-        if (c->c2.buf.len > 0 && !c->c2.tls_multi->opt.server && c->c2.tls_multi->use_peer_id)
+        if (c->c2.buf.len > 0 && c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v2(c->c2.tls_multi, &b->encrypt_buf);
         }

Signature · 1 line

2.14.1

Antonio Quartulli

NAK

2017-11-12 15:33 UTC #1 lore raw

Hi,

Quoted 16 lines · Steffan Karger

P_DATA_V2 introduced the peer-id. This allows clients to float, but as a side-effect 32-bit aligns the encrypted data. That alignment improves performance particularly on cheaper/older CPUs. So although servers don't actually have a peer-id, still use the V2 packet format (with a zero-id) for server->client traffic too.
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
src/openvpn/forward.c +0 -0
index 1b7455bb..20e89d34 100644

src/openvpn/forward.c encrypt_sign(struct context *c, bool comp_frag) · line 502

Quoted 8 lines

src/openvpn/forward.c +1 -1

@@ -496,7 +496,7 @@ encrypt_sign(struct context *c, bool comp_frag)
         /* If using P_DATA_V2, prepend the 1-byte opcode and 3-byte peer-id to the
          * packet before openvpn_encrypt(), so we can authenticate the opcode too.
          */
-        if (c->c2.buf.len > 0 && !c->c2.tls_multi->opt.server && c->c2.tls_multi->use_peer_id)
+        if (c->c2.buf.len > 0 && c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v2(c->c2.tls_multi, &b->encrypt_buf);
         }

NAK.
Although the patch looks good and easy, it is actually missing an important part: c->c2.tls_multi->use_peer_id is never set to true on the server.

This should probably happen after parsing "IV_PROTO=" in prepare_push_reply().

Cheers,

Signature · 1 line

Antonio Quartulli

Steffan Karger

author

2017-11-12 17:22 UTC #2 lore raw

PATCH v2 1/1 Use P_DATA_V2 for server->client packets too

Signed-off-by Steffan Karger <[email protected]...>

---
v2: actually enable P_DATA_V2... Now tested with:

    2.4<>2.4 (V2), 2.4-srv<>2.3-clt (V2), 2.3-srv<>2.4-clt (V1),
    2.4-srv<>2.2-clt (V1)

 src/openvpn/forward.c | 4 ++--
 src/openvpn/push.c    | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

src/openvpn/forward.c +2 -2

index 1b7455bb..a868a8ff 100644
@@ -496,7 +496,7 @@ encrypt_sign(struct context *c, bool comp_frag)
         /* If using P_DATA_V2, prepend the 1-byte opcode and 3-byte peer-id to the
          * packet before openvpn_encrypt(), so we can authenticate the opcode too.
          */
-        if (c->c2.buf.len > 0 && !c->c2.tls_multi->opt.server && c->c2.tls_multi->use_peer_id)
+        if (c->c2.buf.len > 0 && c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v2(c->c2.tls_multi, &b->encrypt_buf);
         }

@@ -512,7 +512,7 @@ encrypt_sign(struct context *c, bool comp_frag)
     /* Do packet administration */
     if (c->c2.tls_multi)
     {
-        if (c->c2.buf.len > 0 && (c->c2.tls_multi->opt.server || !c->c2.tls_multi->use_peer_id))
+        if (c->c2.buf.len > 0 && !c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v1(c->c2.tls_multi, &c->c2.buf);
         }

src/openvpn/push.c +1 -0

index 5947a31f..16a4101f 100644
@@ -366,6 +366,7 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
             push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
                             tls_multi->peer_id);
         }
+        tls_multi->use_peer_id = true;
     }
 
     /* Push cipher if client supports Negotiable Crypto Parameters */

Signature · 1 line

2.14.1

Antonio Quartulli

Acked-by on v2 1/1

2017-11-12 17:32 UTC #3 lore raw

Hi,

Quoted 7 lines · Steffan Karger

P_DATA_V2 introduced the peer-id. This allows clients to float, but as a side-effect 32-bit aligns the encrypted data. That alignment improves performance particularly on cheaper/older CPUs. So although servers don't actually have a peer-id, still use the V2 packet format (with a zero-id) for server->client traffic too.
Signed-off-by Steffan Karger <[email protected]...>

Code looks good and this time I trust the tests performed by Steffan :)

Acked-by Antonio Quartulli <[email protected]...>

Signature · 1 line

Antonio Quartulli

Gert Doering

2017-11-24 13:06 UTC #4 lore raw

In reply to Steffan Karger

Hi,

as just discussed on IRC, I'm not fully happy with this, and want a v3...

On Sun, Nov 12, 2017 at 06:22:37PM +0100, Steffan Karger wrote:
[..]

src/openvpn/push.c prepare_push_reply(struct context *c, struct gc_arena *gc, · line 370

Quoted 10 lines

src/openvpn/push.c +1 -0

index 5947a31f..16a4101f 100644
@@ -366,6 +366,7 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
             push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
                             tls_multi->peer_id);
         }
+        tls_multi->use_peer_id = true;
     }

While this *works* today, it relies on the fact that only clients that can do IV_PROTO=2 will ever send IV_PROTO=<anything>.

But it still creates weird code (more context):

     /* Send peer-id if client supports it */
     optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
     if (optstr)
     {   
         int proto = 0;
         int r = sscanf(optstr, "IV_PROTO=%d", &proto);
         if ((r == 1) && (proto >= 2))
         {   
             push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
                             tls_multi->peer_id);
         }
+        tls_multi->use_peer_id = true;
     }

where we set tls_multi->use_peer_id = true for any client that sends IV_PROTO=<anything>, but the rules for "push a peer ID to the client" is much stricter, requiring it to be IV_PROTO=<num> with num>=2 - so the "tls_multi->use_peer_id = true;" should go inside that block.

gert

Signature · 3 lines

now what should I write here...

Gert Doering - Munich, Germany                             [email protected]...

Steffan Karger

2017-11-24 13:58 UTC #5 lore raw

PATCH v3 1/1 Use P_DATA_V2 for server->client packets too

From: Steffan Karger <[email protected]...>

Signed-off-by Steffan Karger <[email protected]...>

---
v2: actually enable P_DATA_V2... Now tested with:

    2.4<>2.4 (V2), 2.4-srv<>2.3-clt (V2), 2.3-srv<>2.4-clt (V1),
    2.4-srv<>2.2-clt (V1)

v3: move "use_peer_id = true" inside "if IV_PROTO >= 2" (thanks Gert)

 src/openvpn/forward.c | 4 ++--
 src/openvpn/push.c    | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

src/openvpn/forward.c +2 -2

index 1b7455b..a868a8f 100644
@@ -496,7 +496,7 @@ encrypt_sign(struct context *c, bool comp_frag)
         /* If using P_DATA_V2, prepend the 1-byte opcode and 3-byte peer-id to the
          * packet before openvpn_encrypt(), so we can authenticate the opcode too.
          */
-        if (c->c2.buf.len > 0 && !c->c2.tls_multi->opt.server && c->c2.tls_multi->use_peer_id)
+        if (c->c2.buf.len > 0 && c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v2(c->c2.tls_multi, &b->encrypt_buf);
         }

@@ -512,7 +512,7 @@ encrypt_sign(struct context *c, bool comp_frag)
     /* Do packet administration */
     if (c->c2.tls_multi)
     {
-        if (c->c2.buf.len > 0 && (c->c2.tls_multi->opt.server || !c->c2.tls_multi->use_peer_id))
+        if (c->c2.buf.len > 0 && !c->c2.tls_multi->use_peer_id)
         {
             tls_prepend_opcode_v1(c->c2.tls_multi, &c->c2.buf);
         }

src/openvpn/push.c +1 -0

index 5947a31..e7aecbb 100644
@@ -365,6 +365,7 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
         {
             push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
                             tls_multi->peer_id);
+            tls_multi->use_peer_id = true;
         }
     }

Signature · 1 line

2.7.4

Gert Doering

2017-11-24 14:28 UTC #6 lore raw

ACK. Patch looks good and passes t_server tests (2.2, 2.3, 2.4 clients).

(Also registering Antonios ACK, as the v2 code was technically working, and this is just getting it fully right)

Since the patch is fairly trivial I've let myself be convinced that it's a long-term support thing to have this in 2.4 (namely: get rid of P_DATA_V1 as soon as everyone has migrated to 2.4, not having to wait until after 2.5).

Your patch has been applied to the master and release/2.4 branch.

commit 3b9cce657b0ba876c56ee6f14664a8a77f5b82d5 (master) commit 6232c7b3827f90d9b25135c523703204bc2095c0 (release/2.4)

Author: Steffan Karger
Date:   Fri Nov 24 14:58:23 2017 +0100

     Use P_DATA_V2 for server->client packets too

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Antonio Quartulli <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/search?l=mid&[email protected]...
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering