Undo cipher push in client options state if cipher is rejected

Because of the way we re-use the options parser for both config files and pushed options, we always update the local options state when we accept an option. This resulted in a pushed cipher being rejected the first time it was pushed, but being accepted the second time.

This patch is a minimal way to resolve this issue in the master and release/2.4 branches. I'll send a more invasive patch for master, to reset the entire options state on reconnects, later.

Trac: #906

---
 src/openvpn/ssl.c | 2 ++
 1 file changed, 2 insertions(+)

index bbc1c965..f152218e 100644
@@ -1967,6 +1967,8 @@ tls_session_update_crypto_params(struct tls_session *session,
         msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
             options->ciphername, session->opt->config_ciphername,
             options->ncp_ciphers);
+        /* undo cipher push, abort connection setup */
+        options->ciphername = session->opt->config_ciphername;
         return false;
     }
 

2.11.0

Am 26.06.17 um 23:15 schrieb Steffan Karger:

ACK. Not as Steffan says, minimal way to achieve this. Should be applied to both master and 2.4.

Arne

Hi,

Uh. While I find the patch totally logical, and have already finished the ACK-and-merged mail, my compiler disagrees with me...

gcc -DHAVE_CONFIG_H -I. -I../../../openvpn/src/openvpn -I../.. -I../../include -I../../../openvpn/include -I../../../openvpn/src/compat -DPLUGIN_LIBDIR=\"/usr/local/lib/openvpn/plugins\" -g -O2 -std=c99 -MT ssl.o -MD -MP -MF .deps/ssl.Tpo -c -o ssl.o ../../../openvpn/src/openvpn/ssl.c
../../../openvpn/src/openvpn/ssl.c: In function 'tls_session_update_crypto_params':
../../../openvpn/src/openvpn/ssl.c:1971:29: error: assignment of member 'ciphername' in read-only object

         options->ciphername = session->opt->config_ciphername;
                             ^

... wut?

(this is 2.4, but I assume master will look similar enough)

gert

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]...

Because of the way we re-use the options parser for both config files and pushed options, we always update the local options state when we accept an option. This resulted in a pushed cipher being rejected the first time it was pushed, but being accepted the second time.

This patch is a minimal way to resolve this issue in the master and release/2.4 branches. I'll send a more invasive patch for master, to reset the entire options state on reconnects, later.

Trac: #906

---
v2 - send the _entire_ patch, not just 1 hunk...

 src/openvpn/ssl.c | 4 +++-
 src/openvpn/ssl.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

index bbc1c965..f868457e 100644
@@ -1958,7 +1958,7 @@ cleanup:
 
 bool
 tls_session_update_crypto_params(struct tls_session *session,
-                                 const struct options *options, struct frame *frame)
+                                 struct options *options, struct frame *frame)
 {
     if (!session->opt->server
         && 0 != strcmp(options->ciphername, session->opt->config_ciphername)

@@ -1967,6 +1967,8 @@ tls_session_update_crypto_params(struct tls_session *session,
         msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
             options->ciphername, session->opt->config_ciphername,
             options->ncp_ciphers);
+        /* undo cipher push, abort connection setup */
+        options->ciphername = session->opt->config_ciphername;
         return false;
     }
 

index 56ea6013..0e0f68fa 100644
@@ -481,7 +481,7 @@ void tls_update_remote_addr(struct tls_multi *multi,
  * @return true if updating succeeded, false otherwise.
  */
 bool tls_session_update_crypto_params(struct tls_session *session,
-                                      const struct options *options, struct frame *frame);
+                                      struct options *options, struct frame *frame);
 
 /**
  * "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.

2.11.0

ACK. Counting in Arne's ACK as well, as v2 is basically the same actual code change, and the "const" change does not have any further effect ("options" is not const in the callers).

Your patch has been applied to the master and release/2.4 branch.

commit 3be9a1c1cd75627c30dca05bed28c84ad4dc1d37 (master) commit e050c762e2bd3695275ee675f197f062c63e2b6f (release/2.4)

Author: Steffan Karger
Date:   Wed Jun 28 00:20:29 2017 +0200

     Undo cipher push in client options state if cipher is rejected

     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]/msg14984.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering