[Openvpn-devel,S] Change in openvpn[master]: Improve data channel crypto error messages

Message ID c3f2b7b20d867f4cd686a918ec93f53869ec40ff-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,S] Change in openvpn[master]: Improve data channel crypto error messages | expand

Commit Message

cron2 (Code Review) Sept. 29, 2024, 8:18 a.m. UTC
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

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

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

to review the following change.


Change subject: Improve data channel crypto error messages
......................................................................

Improve data channel crypto error messages

 * Make decryption error messages better understandable.
 * Increase verbosity level for authentication errors, because those can
   be expected on bad connections.

Change-Id: I0fd48191babe4fe5c56f10eb3ba88182ffb075d1
Signed-off-by: Steffan Karger <steffan@karger.me>
---
M src/openvpn/crypto.c
M src/openvpn/crypto.h
2 files changed, 12 insertions(+), 9 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/74/774/1

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 12ad0b9..064e59e 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -459,14 +459,14 @@ 
     if (!cipher_ctx_update(ctx->cipher, BPTR(&work), &outlen, BPTR(buf),
                            data_len))
     {
-        CRYPT_ERROR("cipher update failed");
+        CRYPT_ERROR("packet decryption failed");
     }
 
     ASSERT(buf_inc_len(&work, outlen));
     if (!cipher_ctx_final_check_tag(ctx->cipher, BPTR(&work) + outlen,
                                     &outlen, tag_ptr, tag_size))
     {
-        CRYPT_ERROR("cipher final failed");
+        CRYPT_DROP("packet tag authentication failed");
     }
     ASSERT(buf_inc_len(&work, outlen));
 
@@ -538,7 +538,7 @@ 
             /* Compare locally computed HMAC with packet HMAC */
             if (memcmp_constant_time(local_hmac, BPTR(buf), hmac_len))
             {
-                CRYPT_ERROR("packet HMAC authentication failed");
+                CRYPT_DROP("packet HMAC authentication failed");
             }
 
             ASSERT(buf_advance(buf, hmac_len));
@@ -572,26 +572,26 @@ 
             /* ctx->cipher was already initialized with key & keylen */
             if (!cipher_ctx_reset(ctx->cipher, iv_buf))
             {
-                CRYPT_ERROR("cipher init failed");
+                CRYPT_ERROR("decrypt initialization failed");
             }
 
             /* Buffer overflow check (should never happen) */
             if (!buf_safe(&work, buf->len + cipher_ctx_block_size(ctx->cipher)))
             {
-                CRYPT_ERROR("potential buffer overflow");
+                CRYPT_ERROR("packet too big to decrypt");
             }
 
             /* Decrypt packet ID, payload */
             if (!cipher_ctx_update(ctx->cipher, BPTR(&work), &outlen, BPTR(buf), BLEN(buf)))
             {
-                CRYPT_ERROR("cipher update failed");
+                CRYPT_ERROR("packet decryption failed");
             }
             ASSERT(buf_inc_len(&work, outlen));
 
             /* Flush the decryption buffer */
             if (!cipher_ctx_final(ctx->cipher, BPTR(&work) + outlen, &outlen))
             {
-                CRYPT_ERROR("cipher final failed");
+                CRYPT_DROP("packet authentication failed, dropping.");
             }
             ASSERT(buf_inc_len(&work, outlen));
 
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 61184bc..d91de74 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -288,8 +288,11 @@ 
                                  *   security operation functions. */
 };
 
-#define CRYPT_ERROR(format) \
-    do { msg(D_CRYPT_ERRORS, "%s: " format, error_prefix); goto error_exit; } while (false)
+#define CRYPT_ERROR_EXIT(flags, format) \
+    do { msg(flags, "%s: " format, error_prefix); goto error_exit; } while (false)
+
+#define CRYPT_ERROR(format) CRYPT_ERROR_EXIT(D_CRYPT_ERRORS, format)
+#define CRYPT_DROP(format) CRYPT_ERROR_EXIT(D_MULTI_DROPPED, format)
 
 /**
  * Minimal IV length for AEAD mode ciphers (in bytes):