Browse code

Fix '--cipher none --cipher' crash

As reported in trac #699, OpenVPN crashes when an "--cipher none" option
is followed by "--cipher" (without arguments). Fix this by removing the
redudant ciphername_defined and authname_defined members of struct options,
and remove support to specify --cipher or --auth without an argument. That
not only fixes the issue, but also cleans up the code a bit.

v2: don't print a deprecating warning (we'll do that in the 2.3 branch),
but just rip out support for --cipher and --auth without an argument.

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

Steffan Karger authored on 2016/07/26 22:55:38
Showing 6 changed files
... ...
@@ -713,7 +713,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
713 713
 void
714 714
 crypto_adjust_frame_parameters(struct frame *frame,
715 715
 			       const struct key_type* kt,
716
-			       bool cipher_defined,
717 716
 			       bool use_iv,
718 717
 			       bool packet_id,
719 718
 			       bool packet_id_long_form)
... ...
@@ -723,7 +722,7 @@ crypto_adjust_frame_parameters(struct frame *frame,
723 723
   if (packet_id)
724 724
     crypto_overhead += packet_id_size (packet_id_long_form);
725 725
 
726
-  if (cipher_defined)
726
+  if (kt->cipher)
727 727
     {
728 728
       if (use_iv)
729 729
 	crypto_overhead += cipher_kt_iv_size (kt->cipher);
... ...
@@ -756,14 +755,12 @@ crypto_max_overhead(void)
756 756
  */
757 757
 void
758 758
 init_key_type (struct key_type *kt, const char *ciphername,
759
-	       bool ciphername_defined, const char *authname,
760
-	       bool authname_defined, int keysize,
761
-	       bool tls_mode, bool warn)
759
+	       const char *authname, int keysize, bool tls_mode, bool warn)
762 760
 {
763 761
   bool aead_cipher = false;
764 762
 
765 763
   CLEAR (*kt);
766
-  if (ciphername && ciphername_defined)
764
+  if (ciphername)
767 765
     {
768 766
       kt->cipher = cipher_kt_get (translate_cipher_name_from_openvpn(ciphername));
769 767
       kt->cipher_length = cipher_kt_key_size (kt->cipher);
... ...
@@ -788,7 +785,7 @@ init_key_type (struct key_type *kt, const char *ciphername,
788 788
       if (warn)
789 789
 	msg (M_WARN, "******* WARNING *******: null cipher specified, no encryption will be used");
790 790
     }
791
-  if (authname && authname_defined)
791
+  if (authname)
792 792
     {
793 793
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
794 794
 	kt->digest = md_kt_get (authname);
... ...
@@ -296,9 +296,20 @@ bool write_key (const struct key *key, const struct key_type *kt,
296 296
 
297 297
 int read_key (struct key *key, const struct key_type *kt, struct buffer *buf);
298 298
 
299
+/**
300
+ * Initialize a key_type structure with.
301
+ *
302
+ * @param kt          The struct key_type to initialize
303
+ * @param ciphername  The name of the cipher to use
304
+ * @param authname    The name of the HMAC digest to use
305
+ * @param keysize     The length of the cipher key to use, in bytes.  Only valid
306
+ *                    for ciphers that support variable length keys.
307
+ * @param tls_mode    Specifies wether we are running in TLS mode, which allows
308
+ *                    more ciphers than static key mode.
309
+ * @param warn        Print warnings when null cipher / auth is used.
310
+ */
299 311
 void init_key_type (struct key_type *kt, const char *ciphername,
300
-    bool ciphername_defined, const char *authname, bool authname_defined,
301
-    int keysize, bool tls_mode, bool warn);
312
+    const char *authname, int keysize, bool tls_mode, bool warn);
302 313
 
303 314
 /*
304 315
  * Key context functions
... ...
@@ -389,7 +400,6 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
389 389
 /** Calculate crypto overhead and adjust frame to account for that */
390 390
 void crypto_adjust_frame_parameters(struct frame *frame,
391 391
 				    const struct key_type* kt,
392
-				    bool cipher_defined,
393 392
 				    bool use_iv,
394 393
 				    bool packet_id,
395 394
 				    bool packet_id_long_form);
... ...
@@ -2156,10 +2156,8 @@ do_init_crypto_static (struct context *c, const unsigned int flags)
2156 2156
       struct key_direction_state kds;
2157 2157
 
2158 2158
       /* Get cipher & hash algorithms */
2159
-      init_key_type (&c->c1.ks.key_type, options->ciphername,
2160
-		     options->ciphername_defined, options->authname,
2161
-		     options->authname_defined, options->keysize,
2162
-		     options->test_crypto, true);
2159
+      init_key_type (&c->c1.ks.key_type, options->ciphername, options->authname,
2160
+		     options->keysize, options->test_crypto, true);
2163 2161
 
2164 2162
       /* Read cipher and hmac keys from shared secret file */
2165 2163
       {
... ...
@@ -2201,7 +2199,6 @@ do_init_crypto_static (struct context *c, const unsigned int flags)
2201 2201
   /* Compute MTU parameters */
2202 2202
   crypto_adjust_frame_parameters (&c->c2.frame,
2203 2203
 				  &c->c1.ks.key_type,
2204
-				  options->ciphername_defined,
2205 2204
 				  options->use_iv, options->replay, true);
2206 2205
 
2207 2206
   /* Sanity check on IV, sequence number, and cipher mode options */
... ...
@@ -2249,9 +2246,8 @@ do_init_crypto_tls_c1 (struct context *c)
2249 2249
 	}
2250 2250
 
2251 2251
       /* Get cipher & hash algorithms */
2252
-      init_key_type (&c->c1.ks.key_type, options->ciphername,
2253
-		     options->ciphername_defined, options->authname,
2254
-		     options->authname_defined, options->keysize, true, true);
2252
+      init_key_type (&c->c1.ks.key_type, options->ciphername, options->authname,
2253
+		     options->keysize, true, true);
2255 2254
 
2256 2255
       /* Initialize PRNG with config-specified digest */
2257 2256
       prng_init (options->prng_hash, options->prng_nonce_secret_len);
... ...
@@ -2270,7 +2266,7 @@ do_init_crypto_tls_c1 (struct context *c)
2270 2270
 
2271 2271
 	  /* Initialize key_type for tls-auth with auth only */
2272 2272
 	  CLEAR (c->c1.ks.tls_auth_key_type);
2273
-	  if (options->authname && options->authname_defined)
2273
+	  if (options->authname)
2274 2274
 	    {
2275 2275
 	      c->c1.ks.tls_auth_key_type.digest = md_kt_get (options->authname);
2276 2276
 	      c->c1.ks.tls_auth_key_type.hmac_length =
... ...
@@ -2339,8 +2335,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
2339 2339
   else
2340 2340
     {
2341 2341
       crypto_adjust_frame_parameters(&c->c2.frame, &c->c1.ks.key_type,
2342
-	  options->ciphername_defined, options->use_iv, options->replay,
2343
-	  packet_id_long_form);
2342
+	  options->use_iv, options->replay, packet_id_long_form);
2344 2343
     }
2345 2344
   tls_adjust_frame_parameters (&c->c2.frame);
2346 2345
 
... ...
@@ -2468,7 +2463,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
2468 2468
       to.tls_auth.flags |= CO_PACKET_ID_LONG_FORM;
2469 2469
       crypto_adjust_frame_parameters (&to.frame,
2470 2470
 				      &c->c1.ks.tls_auth_key_type,
2471
-				      false, false, true, true);
2471
+				      false, true, true);
2472 2472
     }
2473 2473
 
2474 2474
   /* If we are running over TCP, allow for
... ...
@@ -830,7 +830,6 @@ init_options (struct options *o, const bool init_gc)
830 830
 #endif
831 831
 #ifdef ENABLE_CRYPTO
832 832
   o->ciphername = "BF-CBC";
833
-  o->ciphername_defined = true;
834 833
 #ifdef HAVE_AEAD_CIPHER_MODES /* IV_NCP=2 requires GCM support */
835 834
   o->ncp_enabled = true;
836 835
 #else
... ...
@@ -838,7 +837,6 @@ init_options (struct options *o, const bool init_gc)
838 838
 #endif
839 839
   o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
840 840
   o->authname = "SHA1";
841
-  o->authname_defined = true;
842 841
   o->prng_hash = "SHA1";
843 842
   o->prng_nonce_secret_len = 16;
844 843
   o->replay = true;
... ...
@@ -1618,9 +1616,7 @@ show_settings (const struct options *o)
1618 1618
 #ifdef ENABLE_CRYPTO
1619 1619
   SHOW_STR (shared_secret_file);
1620 1620
   SHOW_INT (key_direction);
1621
-  SHOW_BOOL (ciphername_defined);
1622 1621
   SHOW_STR (ciphername);
1623
-  SHOW_BOOL (authname_defined);
1624 1622
   SHOW_STR (authname);
1625 1623
   SHOW_STR (prng_hash);
1626 1624
   SHOW_INT (prng_nonce_secret_len);
... ...
@@ -2989,12 +2985,11 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
2989 2989
     {
2990 2990
       struct frame fake_frame = *frame;
2991 2991
       struct key_type fake_kt;
2992
-      init_key_type (&fake_kt, o->ciphername, o->ciphername_defined,
2993
-	  o->authname, o->authname_defined, o->keysize, true, false);
2992
+      init_key_type (&fake_kt, o->ciphername, o->authname, o->keysize, true,
2993
+	  false);
2994 2994
       frame_add_to_extra_frame (&fake_frame, -(crypto_max_overhead()));
2995
-      crypto_adjust_frame_parameters (&fake_frame, &fake_kt,
2996
-	  o->ciphername_defined, o->use_iv, o->replay,
2997
-	  cipher_kt_mode_ofb_cfb (fake_kt.cipher));
2995
+      crypto_adjust_frame_parameters (&fake_frame, &fake_kt, o->use_iv,
2996
+	  o->replay, cipher_kt_mode_ofb_cfb (fake_kt.cipher));
2998 2997
       frame_finalize(&fake_frame, o->ce.link_mtu_defined, o->ce.link_mtu,
2999 2998
             o->ce.tun_mtu_defined, o->ce.tun_mtu);
3000 2999
       msg (D_MTU_DEBUG, "%s: link-mtu %zu -> %d", __func__, link_mtu,
... ...
@@ -3146,9 +3141,8 @@ options_string (const struct options *o,
3146 3146
 		+ (TLS_SERVER == true)
3147 3147
 		<= 1);
3148 3148
 
3149
-	init_key_type (&kt, o->ciphername, o->ciphername_defined,
3150
-		       o->authname, o->authname_defined,
3151
-		       o->keysize, true, false);
3149
+	init_key_type (&kt, o->ciphername, o->authname, o->keysize, true,
3150
+	    false);
3152 3151
 
3153 3152
 	buf_printf (&out, ",cipher %s",
3154 3153
 	    translate_cipher_name_to_openvpn(cipher_kt_name (kt.cipher)));
... ...
@@ -6646,35 +6640,21 @@ add_option (struct options *options,
6646 6646
   else if (streq (p[0], "auth") && p[1] && !p[2])
6647 6647
     {
6648 6648
       VERIFY_PERMISSION (OPT_P_GENERAL);
6649
-      options->authname_defined = true;
6650 6649
       options->authname = p[1];
6651 6650
       if (streq (options->authname, "none"))
6652 6651
 	{
6653
-	  options->authname_defined = false;
6654 6652
 	  options->authname = NULL;
6655 6653
 	}
6656 6654
     }
6657
-  else if (streq (p[0], "auth") && !p[1])
6658
-    {
6659
-      VERIFY_PERMISSION (OPT_P_GENERAL);
6660
-      options->authname_defined = true;
6661
-    }
6662 6655
   else if (streq (p[0], "cipher") && p[1] && !p[2])
6663 6656
     {
6664 6657
       VERIFY_PERMISSION (OPT_P_NCP);
6665
-      options->ciphername_defined = true;
6666 6658
       options->ciphername = p[1];
6667 6659
       if (streq (options->ciphername, "none"))
6668 6660
 	{
6669
-	  options->ciphername_defined = false;
6670 6661
 	  options->ciphername = NULL;
6671 6662
 	}
6672 6663
     }
6673
-  else if (streq (p[0], "cipher") && !p[1])
6674
-    {
6675
-      VERIFY_PERMISSION (OPT_P_GENERAL);
6676
-      options->ciphername_defined = true;
6677
-    }
6678 6664
   else if (streq (p[0], "ncp-ciphers") && p[1] && !p[2])
6679 6665
     {
6680 6666
       VERIFY_PERMISSION (OPT_P_GENERAL|OPT_P_INSTANCE);
... ...
@@ -469,11 +469,9 @@ struct options
469 469
   const char *shared_secret_file;
470 470
   const char *shared_secret_file_inline;
471 471
   int key_direction;
472
-  bool ciphername_defined;
473 472
   const char *ciphername;
474 473
   bool ncp_enabled;
475 474
   const char *ncp_ciphers;
476
-  bool authname_defined;
477 475
   const char *authname;
478 476
   int keysize;
479 477
   const char *prng_hash;
... ...
@@ -1678,8 +1678,7 @@ tls_session_update_crypto_params(struct tls_session *session,
1678 1678
     }
1679 1679
 
1680 1680
   init_key_type (&session->opt->key_type, options->ciphername,
1681
-    options->ciphername_defined, options->authname, options->authname_defined,
1682
-    options->keysize, true, true);
1681
+      options->authname, options->keysize, true, true);
1683 1682
 
1684 1683
   bool packet_id_long_form = cipher_kt_mode_ofb_cfb (session->opt->key_type.cipher);
1685 1684
   session->opt->crypto_flags_and &= ~(CO_PACKET_ID_LONG_FORM);
... ...
@@ -1689,8 +1688,7 @@ tls_session_update_crypto_params(struct tls_session *session,
1689 1689
   /* Update frame parameters: undo worst-case overhead, add actual overhead */
1690 1690
   frame_add_to_extra_frame (frame, -(crypto_max_overhead()));
1691 1691
   crypto_adjust_frame_parameters (frame, &session->opt->key_type,
1692
-      options->ciphername_defined, options->use_iv, options->replay,
1693
-      packet_id_long_form);
1692
+      options->use_iv, options->replay, packet_id_long_form);
1694 1693
   frame_finalize(frame, options->ce.link_mtu_defined, options->ce.link_mtu,
1695 1694
       options->ce.tun_mtu_defined, options->ce.tun_mtu);
1696 1695
   frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");