Fix potential null-pointer dereference

Commit a070f75b (master branch only) changed the openvpn_encrypt logic and
now prepends the contents of the work buffer to buf if no encryption is
used (which is the case for tls-auth packets).  In that case, the code
would potentially dereference a null-pointer in a memcpy(some-dest, 0, 0)
call.  Fortunately, memcpy() inplementations usually do not actually
derefence the src (or dst) pointer for zero-length copies.

And since I'm touching this code now anyway, remove a slightly confusing
jump back to a cleanup label in openvpn_encrypt_aead().

Issue spotted by Daniel Hirche.

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1459528980-8304-1-git-send-email-steffan@karger.me>
URL: http://article.gmane.org/gmane.network.openvpn.devel/11372
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index f15ac351101c5873fa89a45683eebbb7b2ceb10e..5c392aad34287c7149173e2b7c19dbdea9fec3a6 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -166,14 +166,14 @@ openvpn_encrypt_aead (struct buffer *buf, struct buffer work,
 
   dmsg (D_PACKET_CONTENT, "ENCRYPT TO: %s", format_hex (BPTR (buf), BLEN (buf), 80, &gc));
 
-cleanup:
   gc_free (&gc);
   return;
 
 err:
   crypto_clear_error();
   buf->len = 0;
-  goto cleanup;
+  gc_free (&gc);
+  return;
 #else /* HAVE_AEAD_CIPHER_MODES */
   ASSERT (0);
 #endif
@@ -295,7 +295,9 @@ openvpn_encrypt_v1 (struct buffer *buf, struct buffer work,
              hmac_start = BPTR(buf);
              ASSERT (mac_out = buf_prepend (buf, hmac_ctx_size(ctx->hmac)));
            }
-         buf_write_prepend(buf, BPTR(&work), BLEN(&work));
+         if (BLEN(&work)) {
+           buf_write_prepend(buf, BPTR(&work), BLEN(&work));
+         }
          work = *buf;
        }