Browse code

Discourage using 64-bit block ciphers

As discussed with the development team, we should start moving away from
ciphers with a small block size. For OpenVPN in particular this means
moving away from 64-bit block ciphers, towards 128-bit block ciphers.
This patch makes a start with that by moving ciphers with a block
size < 128 bits to the bottom of the --show-ciphers output, and printing
a warning in the connection phase if such a cipher is used.

While touching this function, improve the output of --show-ciphers by
ordering the output alphabetically, and changing the output format
slightly.

[DS: Fixed C89 issues in patch, moving 'int nid' and 'size_t i' declaration
to begining of function instead of in the for-loops. This is also
required to not break building on stricter compiler setups where C99
must be enabled explicitly ]

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: Arne Schwabe <arne@rfc2549.org>
Message-Id: <1471358742-8773-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00029.html
CVE: 2016-6329
Signed-off-by: David Sommerseth <davids@openvpn.net>

Steffan Karger authored on 2016/08/16 23:45:42
Showing 4 changed files
... ...
@@ -825,9 +825,14 @@ init_key_ctx (struct key_ctx *ctx, struct key *key,
825 825
       dmsg (D_SHOW_KEYS, "%s: CIPHER KEY: %s", prefix,
826 826
           format_hex (key->cipher, kt->cipher_length, 0, &gc));
827 827
       dmsg (D_CRYPTO_DEBUG, "%s: CIPHER block_size=%d iv_size=%d",
828
-          prefix,
829
-          cipher_kt_block_size(kt->cipher),
830
-          cipher_kt_iv_size(kt->cipher));
828
+          prefix, cipher_kt_block_size(kt->cipher),
829
+	  cipher_kt_iv_size(kt->cipher));
830
+      if (cipher_kt_block_size(kt->cipher) < 128/8)
831
+	{
832
+	  msg (M_WARN, "WARNING: this cipher's block size is less than 128 bit "
833
+	      "(%d bit).  Consider using a --cipher with a larger block size.",
834
+	      cipher_kt_block_size(kt->cipher)*8);
835
+	}
831 836
     }
832 837
   if (kt->digest && kt->hmac_length > 0)
833 838
     {
... ...
@@ -132,6 +132,25 @@ const cipher_name_pair cipher_name_translation_table[] = {
132 132
 const size_t cipher_name_translation_table_count =
133 133
     sizeof (cipher_name_translation_table) / sizeof (*cipher_name_translation_table);
134 134
 
135
+static void print_cipher(const cipher_kt_t *info)
136
+{
137
+  if (info && (cipher_kt_mode_cbc(info)
138
+#ifdef HAVE_AEAD_CIPHER_MODES
139
+      || cipher_kt_mode_aead(info)
140
+#endif
141
+  ))
142
+    {
143
+      const char *ssl_only = cipher_kt_mode_cbc(info) ?
144
+	  "" : ", TLS client/server mode only";
145
+      const char *var_key_size = info->flags & MBEDTLS_CIPHER_VARIABLE_KEY_LEN ?
146
+	  " by default" : "";
147
+
148
+      printf ("%s  (%d bit key%s, %d bit block%s)\n",
149
+	  cipher_kt_name(info), cipher_kt_key_size(info) * 8, var_key_size,
150
+	  cipher_kt_block_size(info) * 8, ssl_only);
151
+    }
152
+}
153
+
135 154
 void
136 155
 show_available_ciphers ()
137 156
 {
... ...
@@ -147,20 +166,23 @@ show_available_ciphers ()
147 147
   while (*ciphers != 0)
148 148
     {
149 149
       const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
150
-
151
-      if (info && (cipher_kt_mode_cbc(info)
152
-#ifdef HAVE_AEAD_CIPHER_MODES
153
-          || cipher_kt_mode_aead(info)
154
-#endif
155
-          ))
150
+      if (info && cipher_kt_block_size(info) >= 128/8)
156 151
 	{
157
-	  const char *ssl_only = cipher_kt_mode_cbc(info) ?
158
-	      "" : " (TLS client/server mode)";
159
-
160
-	  printf ("%s %d bit default key%s\n",
161
-	      cipher_kt_name(info), cipher_kt_key_size(info) * 8, ssl_only);
152
+	  print_cipher(info);
162 153
 	}
154
+      ciphers++;
155
+    }
163 156
 
157
+  printf ("\nThe following ciphers have a block size of less than 128 bits, \n"
158
+	  "and are therefore deprecated.  Do not use unless you have to.\n\n");
159
+  ciphers = mbedtls_cipher_list();
160
+  while (*ciphers != 0)
161
+    {
162
+      const cipher_kt_t *info = mbedtls_cipher_info_from_type(*ciphers);
163
+      if (info && cipher_kt_block_size(info) < 128/8)
164
+	{
165
+	  print_cipher(info);
166
+	}
164 167
       ciphers++;
165 168
     }
166 169
   printf ("\n");
... ...
@@ -249,11 +249,45 @@ const size_t cipher_name_translation_table_count =
249 249
     sizeof (cipher_name_translation_table) / sizeof (*cipher_name_translation_table);
250 250
 
251 251
 
252
+static int
253
+cipher_name_cmp(const void *a, const void *b)
254
+{
255
+  const EVP_CIPHER * const *cipher_a = a;
256
+  const EVP_CIPHER * const *cipher_b = b;
257
+
258
+  const char *cipher_name_a =
259
+      translate_cipher_name_to_openvpn(EVP_CIPHER_name(*cipher_a));
260
+  const char *cipher_name_b =
261
+      translate_cipher_name_to_openvpn(EVP_CIPHER_name(*cipher_b));
262
+
263
+  return strcmp(cipher_name_a, cipher_name_b);
264
+}
265
+
266
+static void
267
+print_cipher(const EVP_CIPHER *cipher)
268
+{
269
+  const char *var_key_size =
270
+	(EVP_CIPHER_flags (cipher) & EVP_CIPH_VARIABLE_LENGTH) ?
271
+	     " by default" : "";
272
+  const char *ssl_only = cipher_kt_mode_cbc(cipher) ?
273
+	"" : ", TLS client/server mode only";
274
+
275
+  printf ("%s  (%d bit key%s, %d bit block%s)\n",
276
+	translate_cipher_name_to_openvpn (EVP_CIPHER_name (cipher)),
277
+	EVP_CIPHER_key_length (cipher) * 8, var_key_size,
278
+	cipher_kt_block_size (cipher) * 8, ssl_only);
279
+}
280
+
252 281
 void
253 282
 show_available_ciphers ()
254 283
 {
255 284
   int nid;
285
+  size_t i;
256 286
 
287
+  /* If we ever exceed this, we must be more selective */
288
+  const size_t cipher_list_len = 1000;
289
+  const EVP_CIPHER *cipher_list[cipher_list_len];
290
+  size_t num_ciphers = 0;
257 291
 #ifndef ENABLE_SMALL
258 292
   printf ("The following ciphers and cipher modes are available for use\n"
259 293
 	  "with " PACKAGE_NAME ".  Each cipher shown below may be use as a\n"
... ...
@@ -263,32 +297,40 @@ show_available_ciphers ()
263 263
 	  "In static key mode only CBC mode is allowed.\n\n");
264 264
 #endif
265 265
 
266
-  for (nid = 0; nid < 10000; ++nid)	/* is there a better way to get the size of the nid list? */
266
+  for (nid = 0; nid < 10000; ++nid)
267 267
     {
268
-      const EVP_CIPHER *cipher = EVP_get_cipherbynid (nid);
269
-      if (cipher)
270
-	{
271
-	  if (cipher_kt_mode_cbc(cipher)
268
+      const EVP_CIPHER *cipher = EVP_get_cipherbynid(nid);
269
+      if (cipher && (cipher_kt_mode_cbc(cipher)
272 270
 #ifdef ENABLE_OFB_CFB_MODE
273 271
 	      || cipher_kt_mode_ofb_cfb(cipher)
274 272
 #endif
275 273
 #ifdef HAVE_AEAD_CIPHER_MODES
276 274
 	      || cipher_kt_mode_aead(cipher)
277 275
 #endif
278
-	      )
279
-	    {
280
-	      const char *var_key_size =
281
-		  (EVP_CIPHER_flags (cipher) & EVP_CIPH_VARIABLE_LENGTH) ?
282
-		       "variable" : "fixed";
283
-	      const char *ssl_only = cipher_kt_mode_cbc(cipher) ?
284
-		  "" : " (TLS client/server mode)";
285
-
286
-	      printf ("%s %d bit default key (%s)%s\n",
287
-		  translate_cipher_name_to_openvpn(OBJ_nid2sn (nid)),
288
-		  EVP_CIPHER_key_length (cipher) * 8, var_key_size, ssl_only);
289
-	    }
276
+          ))
277
+	{
278
+	  cipher_list[num_ciphers++] = cipher;
279
+	}
280
+      if (num_ciphers == cipher_list_len)
281
+	{
282
+	  msg (M_WARN, "WARNING: Too many ciphers, not showing all");
283
+	  break;
290 284
 	}
291 285
     }
286
+
287
+  qsort (cipher_list, num_ciphers, sizeof(*cipher_list), cipher_name_cmp);
288
+
289
+  for (i = 0; i < num_ciphers; i++) {
290
+      if (cipher_kt_block_size(cipher_list[i]) >= 128/8)
291
+	print_cipher(cipher_list[i]);
292
+  }
293
+
294
+  printf ("\nThe following ciphers have a block size of less than 128 bits, \n"
295
+	  "and are therefore deprecated.  Do not use unless you have to.\n\n");
296
+  for (i = 0; i < num_ciphers; i++) {
297
+      if (cipher_kt_block_size(cipher_list[i]) < 128/8)
298
+	print_cipher(cipher_list[i]);
299
+  }
292 300
   printf ("\n");
293 301
 }
294 302
 
... ...
@@ -494,9 +536,37 @@ cipher_kt_iv_size (const EVP_CIPHER *cipher_kt)
494 494
 }
495 495
 
496 496
 int
497
-cipher_kt_block_size (const EVP_CIPHER *cipher_kt)
498
-{
499
-  return EVP_CIPHER_block_size (cipher_kt);
497
+cipher_kt_block_size (const EVP_CIPHER *cipher) {
498
+  /* OpenSSL reports OFB/CFB/GCM cipher block sizes as '1 byte'.  To work
499
+   * around that, try to replace the mode with 'CBC' and return the block size
500
+   * reported for that cipher, if possible.  If that doesn't work, just return
501
+   * the value reported by OpenSSL.
502
+   */
503
+  char *name = NULL;
504
+  char *mode_str = NULL;
505
+  const char *orig_name = NULL;
506
+  const EVP_CIPHER *cbc_cipher = NULL;
507
+
508
+  int block_size = EVP_CIPHER_block_size(cipher);
509
+
510
+  orig_name = cipher_kt_name(cipher);
511
+  if (!orig_name)
512
+    goto cleanup;
513
+
514
+  name = string_alloc(translate_cipher_name_to_openvpn(orig_name), NULL);
515
+  mode_str = strrchr (name, '-');
516
+  if (!mode_str || strlen(mode_str) < 4)
517
+    goto cleanup;
518
+
519
+  strcpy (mode_str, "-CBC");
520
+
521
+  cbc_cipher = EVP_get_cipherbyname(translate_cipher_name_from_openvpn(name));
522
+  if (cbc_cipher)
523
+    block_size = EVP_CIPHER_block_size(cbc_cipher);
524
+
525
+cleanup:
526
+  free (name);
527
+  return block_size;
500 528
 }
501 529
 
502 530
 int
... ...
@@ -26,7 +26,7 @@ trap "rm -f key.$$ log.$$ ; exit 1" 0 3
26 26
 
27 27
 # Get list of supported ciphers from openvpn --show-ciphers output
28 28
 CIPHERS=$(${top_builddir}/src/openvpn/openvpn --show-ciphers | \
29
-            sed -e '1,/^$/d' -e s'/ .*//' -e '/^\s*$/d' | sort)
29
+            sed -e '/The following/,/^$/d' -e s'/ .*//' -e '/^\s*$/d')
30 30
 
31 31
 # SK, 2014-06-04: currently the DES-EDE3-CFB1 implementation of OpenSSL is
32 32
 # broken (see http://rt.openssl.org/Ticket/Display.html?id=2867), so exclude