Fix overflow check in openvpn_decrypt()

PATCH Superseded [email protected]... 3 participants 4 messages Jul 1, 2015 – Aug 1, 2015

lore

Share & tools

lore

[email protected]...

author

2015-07-01 11:03 UTC #0 lore raw

PATCH 1/1 Fix overflow check in openvpn_decrypt()

From: Steffan Karger <[email protected]...>

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.

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/crypto.c         | 21 +++++++++++----------
 src/openvpn/crypto_backend.h |  2 +-
 2 files changed, 12 insertions(+), 11 deletions(-)

src/openvpn/crypto.c +11 ~~-10~~

index d15c85a..49e61d3 100644
@@ -156,11 +156,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 ||

@@ -298,18 +298,20 @@ 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;
+	  if (!buf_inc_len(&work, outlen))
+	    CRYPT_ERROR ("cipher update buffer overflow");
 
 	  /* Flush the decryption buffer */
 	  if (!cipher_ctx_final (ctx->cipher, BPTR (&work) + outlen, &outlen))
 	    CRYPT_ERROR ("cipher final failed");
-	  work.len += outlen;
+	  if (!buf_inc_len(&work, outlen))
+	    CRYPT_ERROR ("cipher final buffer overflow");
 
 	  dmsg (D_PACKET_CONTENT, "DECRYPT TO: %s",
 	       format_hex (BPTR (&work), BLEN (&work), 80, &gc));

@@ -395,9 +397,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;

src/openvpn/crypto_backend.h +1 -1

index 4e45df0..4c1ce9f 100644
@@ -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

Signature · 1 line

2.1.0

[email protected]...

author

2015-07-29 10:30 UTC #1 lore raw

PATCH v2 1/1 Fix overflow check in openvpn_decrypt()

From: Steffan Karger <[email protected]...>

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:

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 <[email protected]...>

---
 src/openvpn/crypto.c         | 19 +++++++++----------
 src/openvpn/crypto_backend.h |  2 +-
 2 files changed, 10 insertions(+), 11 deletions(-)

src/openvpn/crypto.c +9 ~~-10~~

index d15c85a..aab53f2 100644
@@ -156,11 +156,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 ||

@@ -298,18 +298,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));

@@ -395,9 +395,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;

src/openvpn/crypto_backend.h +1 -1

index 4e45df0..4c1ce9f 100644
@@ -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

Signature · 1 line

2.1.0

Gert Doering

2015-08-01 14:47 UTC #2 lore raw

Your patch has been applied to the master and release/2.3 branch.

commit cc377dec820f9e6e7e72981013eb3857aa6ea5ce (master) commit 338f11821fa7ce95c768b0cc074f3af7346b19fa (release/2.3)

Author: Steffan Karger
Date:   Wed Jul 29 12:30:26 2015 +0200

     Fix overflow check in openvpn_decrypt()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/9974
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

Arne Schwabe

2015-08-01 12:57 UTC #3 lore raw

In reply to [email protected]...

Am 01.07.15 um 13:03 schrieb [email protected]...:

Quoted 21 lines

From: Steffan Karger <[email protected]...>
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.

Quoted 8 lines

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.

ACK from me.

Arne