[Openvpn-devel,v2] Use P_DATA_V2 for server->client packets too

Message ID 20171112172237.8285-1-steffan@karger.me
State Superseded
Headers show
Series
  • [Openvpn-devel,v2] Use P_DATA_V2 for server->client packets too
Related show

Commit Message

Steffan Karger Nov. 12, 2017, 5:22 p.m.
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 <steffan@karger.me>
---
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(-)

Comments

Antonio Quartulli Nov. 12, 2017, 5:32 p.m. | #1
Hi,

On 13/11/17 01:22, Steffan Karger wrote:
> 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 <steffan@karger.me>

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

Acked-by: Antonio Quartulli <a@unstable.cc>
Gert Doering Nov. 24, 2017, 1:06 p.m. | #2
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:
[..]
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 5947a31f..16a4101f 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -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

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 1b7455bb..a868a8ff 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -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);
         }
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 5947a31f..16a4101f 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -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 */