From: Steffan Karger Date: Tue, 26 Jul 2016 13:55:38 +0000 (+0200) Subject: Fix '--cipher none --cipher' crash X-Git-Tag: v2.4_alpha1~50 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=dea8917a032e2303b39cce23e60f2348516d5a12;p=thirdparty%2Fopenvpn.git 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 Acked-by: Gert Doering 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 --- diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c index f9cf62aea..d94896ce6 100644 --- a/src/openvpn/crypto.c +++ b/src/openvpn/crypto.c @@ -713,7 +713,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work, void crypto_adjust_frame_parameters(struct frame *frame, const struct key_type* kt, - bool cipher_defined, bool use_iv, bool packet_id, bool packet_id_long_form) @@ -723,7 +722,7 @@ crypto_adjust_frame_parameters(struct frame *frame, if (packet_id) crypto_overhead += packet_id_size (packet_id_long_form); - if (cipher_defined) + if (kt->cipher) { if (use_iv) crypto_overhead += cipher_kt_iv_size (kt->cipher); @@ -756,14 +755,12 @@ crypto_max_overhead(void) */ void init_key_type (struct key_type *kt, const char *ciphername, - bool ciphername_defined, const char *authname, - bool authname_defined, int keysize, - bool tls_mode, bool warn) + const char *authname, int keysize, bool tls_mode, bool warn) { bool aead_cipher = false; CLEAR (*kt); - if (ciphername && ciphername_defined) + if (ciphername) { kt->cipher = cipher_kt_get (translate_cipher_name_from_openvpn(ciphername)); kt->cipher_length = cipher_kt_key_size (kt->cipher); @@ -788,7 +785,7 @@ init_key_type (struct key_type *kt, const char *ciphername, if (warn) msg (M_WARN, "******* WARNING *******: null cipher specified, no encryption will be used"); } - if (authname && authname_defined) + if (authname) { if (!aead_cipher) { /* Ignore auth for AEAD ciphers */ kt->digest = md_kt_get (authname); diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h index de433ae8e..3b6bb9805 100644 --- a/src/openvpn/crypto.h +++ b/src/openvpn/crypto.h @@ -296,9 +296,20 @@ bool write_key (const struct key *key, const struct key_type *kt, int read_key (struct key *key, const struct key_type *kt, struct buffer *buf); +/** + * Initialize a key_type structure with. + * + * @param kt The struct key_type to initialize + * @param ciphername The name of the cipher to use + * @param authname The name of the HMAC digest to use + * @param keysize The length of the cipher key to use, in bytes. Only valid + * for ciphers that support variable length keys. + * @param tls_mode Specifies wether we are running in TLS mode, which allows + * more ciphers than static key mode. + * @param warn Print warnings when null cipher / auth is used. + */ void init_key_type (struct key_type *kt, const char *ciphername, - bool ciphername_defined, const char *authname, bool authname_defined, - int keysize, bool tls_mode, bool warn); + const char *authname, int keysize, bool tls_mode, bool warn); /* * Key context functions @@ -389,7 +400,6 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work, /** Calculate crypto overhead and adjust frame to account for that */ void crypto_adjust_frame_parameters(struct frame *frame, const struct key_type* kt, - bool cipher_defined, bool use_iv, bool packet_id, bool packet_id_long_form); diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 5a47b923e..87a0e32b2 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2156,10 +2156,8 @@ do_init_crypto_static (struct context *c, const unsigned int flags) struct key_direction_state kds; /* Get cipher & hash algorithms */ - init_key_type (&c->c1.ks.key_type, options->ciphername, - options->ciphername_defined, options->authname, - options->authname_defined, options->keysize, - options->test_crypto, true); + init_key_type (&c->c1.ks.key_type, options->ciphername, options->authname, + options->keysize, options->test_crypto, true); /* Read cipher and hmac keys from shared secret file */ { @@ -2201,7 +2199,6 @@ do_init_crypto_static (struct context *c, const unsigned int flags) /* Compute MTU parameters */ crypto_adjust_frame_parameters (&c->c2.frame, &c->c1.ks.key_type, - options->ciphername_defined, options->use_iv, options->replay, true); /* Sanity check on IV, sequence number, and cipher mode options */ @@ -2249,9 +2246,8 @@ do_init_crypto_tls_c1 (struct context *c) } /* Get cipher & hash algorithms */ - init_key_type (&c->c1.ks.key_type, options->ciphername, - options->ciphername_defined, options->authname, - options->authname_defined, options->keysize, true, true); + init_key_type (&c->c1.ks.key_type, options->ciphername, options->authname, + options->keysize, true, true); /* Initialize PRNG with config-specified digest */ prng_init (options->prng_hash, options->prng_nonce_secret_len); @@ -2270,7 +2266,7 @@ do_init_crypto_tls_c1 (struct context *c) /* Initialize key_type for tls-auth with auth only */ CLEAR (c->c1.ks.tls_auth_key_type); - if (options->authname && options->authname_defined) + if (options->authname) { c->c1.ks.tls_auth_key_type.digest = md_kt_get (options->authname); c->c1.ks.tls_auth_key_type.hmac_length = @@ -2339,8 +2335,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags) else { crypto_adjust_frame_parameters(&c->c2.frame, &c->c1.ks.key_type, - options->ciphername_defined, options->use_iv, options->replay, - packet_id_long_form); + options->use_iv, options->replay, packet_id_long_form); } tls_adjust_frame_parameters (&c->c2.frame); @@ -2468,7 +2463,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags) to.tls_auth.flags |= CO_PACKET_ID_LONG_FORM; crypto_adjust_frame_parameters (&to.frame, &c->c1.ks.tls_auth_key_type, - false, false, true, true); + false, true, true); } /* If we are running over TCP, allow for diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 3d4a6455d..7e08fcdab 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -830,7 +830,6 @@ init_options (struct options *o, const bool init_gc) #endif #ifdef ENABLE_CRYPTO o->ciphername = "BF-CBC"; - o->ciphername_defined = true; #ifdef HAVE_AEAD_CIPHER_MODES /* IV_NCP=2 requires GCM support */ o->ncp_enabled = true; #else @@ -838,7 +837,6 @@ init_options (struct options *o, const bool init_gc) #endif o->ncp_ciphers = "AES-256-GCM:AES-128-GCM"; o->authname = "SHA1"; - o->authname_defined = true; o->prng_hash = "SHA1"; o->prng_nonce_secret_len = 16; o->replay = true; @@ -1618,9 +1616,7 @@ show_settings (const struct options *o) #ifdef ENABLE_CRYPTO SHOW_STR (shared_secret_file); SHOW_INT (key_direction); - SHOW_BOOL (ciphername_defined); SHOW_STR (ciphername); - SHOW_BOOL (authname_defined); SHOW_STR (authname); SHOW_STR (prng_hash); SHOW_INT (prng_nonce_secret_len); @@ -2989,12 +2985,11 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame) { struct frame fake_frame = *frame; struct key_type fake_kt; - init_key_type (&fake_kt, o->ciphername, o->ciphername_defined, - o->authname, o->authname_defined, o->keysize, true, false); + init_key_type (&fake_kt, o->ciphername, o->authname, o->keysize, true, + false); frame_add_to_extra_frame (&fake_frame, -(crypto_max_overhead())); - crypto_adjust_frame_parameters (&fake_frame, &fake_kt, - o->ciphername_defined, o->use_iv, o->replay, - cipher_kt_mode_ofb_cfb (fake_kt.cipher)); + crypto_adjust_frame_parameters (&fake_frame, &fake_kt, o->use_iv, + o->replay, cipher_kt_mode_ofb_cfb (fake_kt.cipher)); frame_finalize(&fake_frame, o->ce.link_mtu_defined, o->ce.link_mtu, o->ce.tun_mtu_defined, o->ce.tun_mtu); msg (D_MTU_DEBUG, "%s: link-mtu %zu -> %d", __func__, link_mtu, @@ -3146,9 +3141,8 @@ options_string (const struct options *o, + (TLS_SERVER == true) <= 1); - init_key_type (&kt, o->ciphername, o->ciphername_defined, - o->authname, o->authname_defined, - o->keysize, true, false); + init_key_type (&kt, o->ciphername, o->authname, o->keysize, true, + false); buf_printf (&out, ",cipher %s", translate_cipher_name_to_openvpn(cipher_kt_name (kt.cipher))); @@ -6646,35 +6640,21 @@ add_option (struct options *options, else if (streq (p[0], "auth") && p[1] && !p[2]) { VERIFY_PERMISSION (OPT_P_GENERAL); - options->authname_defined = true; options->authname = p[1]; if (streq (options->authname, "none")) { - options->authname_defined = false; options->authname = NULL; } } - else if (streq (p[0], "auth") && !p[1]) - { - VERIFY_PERMISSION (OPT_P_GENERAL); - options->authname_defined = true; - } else if (streq (p[0], "cipher") && p[1] && !p[2]) { VERIFY_PERMISSION (OPT_P_NCP); - options->ciphername_defined = true; options->ciphername = p[1]; if (streq (options->ciphername, "none")) { - options->ciphername_defined = false; options->ciphername = NULL; } } - else if (streq (p[0], "cipher") && !p[1]) - { - VERIFY_PERMISSION (OPT_P_GENERAL); - options->ciphername_defined = true; - } else if (streq (p[0], "ncp-ciphers") && p[1] && !p[2]) { VERIFY_PERMISSION (OPT_P_GENERAL|OPT_P_INSTANCE); diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 3d644b7b9..9b7b57cf0 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -469,11 +469,9 @@ struct options const char *shared_secret_file; const char *shared_secret_file_inline; int key_direction; - bool ciphername_defined; const char *ciphername; bool ncp_enabled; const char *ncp_ciphers; - bool authname_defined; const char *authname; int keysize; const char *prng_hash; diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 72001a3c2..a220b79a5 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -1678,8 +1678,7 @@ tls_session_update_crypto_params(struct tls_session *session, } init_key_type (&session->opt->key_type, options->ciphername, - options->ciphername_defined, options->authname, options->authname_defined, - options->keysize, true, true); + options->authname, options->keysize, true, true); bool packet_id_long_form = cipher_kt_mode_ofb_cfb (session->opt->key_type.cipher); session->opt->crypto_flags_and &= ~(CO_PACKET_ID_LONG_FORM); @@ -1689,8 +1688,7 @@ tls_session_update_crypto_params(struct tls_session *session, /* Update frame parameters: undo worst-case overhead, add actual overhead */ frame_add_to_extra_frame (frame, -(crypto_max_overhead())); crypto_adjust_frame_parameters (frame, &session->opt->key_type, - options->ciphername_defined, options->use_iv, options->replay, - packet_id_long_form); + options->use_iv, options->replay, packet_id_long_form); frame_finalize(frame, options->ce.link_mtu_defined, options->ce.link_mtu, options->ce.tun_mtu_defined, options->ce.tun_mtu); frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");