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

Message ID 20171111160359.28078-1-steffan@karger.me
State Superseded
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel] Use P_DATA_V2 for server->client packets too
Related show

Commit Message

Steffan Karger Nov. 11, 2017, 4:03 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>
---
 src/openvpn/forward.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Antonio Quartulli Nov. 12, 2017, 3:33 p.m. | #1
Hi,

On 12/11/17 00:03, 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>
> ---
>  src/openvpn/forward.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 1b7455bb..20e89d34 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);
>          }

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,

Patch

diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 1b7455bb..20e89d34 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);
         }