Got rid of a few magic numbers in ntlm.c

PATCH In review Adriaan de Jong 3 participants 12 messages Oct 24, 2011 Kbuild

lore

Share & tools

lore

Adriaan de Jong

author

2011-10-24 08:46 UTC #0 lore raw

PATCH 1/3 Got rid of a few magic numbers in ntlm.c

Signed-off-by Adriaan de Jong <[email protected]...>

---
 crypto_backend.h  |    6 +++---
 crypto_openssl.c  |    2 +-
 crypto_openssl.h  |    3 +++
 crypto_polarssl.c |    2 +-
 crypto_polarssl.h |    2 ++
 ntlm.c            |   30 +++++++++++++++---------------
 6 files changed, 25 insertions(+), 20 deletions(-)

crypto_backend.h +3 -3

index f16dc35..8622962 100644
@@ -135,9 +135,9 @@ void key_des_fixup (uint8_t *key, int key_len, int ndc);
  * @param src		Buffer containing the 8-byte source.
  * @param dst		Buffer containing the 8-byte destination
  */
-void cipher_des_encrypt_ecb (const unsigned char key[8],
-    unsigned char src[8],
-    unsigned char dst[8]);
+void cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
+    unsigned char src[DES_KEY_LENGTH],
+    unsigned char dst[DES_KEY_LENGTH]);
 
 /*
  *

crypto_openssl.c +1 -1

index 7250782..4f14ce2 100644
@@ -636,7 +636,7 @@ cipher_ctx_final (EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len)
 
 
 void
-cipher_des_encrypt_ecb (const unsigned char key[8],
+cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
     unsigned char *src,
     unsigned char *dst)
 {

crypto_openssl.h +3 -0

index 9bc106a..f883c2a 100644
@@ -67,4 +67,7 @@ typedef HMAC_CTX hmac_ctx_t;
 /** Cipher should decrypt */
 #define OPENVPN_OP_DECRYPT 	0
 
+#define DES_KEY_LENGTH 8
+#define MD4_DIGEST_LENGTH 	16
+
 #endif /* CRYPTO_OPENSSL_H_ */

crypto_polarssl.c +1 -1

index 7ae8973..293a1fd 100644
@@ -397,7 +397,7 @@ int cipher_ctx_final (cipher_context_t *ctx, uint8_t *dst, int *dst_len)
 }
 
 void
-cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_SIZE],
+cipher_des_encrypt_ecb (const unsigned char key[DES_KEY_LENGTH],
     unsigned char *src,
     unsigned char *dst)
 {

crypto_polarssl.h +2 -0

index 3ac2c96..358483a 100644
@@ -66,7 +66,9 @@ typedef md_context_t hmac_ctx_t;
 /** Cipher should decrypt */
 #define OPENVPN_OP_DECRYPT 	POLARSSL_DECRYPT
 
+#define MD4_DIGEST_LENGTH 	16
 #define MD5_DIGEST_LENGTH 	16
 #define SHA_DIGEST_LENGTH 	20
+#define DES_KEY_LENGTH 8
 
 #endif /* CRYPTO_POLARSSL_H_ */

ntlm.c +15 ~~-15~~

index ee33bd4..217173f 100644
@@ -71,10 +71,10 @@ gen_md4_hash (const char* data, int data_len, char *result)
 {
   /* result is 16 byte md4 hash */
   const md_kt_t *md4_kt = md_kt_get("MD4");
-  char md[16];
+  char md[MD4_DIGEST_LENGTH];
 
   md_full(md4_kt, data, data_len, md);
-  memcpy (result, md, 16);
+  memcpy (result, md, MD4_DIGEST_LENGTH);
 }
 
 static void

@@ -190,14 +190,14 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
   char buf2[128]; /* decoded reply from proxy */
   unsigned char phase3[464];
 
-  char md4_hash[21];
+  char md4_hash[MD4_DIGEST_LENGTH+5];
   char challenge[8], ntlm_response[24];
   int i, ret_val;
 
 	char ntlmv2_response[144];
 	char userdomain_u[256]; /* for uppercase unicode username and domain */
 	char userdomain[128];   /* the same as previous but ascii */
-	char ntlmv2_hash[16];
+	char ntlmv2_hash[MD5_DIGEST_LENGTH];
 	char ntlmv2_hmacmd5[16];
 	char *ntlmv2_blob = ntlmv2_response + 16; /* inside ntlmv2_response, length: 128 */
 	int ntlmv2_blob_size=0;

@@ -235,7 +235,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
   gen_md4_hash (pwbuf, unicodize (pwbuf, p->up.password) - 2, md4_hash);
 
   /* pad to 21 bytes */
-  memset (md4_hash + 16, 0, 5);
+  memset(md4_hash + MD4_DIGEST_LENGTH, 0, 5);
 
   ret_val = openvpn_base64_decode( phase_2, (void *)buf2, -1);
   if (ret_val < 0)

@@ -260,7 +260,7 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
 		else
 			msg (M_INFO, "Warning: Username or domain too long");
 		unicodize (userdomain_u, userdomain);
-		gen_hmac_md5(userdomain_u, 2 * strlen(userdomain), md4_hash, 16, ntlmv2_hash);
+		gen_hmac_md5(userdomain_u, 2 * strlen(userdomain), md4_hash, MD5_DIGEST_LENGTH, ntlmv2_hash);
 
 		/* NTLMv2 Blob */
 		memset(ntlmv2_blob, 0, 128);                /* Clear blob buffer */

@@ -292,25 +292,25 @@ ntlm_phase_3 (const struct http_proxy_info *p, const char *phase_2, struct gc_ar
 		memcpy(&ntlmv2_response[8], challenge, 8);
 
 		/* hmac-md5 */
-		gen_hmac_md5(&ntlmv2_response[8], ntlmv2_blob_size + 8, ntlmv2_hash, 16, ntlmv2_hmacmd5);
+		gen_hmac_md5(&ntlmv2_response[8], ntlmv2_blob_size + 8, ntlmv2_hash, MD5_DIGEST_LENGTH, ntlmv2_hmacmd5);
 		
 		/* Add hmac-md5 result to the blob */
-		memcpy(ntlmv2_response, ntlmv2_hmacmd5, 16); /* Note: This overwrites challenge previously written at ntlmv2_response[8..15] */
+		memcpy(ntlmv2_response, ntlmv2_hmacmd5, MD5_DIGEST_LENGTH); /* Note: This overwrites challenge previously written at ntlmv2_response[8..15] */
 	
 	} else { /* Generate NTLM response */
-		unsigned char key1[8], key2[8], key3[8];
+		unsigned char key1[DES_KEY_LENGTH], key2[DES_KEY_LENGTH], key3[DES_KEY_LENGTH];
 
 		create_des_keys ((unsigned char *)md4_hash, key1);
 		cipher_des_encrypt_ecb (key1, challenge, ntlm_response);
 
-		create_des_keys ((unsigned char *)&(md4_hash[7]), key2);
-		cipher_des_encrypt_ecb (key2, challenge, &ntlm_response[8]);
+		create_des_keys ((unsigned char *)&(md4_hash[DES_KEY_LENGTH-1]), key2);
+		cipher_des_encrypt_ecb (key2, challenge, &ntlm_response[DES_KEY_LENGTH]);
 
-		create_des_keys ((unsigned char *)&(md4_hash[14]), key3);
-		cipher_des_encrypt_ecb (key3, challenge, &ntlm_response[16]);
+		create_des_keys ((unsigned char *)&(md4_hash[2*(DES_KEY_LENGTH-1)]), key3);
+		cipher_des_encrypt_ecb (key3, challenge, &ntlm_response[DES_KEY_LENGTH*2]);
 	}
-	
-	
+
+
 	memset (phase3, 0, sizeof (phase3)); /* clear reply */
 
 	strcpy ((char *)phase3, "NTLMSSP\0"); /* signature */

Signature · 1 line

1.7.5.4

Adriaan de Jong

author

2011-10-24 08:46 UTC #1 lore raw

PATCH 2/3 Fixed disabling crypto and SSL

Signed-off-by Adriaan de Jong <[email protected]...>

---
 Makefile.am           |   23 ++++++++---------------
 configure.ac          |    2 --
 crypto_openssl.c      |    4 ++++
 crypto_polarssl.c     |    4 ++++
 options.h             |    2 +-
 pkcs11_openssl.c      |    4 ++--
 pkcs11_polarssl.c     |    4 ++--
 ssl_openssl.c         |    5 +++++
 ssl_polarssl.c        |    5 +++++
 ssl_verify.c          |    4 ++++
 ssl_verify_openssl.c  |    6 ++++++
 ssl_verify_polarssl.c |    6 ++++++
 12 files changed, 47 insertions(+), 22 deletions(-)

Makefile.am +8 ~~-15~~

index 3228798..075270f 100644
@@ -86,6 +86,8 @@ openvpn_SOURCES = \
 	common.h \
 	config-win32.h \
 	crypto.c crypto.h crypto_backend.h \
+	crypto_openssl.c crypto_openssl.h \
+	crypto_polarssl.c crypto_polarssl.h \
 	dhcp.c dhcp.h \
 	errlevel.h \
 	error.c error.h \

@@ -115,6 +117,8 @@ openvpn_SOURCES = \
         ntlm.c ntlm.h \
 	occ.c occ.h occ-inline.h \
 	pkcs11.c pkcs11.h pkcs11_backend.h \
+	pkcs11_openssl.c \
+	pkcs11_polarssl.c \
 	openvpn.c openvpn.h \
 	openvpn-plugin.h \
 	options.c options.h \

@@ -139,7 +143,11 @@ openvpn_SOURCES = \
 	socket.c socket.h \
 	socks.c socks.h \
 	ssl.c ssl.h  ssl_backend.h \
+	ssl_openssl.c ssl_openssl.h \
+	ssl_polarssl.c ssl_polarssl.h \
 	ssl_verify.c ssl_verify.h ssl_verify_backend.h \
+	ssl_verify_openssl.c ssl_verify_openssl.h \
+	ssl_verify_polarssl.c ssl_verify_polarssl.h \
 	status.c status.h \
 	syshead.h \
 	tun.c tun.h \

@@ -153,21 +161,6 @@ configure.h: Makefile
 	awk -f $(srcdir)/configure_h.awk config.h > $@
 	awk -f $(srcdir)/configure_log.awk config.log >> $@
 
-if USE_OPENSSL
-openvpn_SOURCES += \
-	crypto_openssl.c crypto_openssl.h \
-	pkcs11_openssl.c \
-	ssl_openssl.c ssl_openssl.h \
-	ssl_verify_openssl.c ssl_verify_openssl.h
-endif
-if USE_POLARSSL
-openvpn_SOURCES += \
-	crypto_polarssl.c crypto_polarssl.h \
-	pkcs11_polarssl.c \
-	ssl_polarssl.c ssl_polarssl.h \
-	ssl_verify_polarssl.c ssl_verify_polarssl.h
-endif
-
 dist-hook:
 	cd $(distdir) && for i in $(EXTRA_DIST) $(SUBDIRS) ; do find $$i -name .svn -type d -prune -exec rm -rf '{}' ';' ; rm -f `find $$i -type f | grep -E '(^|\/)\.?\#|\~$$|\.s?o$$'` ; done

configure.ac +0 -2

index a6b3019..a4d68e6 100644
@@ -299,8 +299,6 @@ AC_ARG_WITH([ssl-type],
    esac],
    [SSL_LIB="openssl"]
 )
-AM_CONDITIONAL([USE_OPENSSL], [test x$SSL_LIB = xopenssl])
-AM_CONDITIONAL([USE_POLARSSL], [test x$SSL_LIB = xpolarssl])
 
 dnl fix search path, to allow compilers to find syshead.h
 CPPFLAGS="$CPPFLAGS -I${srcdir}"

crypto_openssl.c +4 -0

index 4f14ce2..e43d73c 100644
@@ -29,6 +29,8 @@
 
 #include "syshead.h"
 
+#if defined(USE_CRYPTO) && defined(USE_OPENSSL)
+
 #include "basic.h"
 #include "buffer.h"
 #include "integer.h"

@@ -789,3 +791,5 @@ hmac_ctx_final (HMAC_CTX *ctx, uint8_t *dst)
 
   HMAC_Final (ctx, dst, &in_hmac_len);
 }
+
+#endif /* USE_CRYPTO && USE_OPENSSL */

crypto_polarssl.c +4 -0

index 293a1fd..8119d58 100644
@@ -29,6 +29,8 @@
 
 #include "syshead.h"
 
+#if defined(USE_CRYPTO) && defined(USE_POLARSSL)
+
 #include "errlevel.h"
 #include "basic.h"
 #include "buffer.h"

@@ -555,3 +557,5 @@ hmac_ctx_final (md_context_t *ctx, uint8_t *dst)
 {
   ASSERT(0 == md_hmac_finish(ctx, dst));
 }
+
+#endif /* USE_CRYPTO && USE_POLARSSL */

options.h +1 -1

index abdddba..0f80e70 100644
@@ -77,7 +77,7 @@ struct options_pre_pull
 };
 
 #endif
-#if !defined(USE_OPENSSL) && !defined(USE_POLARSSL)
+#if defined(USE_CRYPTO) && !defined(USE_OPENSSL) && !defined(USE_POLARSSL)
 # error "At least one of OpenSSL or PolarSSL needs to be defined."
 #endif

pkcs11_openssl.c +2 -2

index 5c99bf3..e3463dc 100644
@@ -29,7 +29,7 @@
 
 #include "syshead.h"
 
-#ifdef ENABLE_PKCS11
+#if defined(ENABLE_PKCS11) && defined(USE_OPENSSL)
 
 #include "errlevel.h"
 #include "pkcs11_backend.h"

@@ -185,4 +185,4 @@ cleanup:
     }
   return ret;
 }
-#endif /* ENABLE_PKCS11 */
+#endif /* defined(ENABLE_PKCS11) && defined(ENABLE_OPENSSL) */

pkcs11_polarssl.c +2 -2

index 49d6a6b..4f8bf33 100644
@@ -29,7 +29,7 @@
 
 #include "syshead.h"
 
-#if defined(ENABLE_PKCS11)
+#if defined(ENABLE_PKCS11) && defined(USE_POLARSSL)
 
 #include "errlevel.h"
 #include "pkcs11_backend.h"

@@ -118,4 +118,4 @@ cleanup:
 
   return ret;
 }
-#endif /* defined(ENABLE_PKCS11) */
+#endif /* defined(ENABLE_PKCS11) && defined(USE_POLARSSL) */

ssl_openssl.c +5 -0

index 7f579d2..a1e45d9 100644
@@ -28,6 +28,9 @@
  */
 
 #include "syshead.h"
+
+#if defined(USE_SSL) && defined(USE_OPENSSL)
+
 #include "errlevel.h"
 #include "buffer.h"
 #include "misc.h"

@@ -1286,3 +1289,5 @@ get_highest_preference_tls_cipher (char *buf, int size)
   SSL_free (ssl);
   SSL_CTX_free (ctx);
 }
+
+#endif /* defined(USE_SSL) && defined(USE_OPENSSL) */

ssl_polarssl.c +5 -0

index c95c0c0..9a8c49c 100644
@@ -28,6 +28,9 @@
  */
 
 #include "syshead.h"
+
+#if defined(USE_SSL) && defined(USE_POLARSSL)
+
 #include "errlevel.h"
 #include "ssl_backend.h"
 #include "buffer.h"

@@ -857,3 +860,5 @@ get_highest_preference_tls_cipher (char *buf, int size)
   cipher_name = ssl_get_ciphersuite_name(*ciphers);
   strncpynt (buf, cipher_name, size);
 }
+
+#endif /* defined(USE_SSL) && defined(USE_POLARSSL) */

ssl_verify.c +4 -0

index f0bc994..326b005 100644
@@ -28,6 +28,9 @@
  */
 
 #include "syshead.h"
+
+#if defined(USE_CRYPTO) && defined(USE_SSL)
+
 #include "misc.h"
 #include "manage.h"
 #include "ssl_verify.h"

@@ -1237,3 +1240,4 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
       gc_free (&gc);
     }
 }
+#endif /* defined(USE_CRYPTO) && defined(USE_SSL) */

ssl_verify_openssl.c +6 -0

index 4f090e6..13c2f4e 100644
@@ -27,6 +27,10 @@
  * @file Control Channel Verification Module OpenSSL implementation
  */
 
+#include "syshead.h"
+
+#if defined(USE_SSL) && defined(USE_OPENSSL)
+
 #include "ssl_verify.h"
 #include "ssl_verify_backend.h"
 #include "ssl_openssl.h"

@@ -584,3 +588,5 @@ end:
 
   return retval;
 }
+
+#endif /* defined(USE_SSL) && defined(USE_OPENSSL) */

ssl_verify_polarssl.c +6 -0

index 4cd6383..7fdca1f 100644
@@ -27,6 +27,10 @@
  * @file Control Channel Verification Module PolarSSL backend
  */
 
+#include "syshead.h"
+
+#if defined(USE_SSL) && defined(USE_POLARSSL)
+
 #include "ssl_verify.h"
 #include <polarssl/sha1.h>

@@ -417,3 +421,5 @@ end:
   x509_crl_free(&crl);
   return retval;
 }
+
+#endif /* #if defined(USE_SSL) && defined(USE_POLARSSL) */

Signature · 1 line

1.7.5.4

David Sommerseth

2011-10-24 10:38 UTC #2 lore raw

Re: [Openvpn-devel] [PATCH 2/3] Fixed disabling crypto and SSL

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

Quoted 9 lines · Adriaan de Jong

Signed-off-by Adriaan de Jong <[email protected]...> --- Makefile.am

|   23 ++++++++--------------- configure.ac          |    2 -- 
crypto_openssl.c      |    4 ++++ crypto_polarssl.c     |    4 ++++ 
options.h             |    2 +- pkcs11_openssl.c      |    4 ++-- 
pkcs11_polarssl.c     |    4 ++-- ssl_openssl.c         |    5 +++++ 
ssl_polarssl.c        |    5 +++++ ssl_verify.c          |    4 ++++ 
ssl_verify_openssl.c  |    6 ++++++ ssl_verify_polarssl.c |    6

++++++ 12 files changed, 47 insertions(+), 22 deletions(-)

ACK!

I've tried this on master, configuring with all combinations of
- --disable-crypto and --disable-ssl ... and all compiled for me without any issues now. Pushed to master branch on -testing and -stable.

commit 31ea2ee4ca642a4d8bbdac3dadb44eca11f52e35

Author: Adriaan de Jong <[email protected]...>
Date:   Mon Oct 24 10:46:01 2011 +0200

    Fixed disabling crypto and SSL

    Signed-off-by: Adriaan de Jong <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6lQEAACgkQDC186MBRfrp3YwCfaglxEXMbvjLx+2wt7gpWghl/ SG4AoINQiI7emZn8ra5JqRMJu5XnAwGd
=3tgB
-----END PGP SIGNATURE-----

Adriaan de Jong

author

2011-10-24 08:46 UTC #3 lore raw

PATCH 3/3 Changed default algorithm for PolarSSL to AES-128, as BF is not supported

In reply to Adriaan de Jong

Signed-off-by Adriaan de Jong <[email protected]...>

---
 options.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

options.c +5 -0

index 39e7a57..d917072 100644
@@ -810,7 +810,12 @@ init_options (struct options *o, const bool init_gc)
   o->server_poll_timeout = 0;
 #endif
 #ifdef USE_CRYPTO
+#ifdef USE_OPENSSL
   o->ciphername = "BF-CBC";
+#endif
+#ifdef USE_POLARSSL
+  o->ciphername = "AES-128-CBC";
+#endif
   o->ciphername_defined = true;
   o->authname = "SHA1";
   o->authname_defined = true;

Signature · 1 line

1.7.5.4

Jan Just Keijser

2011-10-24 08:55 UTC #4 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

I'd NACK this patch : the default behaviour of OpenVPN should be independent of the SSL implementation.

JJK

Quoted 23 lines · Adriaan de Jong

Signed-off-by Adriaan de Jong <[email protected]...>

---
 options.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

options.c +5 -0

index 39e7a57..d917072 100644
@@ -810,7 +810,12 @@ init_options (struct options *o, const bool init_gc)
   o->server_poll_timeout = 0;
 #endif
 #ifdef USE_CRYPTO
+#ifdef USE_OPENSSL
   o->ciphername = "BF-CBC";
+#endif
+#ifdef USE_POLARSSL
+  o->ciphername = "AES-128-CBC";
+#endif
   o->ciphername_defined = true;
   o->authname = "SHA1";
   o->authname_defined = true;

Adriaan de Jong

author

2011-10-24 08:58 UTC #5 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

Unfortunately BF isn't supported in PolarSSL though.

Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)

Adriaan

Quoted 38 lines

-----Original Message-----
From: Jan Just Keijser [mailto:[email protected]...]
Sent: maandag 24 oktober 2011 10:55
To: Adriaan de Jong
Cc: [email protected]
Subject: Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported
I'd NACK this patch : the default behaviour of OpenVPN should be independent of the SSL implementation.
JJK
options.c init_options (struct options *o, const bool
Quoted 10 lines · Adriaan de Jong
Signed-off-by Adriaan de Jong <[email protected]...>
---
 options.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
options.c +0 -0
index 39e7a57..d917072 100644
@@ -810,7 +810,12 @@ init_options (struct options *o, const bool
init_gc)
Quoted 13 lines
options.c +5 -0
   o->server_poll_timeout = 0;
 #endif
 #ifdef USE_CRYPTO
+#ifdef USE_OPENSSL
   o->ciphername = "BF-CBC";
+#endif
+#ifdef USE_POLARSSL
+  o->ciphername = "AES-128-CBC";
+#endif
   o->ciphername_defined = true;
   o->authname = "SHA1";
   o->authname_defined = true;

Jan Just Keijser

2011-10-24 09:03 UTC #6 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

Quoted 4 lines · Adriaan de Jong

Unfortunately BF isn't supported in PolarSSL though.
Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)

hmmm then perhaps the default should be changed to AES-128 ?

I foresee a lot of openvpn-users mails and forums hits from people saying stuff like 'I am running opevpn 2.1 on the server using default settings and am connecting using openvpn 2.3/polarssl and bla bla' : we need to think about how we want to tackle issues like that (is there a way to determine at runtime which version of which SSL implementation is used? OpenSSL has such a (runtime!) call.

The ideal solution would be to set a 'crypto negotiation phase' during which the client and server negotiate about the available crypto routines, pretty much like the "standard" TLS/SSL protocol does (and which is done on the OpenVPN control channel, but not on the data channel).

JJK

Quoted 44 lines

Quoted 42 lines
-----Original Message-----
From: Jan Just Keijser [mailto:[email protected]...]
Sent: maandag 24 oktober 2011 10:55
To: Adriaan de Jong
Cc: [email protected]
Subject: Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported
I'd NACK this patch : the default behaviour of OpenVPN should be independent of the SSL implementation.
JJK
options.c init_options (struct options *o, const bool · line 810
Quoted 11 lines · Adriaan de Jong
Signed-off-by Adriaan de Jong <[email protected]...>
---
 options.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)
options.c +0 -0
index 39e7a57..d917072 100644
@@ -810,7 +810,12 @@ init_options (struct options *o, const bool
 
init_gc)
Quoted 14 lines
options.c +5 -0
   o->server_poll_timeout = 0;
 #endif
 #ifdef USE_CRYPTO
+#ifdef USE_OPENSSL
   o->ciphername = "BF-CBC";
+#endif
+#ifdef USE_POLARSSL
+  o->ciphername = "AES-128-CBC";
+#endif
   o->ciphername_defined = true;
   o->authname = "SHA1";
   o->authname_defined = true;

Adriaan de Jong

author

2011-10-24 09:18 UTC #7 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

Quoted 7 lines · Jan Just Keijser

Quoted 4 lines · Adriaan de Jong
Unfortunately BF isn't supported in PolarSSL though.
Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)
hmmm then perhaps the default should be changed to AES-128 ?

That's an interesting option, but changing that default might cause even more pain :)

Quoted 1 line

I foresee a lot of openvpn-users mails and forums hits from people saying stuff like 'I am running opevpn 2.1 on the server using default settings and am connecting using openvpn 2.3/polarssl and bla bla' : we need to think about how we want to tackle issues like that (is there a way to determine at runtime which version of which SSL implementation is used? OpenSSL has such a (runtime!) call.

You can see the local SSL implementation in the header, but not the remote one (at the moment).

Quoted 1 line

The ideal solution would be to set a 'crypto negotiation phase' during which the client and server negotiate about the available crypto routines, pretty much like the "standard" TLS/SSL protocol does (and which is done on the OpenVPN control channel, but not on the data channel).

That might be a good idea, as long as there are good filter options to enforce policy. It would probably require a protocol change though.

Adriaan

David Sommerseth

2011-10-24 09:50 UTC #8 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

In reply to Adriaan de Jong

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

Quoted 4 lines · Adriaan de Jong

Unfortunately BF isn't supported in PolarSSL though.
Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)

I agree with JJK, that this is probably not a clever idea - to change the default encryption algorithm.

What about to enforce the use of --cipher when OpenVPN is compiled with PolarSSL support? Stating that blowfish isn't supported and another cipher must explicit be set.

kind regards,

David Sommerseth

Quoted 9 lines

Quoted 9 lines
-----Original Message----- From: Jan Just Keijser [mailto:[email protected]...] Sent: maandag 24 oktober 2011 10:55 To:
Adriaan de Jong Cc: [email protected] Subject: Re:
[Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported
I'd NACK this patch : the default behaviour of OpenVPN should be independent of the SSL implementation.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6lNNgACgkQDC186MBRfrolbQCgiXigb8L0jiWXgWBs/H+qhiMJ EpYAnRJHzHUj83lauUAUe0AIxSHpe+hY
=aIa+
-----END PGP SIGNATURE-----

Adriaan de Jong

author

2011-10-24 10:39 UTC #9 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

Quoted 6 lines · David Sommerseth

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Quoted 2 lines · Adriaan de Jong
Unfortunately BF isn't supported in PolarSSL though.

Quoted 8 lines

Quoted 2 lines
Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)
I agree with JJK, that this is probably not a clever idea - to change the default encryption algorithm.
What about to enforce the use of --cipher when OpenVPN is compiled with PolarSSL support? Stating that blowfish isn't supported and another cipher must explicit be set.

I like this idea. I'll see about implementing it instead of patch 3/3.

Perhaps in the long term we could try going for a protocol change as suggested by JJK.

Adriaan

David Sommerseth

2011-10-24 10:43 UTC #10 lore raw

Re: [Openvpn-devel] [PATCH 3/3] Changed default algorithm for PolarSSL to AES-128, as BF is not supported

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

Quoted 8 lines · Adriaan de Jong

Quoted 7 lines · David Sommerseth
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
Quoted 4 lines · Adriaan de Jong
Unfortunately BF isn't supported in PolarSSL though.
Do you have any other suggestions? I'm open to most ideas other than "implement blowfish" :)

Quoted 8 lines

Quoted 6 lines
I agree with JJK, that this is probably not a clever idea - to change the default encryption algorithm.
What about to enforce the use of --cipher when OpenVPN is compiled with PolarSSL support? Stating that blowfish isn't supported and another cipher must explicit be set.
I like this idea. I'll see about implementing it instead of patch 3/3.

Great! Then we'll consider patch 3/3 as officially NACKed.

Quoted 2 lines

Perhaps in the long term we could try going for a protocol change as suggested by JJK.

Agreed! That is something which falls natural into the OpenVPN 3 development, where we most likely will do changes to with the wire protocol as well.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6lQVAACgkQDC186MBRfroz6ACeLb3dYOQbb0ydX79CefGlclxd RLQAoKDHRJxK3+IyJWBjyS1amcP9x4vL
=9ynn
-----END PGP SIGNATURE-----

David Sommerseth

2011-10-24 10:51 UTC #11 lore raw

In reply to Adriaan de Jong

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

Quoted 6 lines · Adriaan de Jong

Signed-off-by Adriaan de Jong <[email protected]...> ---
crypto_backend.h  |    6 +++--- crypto_openssl.c  |    2 +- 
crypto_openssl.h  |    3 +++ crypto_polarssl.c |    2 +- 
crypto_polarssl.h |    2 ++ ntlm.c            |   30
+++++++++++++++--------------- 6 files changed, 25 insertions(+), 20 deletions(-)

As this is the same patch [1] as the one in the original PolarSSL patch inclusion round [2], I kept James Yonan's ACK and gave myself an extra ACK to this one. The reason it failed was due to a formatting patch which was NACKed during the review which the previous patch [1] depended on.

Pushed out new master branch to -testing and -stable trees.

commit 9788322b9566101119484d992364e8b1bb1d4dd4

Author: Adriaan de Jong <[email protected]...>
Date:   Mon Oct 24 10:46:00 2011 +0200

    Got rid of a few magic numbers in ntlm.c

    Signed-off-by: Adriaan de Jong <[email protected]...>
    Acked-by: James Yonan <[email protected]...>
    Acked-by: David Sommerseth <[email protected]...>
    Signed-off-by: David Sommerseth <[email protected]...>

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk6lQygACgkQDC186MBRfroNbgCfUAuA7TMrvyuh4sEv/ZYy8wyB k14AnA2gpY0cL+M/36UNDyF96XnPOLtD
=dLJr
-----END PGP SIGNATURE-----