Fix overflow check in openvpn_decrypt()

Sebastian Krahmer from the SuSE security team reported that the buffer
overflow check in openvpn_decrypt() was too strict according to the
cipher update function contract:

"The amount of data written depends on the block alignment of the
encrypted data: as a result the amount of data written may be anything
from zero bytes to (inl + cipher_block_size - 1) so outl should contain
sufficient room."

This stems from the way CBC mode works, which caches input and 'flushes'
it block-wise to the output buffer.  We do allocate enough space for this
extra block in the output buffer for CBC mode, but not for CFB/OFB modes.

This patch:
 * updates the overflow check to also verify that the extra block required
   according to the function contract is available.
 * uses buf_inc_len() to double-check for overflows during en/decryption.
 * also reserves the extra block for non-CBC cipher modes.

In practice, I could not find a way in which this would fail. The plaintext
is never longer than the ciphertext, and the implementations of CBC/OFB/CBC
for AES and BF in both OpenSSL and PolarSSL/mbed TLS do not use the buffer
beyond the plaintext length when decrypting.  However, some funky OpenSSL
engine I did not check *might* use the buffer space required by the
function contract.  So we should still make sure we have enough room
anyway.

v2 - always ASSERT() on buf_inc_len().  It is a double-check so should
     really not fail, but if it fails there has been a buffer overflow.
     At that point the best thing we can do is assert out. (The primary
     check *is* handled gracefully, and just drops the packet.)

Signed-off-by: Steffan Karger <steffan.karger@fox-it.com>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <1438165826-32762-1-git-send-email-steffan.karger@fox-it.com>
URL: http://article.gmane.org/gmane.network.openvpn.devel/9974
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 588d9f05436d92e29075d63cdf61d6e02b16b652..1ceb411a4e5263cdf119f927b3ee95c40106c8da 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -166,11 +166,11 @@ openvpn_encrypt (struct buffer *buf, struct buffer work,
 
          /* Encrypt packet ID, payload */
          ASSERT (cipher_ctx_update (ctx->cipher, BPTR (&work), &outlen, BPTR (buf), BLEN (buf)));
-         work.len += outlen;
+         ASSERT (buf_inc_len(&work, outlen));
 
          /* Flush the encryption buffer */
-         ASSERT(cipher_ctx_final(ctx->cipher, BPTR (&work) + outlen, &outlen));
-         work.len += outlen;
+         ASSERT (cipher_ctx_final(ctx->cipher, BPTR (&work) + outlen, &outlen));
+         ASSERT (buf_inc_len(&work, outlen));
 
          /* For all CBC mode ciphers, check the last block is complete */
          ASSERT (cipher_kt_mode (cipher_kt) != OPENVPN_MODE_CBC ||
@@ -305,18 +305,18 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
            CRYPT_ERROR ("cipher init failed");
 
          /* Buffer overflow check (should never happen) */
-         if (!buf_safe (&work, buf->len))
-           CRYPT_ERROR ("buffer overflow");
+         if (!buf_safe (&work, buf->len + cipher_ctx_block_size(ctx->cipher)))
+           CRYPT_ERROR ("potential buffer overflow");
 
          /* Decrypt packet ID, payload */
          if (!cipher_ctx_update (ctx->cipher, BPTR (&work), &outlen, BPTR (buf), BLEN (buf)))
            CRYPT_ERROR ("cipher update failed");
-         work.len += outlen;
+         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");
-         work.len += outlen;
+         ASSERT (buf_inc_len(&work, outlen));
 
          dmsg (D_PACKET_CONTENT, "DECRYPT TO: %s",
               format_hex (BPTR (&work), BLEN (&work), 80, &gc));
@@ -413,9 +413,8 @@ crypto_adjust_frame_parameters(struct frame *frame,
       if (use_iv)
        crypto_overhead += cipher_kt_iv_size (kt->cipher);
 
-      if (cipher_kt_mode_cbc (kt->cipher))
-       /* worst case padding expansion */
-       crypto_overhead += cipher_kt_block_size (kt->cipher);
+      /* extra block required by cipher_ctx_update() */
+      crypto_overhead += cipher_kt_block_size (kt->cipher);
     }
 
   crypto_overhead += kt->hmac_length;

diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 4e45df00a044102e7ae5243979b04f836dad6c82..4c1ce9fa6aa84302c2f1a9a04ac35d469a16fd36 100644 (file)
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -333,7 +333,7 @@ int cipher_ctx_reset (cipher_ctx_t *ctx, uint8_t *iv_buf);
  * Note that if a complete block cannot be written, data is cached in the
  * context, and emitted at a later call to \c cipher_ctx_update, or by a call
  * to \c cipher_ctx_final(). This implies that dst should have enough room for
- * src_len + \c cipher_ctx_block_size() - 1.
+ * src_len + \c cipher_ctx_block_size().
  *
  * @param ctx          Cipher's context. May not be NULL.
  * @param dst          Destination buffer