Limit --reneg-bytes to 64MB when using small block ciphers

PATCH In review Steffan Karger 3 participants 10 messages Oct 26, 2016 – Oct 28, 2016

Share & tools

Steffan Karger

author

2016-10-26 15:47 UTC #0 lore raw

PATCH 1/1 Limit --reneg-bytes to 64MB when using small block ciphers

Following the earlier warning about small block ciphers, now limit the --reneg-bytes value when using a cipher that susceptible to SWEET32-like attacks. The 64 MB value has been selected with the researchers who published the SWEET32 paper.

Note that this will not change a user-set (non-zero) --reneg-bytes value, to allow a user to override this behaviour if really needed.

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

---
 src/openvpn/crypto.c |  5 +++--
 src/openvpn/ssl.c    | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)

src/openvpn/crypto.c +3 -2

index 45689f2..b40fc7a 100644
@@ -836,8 +836,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 "
+	      "bit (%d bit).  This allows attacks like SWEET32.  Consider "
+	      "using a --cipher with a larger block size.",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }

src/openvpn/ssl.c +24 -0

index dfdc200..2c341a2 100644
@@ -283,6 +283,24 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**
+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (*reneg_bytes == 0 && cipher && (cipher_kt_block_size(cipher) < 128/8))
+    {
+      msg (M_WARN, "WARNING: cipher with small block size in use, "
+	   "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");
+      *reneg_bytes = 64 * 1024 * 1024;
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1742,6 +1760,8 @@ tls_session_update_crypto_params(struct tls_session *session,
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
     }
+  tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			 &session->opt->renegotiate_bytes);
   ret = true;
 cleanup:
   CLEAR (*ks->key_src);

@@ -2126,6 +2146,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 		      
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2354,6 +2376,8 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
 	}
 
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   gc_free (&gc);

Signature · 1 line

2.7.4

Steffan Karger

author

2016-10-26 15:49 UTC #1 lore raw

PATCH 1/1 Limit --reneg-bytes to 64MB when using small block ciphers

Note that this will not change a user-set (non-zero) --reneg-bytes value, to allow a user to override this behaviour if really needed.

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

---
 src/openvpn/crypto.c |  5 +++--
 src/openvpn/ssl.c    | 22 ++++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

src/openvpn/crypto.c +3 -2

index 552e333..56838af 100644
@@ -496,8 +496,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 "
+	      "bit (%d bit).  This allows attacks like SWEET32.  Consider "
+	      "using a --cipher with a larger block size.",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }

src/openvpn/ssl.c +22 -0

index 33fd9dd..08cc10c 100644
@@ -271,6 +271,24 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**
+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (*reneg_bytes == 0 && cipher && (cipher_kt_block_size(cipher) < 128/8))
+    {
+      msg (M_WARN, "WARNING: cipher with small block size in use, "
+	   "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");
+      *reneg_bytes = 64 * 1024 * 1024;
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1956,6 +1974,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 		      
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2166,6 +2186,8 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
 	}
 		      
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   gc_free (&gc);

Signature · 1 line

2.7.4

Steffan Karger

author

2016-10-28 12:10 UTC #2 lore raw

PATCH v2 1/1 Limit --reneg-bytes to 64MB when using small block ciphers

Note that this will not change a user-set --reneg-bytes value, to allow a user to align a gun with his feet^w^w^w^w^w^w override this behaviour if really needed.

Furthermore, in contrast with the patch for master, this will not limit --reneg-bytes on the client side. This allows server administrators to revert to the old behaviour, or increase --reneg-bytes to something they believe is workable, without having to change client configs. (The master branch provides cipher negotiation as a real solution, so we can be stricter there.)

v2: obey user-set --reneg-bytes 0 to revert to old behaviour, use more firm
language in warning message, add URL to man page, and only limit at the server side.

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

---
 doc/openvpn.8         |  1 +
 src/openvpn/crypto.c  |  5 +++--
 src/openvpn/options.c |  1 +
 src/openvpn/ssl.c     | 25 ++++++++++++++++++++++++-
 4 files changed, 29 insertions(+), 3 deletions(-)

doc/openvpn.8 +1 -0

index 70573da..7be30ec 100644
@@ -3913,6 +3913,7 @@ an abbreviation for Blowfish in Cipher Block Chaining mode.
 
 Using BF-CBC is no longer recommended, because of it's 64-bit block size.  This
 small block size allows attacks based on collisions, as demonstrated by SWEET32.
+See https://community.openvpn.net/openvpn/wiki/SWEET32 for details.
 
 To see other ciphers that are available with OpenVPN, use the
 .B \-\-show\-ciphers

src/openvpn/crypto.c +3 -2

index 552e333..ca4af27 100644
@@ -496,8 +496,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: INSECURE cipher with block size less than 128"
+	      " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
+	      "using a --cipher with a larger block size (e.g. AES-256-CBC).",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }

src/openvpn/options.c +1 -0

index 1ef0299..19cd815 100644
@@ -849,6 +849,7 @@ init_options (struct options *o, const bool init_gc)
 #ifdef ENABLE_SSL
   o->key_method = 2;
   o->tls_timeout = 2;
+  o->renegotiate_bytes = -1;
   o->renegotiate_seconds = 3600;
   o->handshake_window = 60;
   o->transition_window = 3600;

src/openvpn/ssl.c +24 -1

index 33fd9dd..bfad291 100644
@@ -271,6 +271,27 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**
+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (cipher && (cipher_kt_block_size(cipher) < 128/8))
+    {
+      if (*reneg_bytes == -1) /* Not user-specified */
+	{
+	  msg (M_WARN, "WARNING: cipher with small block size in use, "
+	       "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");
+	  *reneg_bytes = 64 * 1024 * 1024;
+	}
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1956,6 +1977,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 		      
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2222,7 +2245,7 @@ tls_process (struct tls_multi *multi,
   if (ks->state >= S_ACTIVE &&
       ((session->opt->renegotiate_seconds
 	&& now >= ks->established + session->opt->renegotiate_seconds)
-       || (session->opt->renegotiate_bytes
+       || (session->opt->renegotiate_bytes > 0
 	   && ks->n_bytes >= session->opt->renegotiate_bytes)
        || (session->opt->renegotiate_packets
 	   && ks->n_packets >= session->opt->renegotiate_packets)

Signature · 1 line

2.7.4

David Sommerseth

2016-10-28 13:16 UTC #3 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

ACK.

Your patch has been applied to the branch

commit a91ddc99a524014ec79560d873721e8fa81a5631

Author: Steffan Karger
Date:   Fri Oct 28 14:10:07 2016 +0200

     Limit --reneg-bytes to 64MB when using small block ciphers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12799.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYE0+kAAoJEIbPlEyWcf3yMJYP/0owxGi9kgHWnU6eg/9yFxgj GORwUBZFTOCbj6funmVinJ43EWLXY78QcmUcSgiCCOOmapA7IByWNo/4KBMd9ypD h5hVcnosKEQmcp7hBS8bDV5xb0X+R7JnDUvw1ErGsU2a15C2+80lEg86HX70bkg5 MtkxYY+FK3h9RO53T4a2+RTctukikbfo7Hd3HkDo+MaNY1ZsEwM/GytBko4tHTql fwGhbCBmL5o8MK9l3M8AUVBgMa5lPvvwnINP0dLyx4obyAscLnQtxyEN63j7CPx6 JjUwWpycVN7jERm78l/kce4/CYSwAtvbYqnQAvn2dioiwSyk155vDYmp9Jslv30b ojmMpMGaewVTuAUS5/MpE7qNfw2Pl1EBDs8nTfYDvYROIQ+6d1iZ7gLdqJ8Xd75m c5e9yVkOcyXxLHgeKduCnQYfLmnH+kSQSFnToLupENqeJSjqLD4uiAGwBGwlBp2J yzbuf0GFvmRcQ0Rjtg26VEocfuASLMnNDK7s4u3iCGPqhvq3umBwIqynBSiPGMbu qgX/ottDL3QcIZoHIRU2SphQ+dosXwEvnm9zg9RLkFvf7ZJX+02mVTHT/tPkLE37 1q7WQ27Owwm9OkjLrV+wlLbZgOvXWNcpJMMXO4wTyxkGJ0NESjkBt67oY+5ov83F AEWrOrgWpnoRGUxs/x0h
=WdOO
-----END PGP SIGNATURE-----

David Sommerseth

2016-10-28 13:31 UTC #4 lore raw

Quoted 3 lines · David Sommerseth

ACK.
Your patch has been applied to the branch

This was pushed to the release/2.3 branch.

Quoted 12 lines

commit a91ddc99a524014ec79560d873721e8fa81a5631

Author: Steffan Karger
Date:   Fri Oct 28 14:10:07 2016 +0200

     Limit --reneg-bytes to 64MB when using small block ciphers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]......>
     URL: https://www.mail-archive.com/[email protected]/msg12799.html
     Signed-off-by: David Sommerseth <[email protected]...>

Signature · 3 lines

kind regards,

David Sommerseth

David Sommerseth

2016-10-27 14:18 UTC #5 lore raw

In reply to Steffan Karger

Quoted 39 lines · Steffan Karger

Following the earlier warning about small block ciphers, now limit the --reneg-bytes value when using a cipher that susceptible to SWEET32-like attacks. The 64 MB value has been selected with the researchers who published the SWEET32 paper.
Note that this will not change a user-set (non-zero) --reneg-bytes value, to allow a user to override this behaviour if really needed.
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/crypto.c |  5 +++--
 src/openvpn/ssl.c    | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
src/openvpn/crypto.c +3 -2
index 45689f2..b40fc7a 100644
@@ -836,8 +836,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 "
+	      "bit (%d bit).  This allows attacks like SWEET32.  Consider "
+	      "using a --cipher with a larger block size.",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }
src/openvpn/ssl.c +1 -0
index dfdc200..2c341a2 100644
@@ -283,6 +283,24 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**

src/openvpn/ssl.c tls_get_cipher_name_pair (const char * cipher_name, size_t len) { · line 296

Quoted 10 lines

src/openvpn/ssl.c +10 -0

+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (*reneg_bytes == 0 && cipher && (cipher_kt_block_size(cipher) < 128/8))

Should we also consider if *reneg_bytes > 64*1024*1024 and enforce maximum? This will not allow users to override the 64MB limit, though.

Perhaps we should allow reneg_bytes in to be overridden in v2.3 but enforce it to max 64MB in v2.4 or v2.5.

src/openvpn/ssl.c tls_get_cipher_name_pair (const char * cipher_name, size_t len) { · line 299

Quoted 3 lines

src/openvpn/ssl.c +3 -0

+    {
+      msg (M_WARN, "WARNING: cipher with small block size in use, "
+	   "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");

Depending on what we do with the >64MB ... this might need to be slightly reworded. If we allow users to override the 64MB default, it
should probably say: "... reneg-bytes is set to 64MB ...". I find
"reducing" slightly misleading.

Also if we decide not to cap at 64MB in this case, we should print another warning if --reneg-bytes is > 64MB. I know this will be somewhat annoying to users (imaginary #openvpn question: "I get so many warnings now, why!? I want to get rid of them"). My experience is that the more warnings we produce, the easier it is to convince users to fix the issue rather to ignore them.

Quoted 26 lines

src/openvpn/ssl.c +8 -0

+      *reneg_bytes = 64 * 1024 * 1024;
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1742,6 +1760,8 @@ tls_session_update_crypto_params(struct tls_session *session,
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
     }
+  tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			 &session->opt->renegotiate_bytes);
   ret = true;
 cleanup:
   CLEAR (*ks->key_src);

@@ -2126,6 +2146,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

src/openvpn/ssl.c key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi · line 2381

Quoted 6 lines

src/openvpn/ssl.c +2 -0

@@ -2354,6 +2376,8 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
 	}
 
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }

Regarding adding tls_limit_reneg_bytes() in key_method_2_{read,write}() ... How often will those functions be called? Is it once per renegotiation?

Signature · 3 lines

kind regards,

David Sommerseth

Steffan Karger

author

2016-10-27 14:42 UTC #6 lore raw

Hi,

Quoted 54 lines · David Sommerseth

Quoted 39 lines · Steffan Karger
Following the earlier warning about small block ciphers, now limit the --reneg-bytes value when using a cipher that susceptible to SWEET32-like attacks. The 64 MB value has been selected with the researchers who published the SWEET32 paper.
Note that this will not change a user-set (non-zero) --reneg-bytes value, to allow a user to override this behaviour if really needed.
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/crypto.c |  5 +++--
 src/openvpn/ssl.c    | 24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)
src/openvpn/crypto.c +3 -2
index 45689f2..b40fc7a 100644
@@ -836,8 +836,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 "
+	      "bit (%d bit).  This allows attacks like SWEET32.  Consider "
+	      "using a --cipher with a larger block size.",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }
src/openvpn/ssl.c +1 -0
index dfdc200..2c341a2 100644
@@ -283,6 +283,24 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**
src/openvpn/ssl.c tls_get_cipher_name_pair (const char * cipher_name, size_t len) { · line 296
Quoted 10 lines
src/openvpn/ssl.c +10 -0
+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (*reneg_bytes == 0 && cipher && (cipher_kt_block_size(cipher) < 128/8))
Should we also consider if *reneg_bytes > 64*1024*1024 and enforce maximum? This will not allow users to override the 64MB limit, though.

Quoted 2 lines

Perhaps we should allow reneg_bytes in to be overridden in v2.3 but enforce it to max 64MB in v2.4 or v2.5.

I believe we should provide 'sane defaults', but as far as I'm concerned users may align a gun with their feet if they really want to.

Quoted 9 lines

src/openvpn/ssl.c tls_get_cipher_name_pair (const char * cipher_name, size_t len) { · line 299
Quoted 3 lines
src/openvpn/ssl.c +3 -0
+    {
+      msg (M_WARN, "WARNING: cipher with small block size in use, "
+	   "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");
Depending on what we do with the >64MB ... this might need to be slightly reworded. If we allow users to override the 64MB default, it
should probably say: "... reneg-bytes is set to 64MB ...". I find
"reducing" slightly misleading.

Quoted 6 lines

Also if we decide not to cap at 64MB in this case, we should print another warning if --reneg-bytes is > 64MB. I know this will be somewhat annoying to users (imaginary #openvpn question: "I get so many warnings now, why!? I want to get rid of them"). My experience is that the more warnings we produce, the easier it is to convince users to fix the issue rather to ignore them.

We already print the generic warning from init_key_ctx() multiple times per connection setup (once for data channel encrypt, once for data channel decrypt). I think that should be annoying enough ;)

Quoted 28 lines

src/openvpn/ssl.c +8 -0

+      *reneg_bytes = 64 * 1024 * 1024;
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1742,6 +1760,8 @@ tls_session_update_crypto_params(struct tls_session *session,
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
     }
+  tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			 &session->opt->renegotiate_bytes);
   ret = true;
 cleanup:
   CLEAR (*ks->key_src);

@@ -2126,6 +2146,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2354,6 +2376,8 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
 	}

Quoted 7 lines

src/openvpn/ssl.c key_method_2_write (struct buffer *buf, struct tls_session *session) · line 2151
Quoted 4 lines
src/openvpn/ssl.c +2 -0
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
Regarding adding tls_limit_reneg_bytes() in key_method_2_{read,write}() ... How often will those functions be called? Is it once per renegotiation?

Indeed. This is needed in master because of negotiable crypto parameters. We could also do it just once on startup in release/2.3, but I thought it would be easier to keep 2.4 and 2.3 a bit similar in this respect.

-Steffan

Steffan Karger

author

2016-10-28 11:57 UTC #7 lore raw

PATCH v2 1/1 Limit --reneg-bytes to 64MB when using small block ciphers

Note that this will not change a user-set --reneg-bytes value, to allow a user to align a gun with his feet^w^w^w^w^w^w override this behaviour if really needed.

v2: obey user-set --reneg-bytes 0 to revert to old behaviour, use more firm
language in warning message, and add URL to man page.

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

---
 doc/openvpn.8         |  1 +
 src/openvpn/crypto.c  |  5 +++--
 src/openvpn/options.c |  1 +
 src/openvpn/ssl.c     | 29 ++++++++++++++++++++++++++++-
 4 files changed, 33 insertions(+), 3 deletions(-)

doc/openvpn.8 +1 -0

index 2e58f33..0ac8184 100644
@@ -4101,6 +4101,7 @@ an abbreviation for Blowfish in Cipher Block Chaining mode.
 
 Using BF-CBC is no longer recommended, because of it's 64-bit block size.  This
 small block size allows attacks based on collisions, as demonstrated by SWEET32.
+See https://community.openvpn.net/openvpn/wiki/SWEET32 for details.
 
 To see other ciphers that are available with OpenVPN, use the
 .B \-\-show\-ciphers

src/openvpn/crypto.c +3 -2

index 45689f2..f600070 100644
@@ -836,8 +836,9 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
 	  cipher_kt_iv_size(kt->cipher));
       if (cipher_kt_block_size(kt->cipher) < 128/8)
 	{
-	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
-	      "(%d bit).  Consider using a --cipher with a larger block size.",
+	  msg (M_WARN, "WARNING: INSECURE cipher with block size less than 128"
+	      " bit (%d bit).  This allows attacks like SWEET32.  Mitigate by "
+	      "using a --cipher with a larger block size (e.g. AES-256-CBC).",
 	      cipher_kt_block_size(kt->cipher)*8);
 	}
     }

src/openvpn/options.c +1 -0

index 281ef0b..996c26e 100644
@@ -852,6 +852,7 @@ init_options (struct options *o, const bool init_gc)
 #endif
   o->key_method = 2;
   o->tls_timeout = 2;
+  o->renegotiate_bytes = -1;
   o->renegotiate_seconds = 3600;
   o->handshake_window = 60;
   o->transition_window = 3600;

src/openvpn/ssl.c +28 -1

index dfdc200..33122a3 100644
@@ -283,6 +283,27 @@ tls_get_cipher_name_pair (const char * cipher_name, size_t len) {
   return NULL;
 }
 
+/**
+ * Limit the reneg_bytes value when using a small-block (<128 bytes) cipher.
+ *
+ * @param cipher	The current cipher (may be NULL).
+ * @param reneg_bytes	Pointer to the current reneg_bytes, updated if needed.
+ * 			May *not* be NULL.
+ */
+static void
+tls_limit_reneg_bytes (const cipher_kt_t *cipher, int *reneg_bytes)
+{
+  if (cipher && (cipher_kt_block_size(cipher) < 128/8))
+    {
+      if (*reneg_bytes == -1) /* Not user-specified */
+	{
+	  msg (M_WARN, "WARNING: cipher with small block size in use, "
+	       "reducing reneg-bytes to 64MB to mitigate SWEET32 attacks.");
+	  *reneg_bytes = 64 * 1024 * 1024;
+	}
+    }
+}
+
 /*
  * Max number of bytes we will add
  * for data structures common to both

@@ -1742,6 +1763,8 @@ tls_session_update_crypto_params(struct tls_session *session,
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
     }
+  tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			 &session->opt->renegotiate_bytes);
   ret = true;
 cleanup:
   CLEAR (*ks->key_src);

@@ -2126,6 +2149,8 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
 	}
 		      
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   return true;

@@ -2354,6 +2379,8 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
 	}
 
       CLEAR (*ks->key_src);
+      tls_limit_reneg_bytes (session->opt->key_type.cipher,
+			     &session->opt->renegotiate_bytes);
     }
 
   gc_free (&gc);

@@ -2410,7 +2437,7 @@ tls_process (struct tls_multi *multi,
   if (ks->state >= S_ACTIVE &&
       ((session->opt->renegotiate_seconds
 	&& now >= ks->established + session->opt->renegotiate_seconds)
-       || (session->opt->renegotiate_bytes
+       || (session->opt->renegotiate_bytes > 0
 	   && ks->n_bytes >= session->opt->renegotiate_bytes)
        || (session->opt->renegotiate_packets
 	   && ks->n_packets >= session->opt->renegotiate_packets)

Signature · 1 line

2.7.4

David Sommerseth

2016-10-28 13:16 UTC #8 lore raw

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

ACK.

Your patch has been applied to the dev/pre-push-hook branch

commit 752caece99a61e516386f94823e82ddf13fcbcab

Author: Steffan Karger
Date:   Fri Oct 28 13:57:01 2016 +0200

     Limit --reneg-bytes to 64MB when using small block ciphers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg12798.html
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iQIcBAEBAgAGBQJYE0+TAAoJEIbPlEyWcf3yPZ4P/08Gs7c8NXaER8G6dMY6y95g ZHCwOSjBJaQpdHVZE4gnSq/pip+49fQjf4gz5g/xb+NgxSQqtzfYYRsmc2y9cOJJ xj5jRzrJnrTgWEFumVJ3/4byzz+2WkkWVfeR+JUXyenwor4biyUbFyVbTVJcGFfO GZRY+ZPoNjBMlbWyIAIit0O83XsdDnVd6aur1d1WK9HQsUTaYkkv4q5WS42vIWCn CCrq0EN3V/vi6OnRQs2zQhYiV1/UvlojOuobLr9w5koeduKVtPsq0+7JM6a1+sUL GuofhGWDlNtisoYO22g2RSluc//pwB1Ur3KyaXTvq/3o2ViwJI14ohK1sQWuFOgv PZzH/eZ0lc4UN80kNskJ26e9QLD+/LQtSZMwlkNS1iL3oL1WfQRrn+yAzzr551GQ HyK9q1zw7T5FPT0j3qnXfkn5HiwBv0SWjWipD4ot1OW+9fK/0StaI+cIHitWrPNl FzJauLfAt9wOSFAaPePRlxz5fIRVWZ3I/1hK93qF7JhQMfCIkncyvn7UKAPgYAUc SBGndYtLNkbBgfv/9F+1k8cHH8wcRNAPdexVdqrbM72I0K155Sl5eNoY9Gn1+A3s JzxGGoEq+nF223fXVfQzNZ5jzKUx4NgWfe8Qbm3qJ6Ul9ZCYacan2to6kwR4YT1p gngtP5O9S54N8L/FiWKJ
=z8RU
-----END PGP SIGNATURE-----

David Sommerseth

2016-10-28 13:31 UTC #9 lore raw

Quoted 3 lines · David Sommerseth

ACK.
Your patch has been applied to the dev/pre-push-hook branch

This was pushed to the master branch.

Quoted 12 lines

commit 752caece99a61e516386f94823e82ddf13fcbcab

Author: Steffan Karger
Date:   Fri Oct 28 13:57:01 2016 +0200

     Limit --reneg-bytes to 64MB when using small block ciphers

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: David Sommerseth <[email protected]...>
     Message-Id: <[email protected]......>
     URL: https://www.mail-archive.com/[email protected]/msg12798.html
     Signed-off-by: David Sommerseth <[email protected]...>

Signature · 3 lines

kind regards,

David Sommerseth