diff mbox series

[Openvpn-devel,M] Change in openvpn[master]: Use XOR instead of concatenation for calculation of IV from implicit IV

Message ID 5719683e0d368a4edae012ee66a335b2d08f57aa-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: Use XOR instead of concatenation for calculation of IV from implicit IV | expand

Commit Message

mrbff (Code Review) Nov. 11, 2024, 1:59 a.m. UTC 
Attention is currently required from: flichtenheld.

Hello flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/797?usp=email

to review the following change.


Change subject: Use XOR instead of concatenation for calculation of  IV from implicit IV
......................................................................

Use XOR instead of concatenation for calculation of  IV from implicit IV

This change prepares the extended packet id data where also the packet id
part of the IV will be derived using xor.  Using xor also in the AEAD
case where this degenerates to a concatenation allows using the same
IV generation code later.

Change-Id: I74216d776d3e0a8dc987ec7b1671c8e8dcccdbd6
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
M src/openvpn/crypto.c
M src/openvpn/crypto.h
M src/openvpn/dco_freebsd.c
M src/openvpn/dco_linux.c
M src/openvpn/dco_win.c
M src/openvpn/ssl.c
M tests/unit_tests/openvpn/test_ssl.c
7 files changed, 37 insertions(+), 23 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/97/797/1
diff mbox series

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index d136663..a366474 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -95,12 +95,16 @@ 
             goto err;
         }
 
-        /* Remainder of IV consists of implicit part (unique per session) */
-        ASSERT(buf_write(&iv_buffer, ctx->implicit_iv, ctx->implicit_iv_len));
-        ASSERT(iv_buffer.len == iv_len);
+        /* Write packet id part of IV to work buffer */
+        ASSERT(buf_write(&work, iv, packet_id_size(false)));
 
-        /* Write explicit part of IV to work buffer */
-        ASSERT(buf_write(&work, iv, iv_len - ctx->implicit_iv_len));
+        /* Remainder of IV consists of implicit part (unique per session)
+         * XOR of packet and implicit IV */
+        for (int i = 0; i < iv_len; i++)
+        {
+            iv[i] = iv[i] ^ ctx->implicit_iv[i];
+        }
+
         dmsg(D_PACKET_CONTENT, "ENCRYPT IV: %s", format_hex(iv, iv_len, 0, &gc));
 
         /* Init cipher_ctx with IV.  key & keylen are already initialized */
@@ -428,16 +432,21 @@ 
     {
         uint8_t iv[OPENVPN_MAX_IV_LENGTH] = { 0 };
         const int iv_len = cipher_ctx_iv_length(ctx->cipher);
-        const size_t packet_iv_len = iv_len - ctx->implicit_iv_len;
+        const size_t packet_iv_len = packet_id_size(false);
 
-        ASSERT(ctx->implicit_iv_len <= iv_len);
-        if (buf->len + ctx->implicit_iv_len < iv_len)
+        if (buf->len < packet_id_size(false))
         {
             CRYPT_ERROR("missing IV info");
         }
 
         memcpy(iv, BPTR(buf), packet_iv_len);
-        memcpy(iv + packet_iv_len, ctx->implicit_iv, ctx->implicit_iv_len);
+
+        /* Remainder of IV consists of implicit part (unique per session)
+         * XOR of packet counter and implicit IV */
+        for (int i = 0; i < iv_len; i++)
+        {
+            iv[i] = iv[i] ^ ctx->implicit_iv[i];
+        }
 
         dmsg(D_PACKET_CONTENT, "DECRYPT IV: %s", format_hex(iv, iv_len, 0, &gc));
 
@@ -963,7 +972,6 @@ 
         hmac_ctx_free(ctx->hmac);
         ctx->hmac = NULL;
     }
-    ctx->implicit_iv_len = 0;
 }
 
 void
@@ -1079,18 +1087,15 @@ 
         cipher_ctx_t *cipher = co->key_ctx_bi.encrypt.cipher;
         if (cipher_ctx_mode_aead(cipher))
         {
-            size_t impl_iv_len = cipher_ctx_iv_length(cipher) - sizeof(packet_id_type);
             ASSERT(cipher_ctx_iv_length(cipher) <= OPENVPN_MAX_IV_LENGTH);
             ASSERT(cipher_ctx_iv_length(cipher) >= OPENVPN_AEAD_MIN_IV_LEN);
 
             /* Generate dummy implicit IV */
             ASSERT(rand_bytes(co->key_ctx_bi.encrypt.implicit_iv,
                               OPENVPN_MAX_IV_LENGTH));
-            co->key_ctx_bi.encrypt.implicit_iv_len = impl_iv_len;
 
             memcpy(co->key_ctx_bi.decrypt.implicit_iv,
                    co->key_ctx_bi.encrypt.implicit_iv, OPENVPN_MAX_IV_LENGTH);
-            co->key_ctx_bi.decrypt.implicit_iv_len = impl_iv_len;
         }
     }
 
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 7b0f713..0ae86f4 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -163,9 +163,14 @@ 
 {
     cipher_ctx_t *cipher;       /**< Generic cipher %context. */
     hmac_ctx_t *hmac;           /**< Generic HMAC %context. */
+    /**
+     * This implicit IV will be always XORed with the packet id that is sent on
+     * the wire to get the IV.  For the older AEAD format the first 32 bits
+     * of implicit_iv are always 0 so this works as concatenation: after xor
+     * the lower 32 bit of the IV are the packet id and the rest of the IV is
+     * from the implicit IV.
+     */
     uint8_t implicit_iv[OPENVPN_MAX_IV_LENGTH];
-    /**< The implicit part of the IV */
-    size_t implicit_iv_len;     /**< The length of implicit_iv */
     /** Counter for the number of plaintext encrypted using this cipher
      * in number of 128 bit blocks (only used for AEAD ciphers) */
     uint64_t plaintext_blocks;
diff --git a/src/openvpn/dco_freebsd.c b/src/openvpn/dco_freebsd.c
index f4c3b02..bdde16f 100644
--- a/src/openvpn/dco_freebsd.c
+++ b/src/openvpn/dco_freebsd.c
@@ -395,7 +395,9 @@ 
         key_len = cipher_kt_key_size(ciphername);
 
         nvlist_add_binary(nvl, "key", key, key_len);
-        nvlist_add_binary(nvl, "iv", implicit_iv, 8);
+        /* FreeBSD uses the contact operation, need to skip the first 4 null
+         * bytes */
+        nvlist_add_binary(nvl, "iv", implicit_iv + 4, 8);
     }
 
     return (nvl);
diff --git a/src/openvpn/dco_linux.c b/src/openvpn/dco_linux.c
index b038382..c3e2ecc 100644
--- a/src/openvpn/dco_linux.c
+++ b/src/openvpn/dco_linux.c
@@ -585,8 +585,9 @@ 
     if (dco_cipher != OVPN_CIPHER_ALG_NONE)
     {
         NLA_PUT(nl_msg, OVPN_KEY_DIR_ATTR_CIPHER_KEY, key_len, encrypt_key);
+        /* First 4 zero bytes as the kernel does concat instead of XOR */
         NLA_PUT(nl_msg, OVPN_KEY_DIR_ATTR_NONCE_TAIL, nonce_tail_len,
-                encrypt_iv);
+                encrypt_iv + 4);
     }
     nla_nest_end(nl_msg, key_enc);
 
@@ -595,8 +596,9 @@ 
     if (dco_cipher != OVPN_CIPHER_ALG_NONE)
     {
         NLA_PUT(nl_msg, OVPN_KEY_DIR_ATTR_CIPHER_KEY, key_len, decrypt_key);
+        /* First 4 zero bytes as the kernel does concat instead of XOR */
         NLA_PUT(nl_msg, OVPN_KEY_DIR_ATTR_NONCE_TAIL, nonce_tail_len,
-                decrypt_iv);
+                decrypt_iv + 4);
     }
     nla_nest_end(nl_msg, key_dec);
 
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 9224bca..8f07ead 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -314,10 +314,12 @@ 
 
     CopyMemory(crypto_data.Encrypt.Key, encrypt_key, key_len);
     crypto_data.Encrypt.KeyLen = (char)key_len;
+    /* First 4 zero bytes as ovpn-dco-win does concat instead of XOR */
     CopyMemory(crypto_data.Encrypt.NonceTail, encrypt_iv, nonce_len);
 
     CopyMemory(crypto_data.Decrypt.Key, decrypt_key, key_len);
     crypto_data.Decrypt.KeyLen = (char)key_len;
+    /* First 4 zero bytes as ovpn-dco-win does concat instead of XOR */
     CopyMemory(crypto_data.Decrypt.NonceTail, decrypt_iv, nonce_len);
 
     ASSERT(crypto_data.CipherAlg > 0);
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index a8cc83b..2d6cf64 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1562,8 +1562,9 @@ 
         impl_iv_len = cipher_ctx_iv_length(ctx->cipher) - sizeof(packet_id_type);
         ASSERT(impl_iv_len <= OPENVPN_MAX_IV_LENGTH);
         ASSERT(impl_iv_len <= key_len);
-        memcpy(ctx->implicit_iv, key, impl_iv_len);
-        ctx->implicit_iv_len = impl_iv_len;
+        CLEAR(ctx->implicit_iv);
+        /* The first bytes of the IV are filled with the packet id */
+        memcpy(ctx->implicit_iv + sizeof(packet_id_type), key, impl_iv_len);
     }
 }
 
diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c
index a1ca344..ae33cc6 100644
--- a/tests/unit_tests/openvpn/test_ssl.c
+++ b/tests/unit_tests/openvpn/test_ssl.c
@@ -284,18 +284,15 @@ 
 
     if (cipher_ctx_mode_aead(cipher))
     {
-        size_t impl_iv_len = cipher_ctx_iv_length(cipher) - sizeof(packet_id_type);
         ASSERT(cipher_ctx_iv_length(cipher) <= OPENVPN_MAX_IV_LENGTH);
         ASSERT(cipher_ctx_iv_length(cipher) >= OPENVPN_AEAD_MIN_IV_LEN);
 
         /* Generate dummy implicit IV */
         ASSERT(rand_bytes(co->key_ctx_bi.encrypt.implicit_iv,
                           OPENVPN_MAX_IV_LENGTH));
-        co->key_ctx_bi.encrypt.implicit_iv_len = impl_iv_len;
 
         memcpy(co->key_ctx_bi.decrypt.implicit_iv,
                co->key_ctx_bi.encrypt.implicit_iv, OPENVPN_MAX_IV_LENGTH);
-        co->key_ctx_bi.decrypt.implicit_iv_len = impl_iv_len;
     }
 }