Skip to contentSkip to conversation

Make sure options->ciphername and options->authname are always defined

PATCH In review Steffan Karger 3 participants 4 messages Sep 28, 2016

lore
Share & tools
lore

Steffan Karger

author
#0 lore raw

PATCH 1/1 Make sure options->ciphername and options->authname are always defined

The NCP code does a strcmp(options->ciphername, ...) without first checking whether options->ciphername is NULL. This could cause a crash when using "--cipher none". This patch fixes that problem by ensuring that options->ciphername (and options->authname) are never NULL. Ensuring that options->ciphername is never null prevents us from having to write null checks everywhere.

Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/crypto.c  | 7 +++++--
 src/openvpn/init.c    | 2 +-
 src/openvpn/options.c | 8 --------
 3 files changed, 6 insertions(+), 11 deletions(-)
src/openvpn/crypto.c +5 -2
index 1b17f9d..3dd4a9e 100644
@@ -759,8 +759,11 @@ init_key_type (struct key_type *kt, const char *ciphername,
 {
   bool aead_cipher = false;
 
+  ASSERT(ciphername);
+  ASSERT(authname);
+
   CLEAR (*kt);
-  if (ciphername)
+  if (strcmp (ciphername, "none") != 0)
     {
       kt->cipher = cipher_kt_get (translate_cipher_name_from_openvpn(ciphername));
       if (!kt->cipher)

@@ -790,7 +793,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)
+  if (strcmp (authname, "none") != 0)
     {
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
 	kt->digest = md_kt_get (authname);
src/openvpn/init.c +1 -1
index 9cbbe9d..49f4220 100644
@@ -2259,7 +2259,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)
+	  if (!streq (options->authname, "none"))
 	    {
 	      c->c1.ks.tls_auth_key_type.digest = md_kt_get (options->authname);
 	      c->c1.ks.tls_auth_key_type.hmac_length =
src/openvpn/options.c +0 -8
index 1358237..53b4617 100644
@@ -6653,19 +6653,11 @@ add_option (struct options *options,
     {
       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname = p[1];
-      if (streq (options->authname, "none"))
-	{
-	  options->authname = NULL;
-	}
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
       VERIFY_PERMISSION (OPT_P_NCP);
       options->ciphername = p[1];
-      if (streq (options->ciphername, "none"))
-	{
-	  options->ciphername = NULL;
-	}
     }
   else if (streq (p[0], "ncp-ciphers") && p[1] && !p[2])
     {
Signature · 1 line
2.7.4
#1 lore raw

Am 28.09.16 um 11:33 schrieb Steffan Karger:

Quoted 7 lines

The NCP code does a strcmp(options->ciphername, ...) without first checking whether options->ciphername is NULL. This could cause a crash when using "--cipher none". This patch fixes that problem by ensuring that options->ciphername (and options->authname) are never NULL. Ensuring that options->ciphername is never null prevents us from having to write null checks everywhere.

ACK from me. It also fixes my test setup to reproduce the bug.

Arne

#2 lore raw
In reply to Steffan Karger

Hi,

Quoted 6 lines · Steffan Karger

The NCP code does a strcmp(options->ciphername, ...) without first checking whether options->ciphername is NULL. This could cause a crash when using "--cipher none". This patch fixes that problem by ensuring that options->ciphername (and options->authname) are never NULL. Ensuring that options->ciphername is never null prevents us from having to write null checks everywhere.

Another ACK from me. Looks reasonable :-) - and passes all tests. (Which means I need more tests to actually catch the next "cipher none" breakage quicker)

Your patch has been applied to master. Due to differences in surrounding code (which do not affect this change) I had to do it manually - code change is the same, patch looks different. Please verify that it's all as it should be :-)

commit 348c416face9a025b618ebcae9d3a74c5a4a242b

Author: Steffan Karger <[email protected]...>
Date:   Wed Sep 28 12:40:51 2016 +0200
    Make sure options->ciphername and options->authname are always defined

    Signed-off-by: Steffan Karger <[email protected]...>
    Acked-by: Arne Schwabe <[email protected]...>
    Acked-by: Gert Doering <[email protected]...>
    Message-Id: <[email protected]...>
    URL: https://www.mail-archive.com/[email protected]/msg12576.html

gert

Signature · 4 lines
USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...
author
#3 lore raw
Quoted 4 lines · Gert Doering

Your patch has been applied to master. Due to differences in surrounding code (which do not affect this change) I had to do it manually - code change is the same, patch looks different. Please verify that it's all as it should be :-)

Thanks. All looks good.

-Steffan