Re: [Openvpn-devel] use of --cipher with no arguments?

[Steffan Karger]

Hi,

On Mon, Jul 25, 2016 at 10:04 PM, Gert Doering  wrote:
> has anyone ever used "--cipher" without an argument?  If yes, what is the
> intended usage?  It sort of "tells openvpn we want crypto!" but does not
> go into detail about it...
>
> Normally, this would just be a random weird option, but I ran across
>
>   --cipher none --cipher
>
> which first tells openvpn "nah, we do not want anything!" and sets
> a pointer to NULL, and then tells openvpn "but please *do* use the
> ciphers already setup!", which core dumps.
>
> This is not remotely exploitable, so not a *security* issue, but a bit
> stupid nonetheless - so I propose we just throw out "--cipher" with
> no arguments (--cipher none, or --cipher bf-cbc would, of course,
> continue to work).
>
> Anyone having a good argument against it?  JJK, do you happen to know
> what this is about?

As the patch I just sent suggests, I don't believe this can be useful at all.

Using just --cipher is a no-op if anything but '--cipher none' is used
(o->ciphername_defined is already set to true), and crashes OpenVPN
otherwise.  Probably just a leftover 'from the old days'.

-Steffan

[Openvpn-devel] [PATCH] Fix "--cipher none --cipher" crash

[Steffan Karger]

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.
That not only fixes the issue, but also cleans up the code a bit.

Signed-off-by: Steffan Karger 
---
 src/openvpn/crypto.c  | 11 ---
 src/openvpn/crypto.h  | 16 +---
 src/openvpn/init.c| 19 +++
 src/openvpn/options.c | 26 --
 src/openvpn/options.h |  2 --
 src/openvpn/ssl.c |  6 ++
 6 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index f9cf62a..d94896c 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 de433ae..3b6bb98 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 authnameThe 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_modeSpecifies wether we are running in TLS mode, which allows
+ *more ciphers than static key mode.
+ * @param warnPrint 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 3f97794..5685b69 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2149,10 +2149,8 @@ do_init_crypto_static (struct context *c, const unsigned 
int flags)
   struct key_direction_state kds;

   /* Get cipher & hash algorithms */
-  init_key_type (>c1.ks.key_type, options->ciphername,
-options->ciphername_defined, options->authname,
-options->authname_defined, options->keysize,
-options->test_crypto, true);
+  init_key_type (>c1.ks.key_type, options->ciphername, 
options->authname,
+options->keysize, options->test_crypto, true);

   /* Read cipher and hmac keys from shared secret file */
   {
@@ -2194,7 +2192,6 @@ do_init_crypto_static (struct context *c, const unsigned 
int flags)
   /* 

[Openvpn-devel] use of --cipher with no arguments?

[Gert Doering]

Hi,

has anyone ever used "--cipher" without an argument?  If yes, what is the
intended usage?  It sort of "tells openvpn we want crypto!" but does not
go into detail about it...

Normally, this would just be a random weird option, but I ran across

  --cipher none --cipher

which first tells openvpn "nah, we do not want anything!" and sets
a pointer to NULL, and then tells openvpn "but please *do* use the
ciphers already setup!", which core dumps.

This is not remotely exploitable, so not a *security* issue, but a bit
stupid nonetheless - so I propose we just throw out "--cipher" with
no arguments (--cipher none, or --cipher bf-cbc would, of course, 
continue to work).

Anyone having a good argument against it?  JJK, do you happen to know
what this is about?

gert


-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

Re: [Openvpn-devel] [PATCH applied] Re: Allow ncp-disable and ncp-ciphers to be specified in ccd files

[Gert Doering]

Hi,

On Mon, Jul 25, 2016 at 09:20:03PM +0200, Gert Doering wrote:
> Your patch has been applied to the master branch.
> 
> commit 834f602fd069118b5d00a9042c9fdb20930257eb
> Author: Steffan Karger
> Date:   Mon Jul 25 20:52:46 2016 +0200
> 
>  Allow ncp-disable and ncp-ciphers to be specified in ccd files
> 
>  Signed-off-by: Steffan Karger 
>  Acked-by: Gert Doering 

... this, obviously, means there should have been an "ACK!" in the mail :-)

So, yes, ACK!

gert


-- 
USENET is *not* the non-clickable part of WWW!
   //www.muc.de/~gert/
Gert Doering - Munich, Germany g...@greenie.muc.de
fax: +49-89-35655025g...@net.informatik.tu-muenchen.de


signature.asc
Description: PGP signature

[Openvpn-devel] [PATCH applied] Re: Allow ncp-disable and ncp-ciphers to be specified in ccd files

[Gert Doering]

Your patch has been applied to the master branch.

commit 834f602fd069118b5d00a9042c9fdb20930257eb
Author: Steffan Karger
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Mon Jul 25 20:52:46 2016 +0200

 Allow ncp-disable and ncp-ciphers to be specified in ccd files

 Signed-off-by: Steffan Karger 
 Acked-by: Gert Doering 
 Message-Id: <1469472766-25131-1-git-send-email-stef...@karger.me>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/12096
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering

[Openvpn-devel] [PATCH applied] Re: Add server-side support for cipher negotiation

[Gert Doering]

Another ACK from me.

Stared at the code, tested it vs. iOS OpenVPN and 2.2/2.3/git master 
clients (works!), and we've discussed it quite a bit on IRC, so it finally
all made sense :-)

Testing this turned up something we need to fix: with the patch set 1-5 
enabled, and "delayed key init" active on the server, mssfix is broken 
("does not do anything") - and if setting "ncp-disable", mssfix works 
again.  Going back to commit d728ebeda8 (4/5 v4) on the server side makes 
mssfix on the server side work, but running it on the client it is still
broken - so I think it's related to the delayed initialization (though 
I have no idea yet what it is).  

Still applying this one, as it's not the culprit.

Your patch has been applied to the master branch.

commit a17aa98180319f34c3240aea617bf8114d0bcaf7
Author: Steffan Karger
List-Post: openvpn-devel@lists.sourceforge.net
Date:   Tue Jun 28 23:36:11 2016 +0200

 Add server-side support for cipher negotiation

 Signed-off-by: Steffan Karger 
 Acked-by: Arne Schwabe 
 Acked-by: Gert Doering 
 Message-Id: <1467149771-10374-1-git-send-email-stef...@karger.me>
 URL: http://article.gmane.org/gmane.network.openvpn.devel/12009
 Signed-off-by: Gert Doering 


--
kind regards,

Gert Doering

Re: [Openvpn-devel] [PATCH] Added client-ip option to NAT

[Arne Schwabe]

Am 05.07.16 um 15:13 schrieb Rafael Gava:
> Hi Arne, sorry for replying so late. 
>
> Below is the NAT client-ip patch fixing the messed up whitespaces.
> The only difference from the previous patch, besides the whitespaces,
> is that I'm considering both strings 'client-ip' and 'localhost' as
> valid options.
>
> Please, whatever problem let me know.
Wasn't the general consensus on the mailing list that making localhost
an alias for client-ip is confusing? I wonder why you introduced it again.

Arne