Skip to contentSkip to conversation

Add support for cipher negotiation

PATCH Superseded Steffan Karger 5 participants 40 messages Jun 8, 2016 – Jul 25, 2016

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Add support for cipher negotiation

Hi,

The following five patches add support for cipher negotiation, according to the 'IV_NCP=2' mechanism as described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

Please test and provide comments. I ran the 'make check' test suite, including t_client tests, and performed manual testing. But, as Gert eloquently puts it, this is likely to 'break stuff in interesting ways', so more testing (and review) is very welcome!

-Steffan

PS I already sent 1/5 to the list before, but it did not get reviewed back then.
This patch set requires it to work properly, so I included it anyway.

#1 lore raw

PATCH 1/5 Don't limit max incoming message size based on c2->frame

"Be conservative in what you send, be liberal in what you accept"

When receiving packets, the real limitation of how much data we can accept is the size of our internal buffers, not the maximum size we expect incoming packets to have.

I ran into this while working on cipher negotiation, which will need separate bookkeeping for the required internal buffer size, and the link/tun MTU. Basing this code on the buffer size instead of c2->frame makes that easier. A nice side-effect of this change is that it simplifies the code.

This should also reduce the impact of using asymmetric tun/link MTU's, such as in trac ticket #647.

---
 src/openvpn/forward.c | 1 -
 src/openvpn/socket.c  | 9 +++------
 src/openvpn/socket.h  | 4 +---
 3 files changed, 4 insertions(+), 10 deletions(-)
src/openvpn/forward.c +0 -1
index 4a91f92..2c9a082 100644
@@ -669,7 +669,6 @@ read_incoming_link (struct context *c)
 
   status = link_socket_read (c->c2.link_socket,
 			     &c->c2.buf,
-			     MAX_RW_SIZE_LINK (&c->c2.frame),
 			     &c->c2.from);
 
   if (socket_connection_reset (c->c2.link_socket, status))
src/openvpn/socket.c +3 -6
index f7264ef..d2872be 100644
@@ -2886,7 +2886,6 @@ union openvpn_pktinfo {
 static socklen_t
 link_socket_read_udp_posix_recvmsg (struct link_socket *sock,
 				    struct buffer *buf,
-				    int maxsize,
 				    struct link_socket_actual *from)
 {
   struct iovec iov;
@@ -2895,7 +2894,7 @@ link_socket_read_udp_posix_recvmsg (struct link_socket *sock,
   socklen_t fromlen = sizeof (from->dest.addr);
 
   iov.iov_base = BPTR (buf);
-  iov.iov_len = maxsize;
+  iov.iov_len = buf_forward_capacity_total (buf);
   mesg.msg_iov = &iov;
   mesg.msg_iovlen = 1;
   mesg.msg_name = &from->dest.addr;
@@ -2954,20 +2953,18 @@ link_socket_read_udp_posix_recvmsg (struct link_socket *sock,
 int
 link_socket_read_udp_posix (struct link_socket *sock,
 			    struct buffer *buf,
-			    int maxsize,
 			    struct link_socket_actual *from)
 {
   socklen_t fromlen = sizeof (from->dest.addr);
   socklen_t expectedlen = af_addr_size(sock->info.af);
   addr_zero_host(&from->dest);
-  ASSERT (buf_safe (buf, maxsize));
 #if ENABLE_IP_PKTINFO
   /* Both PROTO_UDPv4 and PROTO_UDPv6 */
   if (sock->info.proto == PROTO_UDP && sock->sockflags & SF_USE_IP_PKTINFO)
-    fromlen = link_socket_read_udp_posix_recvmsg (sock, buf, maxsize, from);
+    fromlen = link_socket_read_udp_posix_recvmsg (sock, buf, from);
   else
 #endif
-    buf->len = recvfrom (sock->sd, BPTR (buf), maxsize, 0,
+    buf->len = recvfrom (sock->sd, BPTR (buf), buf_forward_capacity(buf), 0,
 			 &from->dest.addr.sa, &fromlen);
   /* FIXME: won't do anything when sock->info.af == AF_UNSPEC */
   if (buf->len >= 0 && expectedlen && fromlen != expectedlen)
src/openvpn/socket.h +1 -3
index b154bc0..6f4d34f 100644
@@ -961,7 +961,6 @@ link_socket_read_udp_win32 (struct link_socket *sock,
 
 int link_socket_read_udp_posix (struct link_socket *sock,
 				struct buffer *buf,
-				int maxsize,
 				struct link_socket_actual *from);
 
 #endif
@@ -970,7 +969,6 @@ int link_socket_read_udp_posix (struct link_socket *sock,
 static inline int
 link_socket_read (struct link_socket *sock,
 		  struct buffer *buf,
-		  int maxsize,
 		  struct link_socket_actual *from)
 {
   if (proto_is_udp(sock->info.proto)) /* unified UDPv4 and UDPv6 */
@@ -980,7 +978,7 @@ link_socket_read (struct link_socket *sock,
 #ifdef WIN32
       res = link_socket_read_udp_win32 (sock, buf, from);
 #else
-      res = link_socket_read_udp_posix (sock, buf, maxsize, from);
+      res = link_socket_read_udp_posix (sock, buf, from);
 #endif
       return res;
     }
Signature · 1 line
2.7.4
#2 lore raw

Re: [Openvpn-devel] [PATCH 1/5] Don't limit max incoming message size based on c2->frame

Am 08.06.16 um 14:20 schrieb Steffan Karger:

Quoted 6 lines

"Be conservative in what you send, be liberal in what you accept"

When receiving packets, the real limitation of how much data we can accept is the size of our internal buffers, not the maximum size we expect incoming packets to have.

Quoted 10 lines

I ran into this while working on cipher negotiation, which will need separate bookkeeping for the required internal buffer size, and the link/tun MTU. Basing this code on the buffer size instead of c2->frame makes that easier. A nice side-effect of this change is that it simplifies the code.

This should also reduce the impact of using asymmetric tun/link MTU's, such as in trac ticket #647.

ACK I think this one should be safe.

Arne

#3 lore raw

[Openvpn-devel] [PATCH applied] Re: Don't limit max incoming message size based on c2->frame

In reply to Steffan Karger

Another ACK from me.

I'm not sure I understand all the intricacies of our buffer/frame handling and *why* there is always enough "capacity" present, but since we're never reading more bytes than we have capacity, this is definitely *safe*.

Obviously the old code never requested more bytes than capacity (buf_safe()), so the new code will read() "at least as many bytes as before" - and since I remember that there is quite a bit of headroom in the frame stuff, the goal should be achieved.

Your patch has been applied to the master and release/2.3 branch (the latter because it helps with certain incompatibilities in --*-mtu setup, which otherwise lead to hard-to-debug TLS failures).

commit 3c1b19e04745177185decd14da82c71458442b82 (master) commit 358f513c008bf01fadb82759ac75ffb8613fc785 (release/2.3)

Author: Steffan Karger
Date:   Wed Jun 8 14:20:39 2016 +0200
     Don't limit max incoming message size based on c2->frame

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11850
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#4 lore raw

PATCH 2/5 cleanup: remove alloc_buffers argument from multi_top_init()

In reply to Steffan Karger

multi_top_init() is always called with alloc_buffers=true, so just remove the argument and alloc unconditionally.

---
 src/openvpn/mtcp.c  | 2 +-
 src/openvpn/mudp.c  | 2 +-
 src/openvpn/multi.c | 6 ++----
 src/openvpn/multi.h | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)
src/openvpn/mtcp.c +1 -1
index 9926d47..78e5ccd 100644
@@ -696,7 +696,7 @@ tunnel_server_tcp (struct context *top)
   multi_init (&multi, top, true, MC_SINGLE_THREADED);
 
   /* initialize our cloned top object */
-  multi_top_init (&multi, top, true);
+  multi_top_init (&multi, top);
 
   /* initialize management interface */
   init_management_callback_multi (&multi);
src/openvpn/mudp.c +1 -1
index ce67206..6e0568e 100644
@@ -283,7 +283,7 @@ tunnel_server_udp_single_threaded (struct context *top)
   multi_init (&multi, top, false, MC_SINGLE_THREADED);
 
   /* initialize our cloned top object */
-  multi_top_init (&multi, top, true);
+  multi_top_init (&multi, top);
 
   /* initialize management interface */
   init_management_callback_multi (&multi);
src/openvpn/multi.c +2 -4
index 4c43fcc..ba7f2c0 100644
@@ -2859,12 +2859,10 @@ multi_process_per_second_timers_dowork (struct multi_context *m)
 }
 
 void
-multi_top_init (struct multi_context *m, const struct context *top, const bool alloc_buffers)
+multi_top_init (struct multi_context *m, const struct context *top)
 {
   inherit_context_top (&m->top, top);
-  m->top.c2.buffers = NULL;
-  if (alloc_buffers)
-    m->top.c2.buffers = init_context_buffers (&top->c2.frame);
+  m->top.c2.buffers = init_context_buffers (&top->c2.frame);
 }
 
 void
src/openvpn/multi.h +1 -1
index ec1e7ab..9d8185d 100644
@@ -233,7 +233,7 @@ const char *multi_instance_string (const struct multi_instance *mi, bool null, s
 void multi_init (struct multi_context *m, struct context *t, bool tcp_mode, int thread_mode);
 void multi_uninit (struct multi_context *m);
 
-void multi_top_init (struct multi_context *m, const struct context *top, const bool alloc_buffers);
+void multi_top_init (struct multi_context *m, const struct context *top);
 void multi_top_free (struct multi_context *m);
 
 struct multi_instance *multi_create_instance (struct multi_context *m, const struct mroute_addr *real);
Signature · 1 line
2.7.4
#5 lore raw

Re: [Openvpn-devel] [PATCH 2/5] cleanup: remove alloc_buffers argument from multi_top_init()

Am 08.06.16 um 14:20 schrieb Steffan Karger:

Quoted 4 lines

multi_top_init() is always called with alloc_buffers=true, so just remove the argument and alloc unconditionally.

Si

ACK. This is one is easy!

Arne

#6 lore raw

[Openvpn-devel] [PATCH applied] Re: cleanup: remove alloc_buffers argument from multi_top_init()

In reply to Steffan Karger

ACK. Easy to review :-) - Arne just beat me to it.

Your patch has been applied to the master branch.

commit d92718141287a0f9adcffec5c2fa7d76213162cc

Author: Steffan Karger
Date:   Wed Jun 8 14:20:40 2016 +0200
     cleanup: remove alloc_buffers argument from multi_top_init()

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/11851
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#7 lore raw

PATCH 3/5 Add client-side support for cipher negotiation

In reply to Steffan Karger

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negatiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

---
 src/openvpn/crypto.c         | 18 +++++++++---
 src/openvpn/crypto.h         |  4 +++
 src/openvpn/crypto_backend.h |  6 ++++
 src/openvpn/init.c           | 51 +++++++++++++++++++++++----------
 src/openvpn/init.h           |  3 +-
 src/openvpn/misc.h           |  7 +++++
 src/openvpn/mtu.c            |  2 --
 src/openvpn/multi.c          |  2 +-
 src/openvpn/route.c          |  4 +--
 src/openvpn/ssl.c            | 67 +++++++++++++++++++++++++++++++++++++++-----
 src/openvpn/ssl.h            | 14 +++++++++
 src/openvpn/ssl_common.h     | 16 +++++++++--
 12 files changed, 159 insertions(+), 35 deletions(-)
src/openvpn/crypto.c +14 -4
index 5c392aa..e43c30b 100644
@@ -709,10 +709,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
   return ret;
 }
 
-/*
- * How many bytes will we add to frame buffer for a given
- * set of crypto options?
- */
 void
 crypto_adjust_frame_parameters(struct frame *frame,
 			       const struct key_type* kt,
@@ -746,6 +742,14 @@ crypto_adjust_frame_parameters(struct frame *frame,
       __func__, crypto_overhead);
 }
 
+size_t
+crypto_max_overhead(void)
+{
+  return packet_id_size(true) + OPENVPN_MAX_IV_LENGTH +
+      OPENVPN_MAX_CIPHER_BLOCK_SIZE +
+      MAX(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
+}
+
 /*
  * Build a struct key_type.
  */
@@ -774,6 +778,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
 #endif
 	    ))
 	msg (M_FATAL, "Cipher '%s' mode not supported", ciphername);
+
+      if (OPENVPN_MAX_CIPHER_BLOCK_SIZE < cipher_kt_block_size(kt->cipher))
+	msg (M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
     }
   else
     {
@@ -785,6 +792,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
 	kt->digest = md_kt_get (authname);
 	kt->hmac_length = md_kt_size (kt->digest);
+
+	if (OPENVPN_MAX_HMAC_SIZE < kt->hmac_length)
+	  msg (M_FATAL, "HMAC '%s' not allowed: digest size too big.", authname);
       }
     }
   else if (!aead_cipher)
src/openvpn/crypto.h +4 -0
index 63d7040..de433ae 100644
@@ -226,6 +226,7 @@ struct key_ctx_bi
 				 *   direction. */
   struct key_ctx decrypt;       /**< cipher and/or HMAC contexts for
                                  *   receiving direction. */
+  bool initialized;
 };
 
 /**
@@ -385,6 +386,7 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
 
 /** @} name Functions for performing security operations on data channel packets */
 
+/** 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,
@@ -392,6 +394,8 @@ void crypto_adjust_frame_parameters(struct frame *frame,
 				    bool packet_id,
 				    bool packet_id_long_form);
 
+/** Return the worst-case OpenVPN crypto overhead (in bytes) */
+size_t crypto_max_overhead(void);
 
 /* Minimum length of the nonce used by the PRNG */
 #define NONCE_SECRET_LEN_MIN 16
src/openvpn/crypto_backend.h +6 -0
index 3893f14..a699673 100644
@@ -41,6 +41,12 @@
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16
 
+/* Maximum cipher block size (bytes) */
+#define OPENVPN_MAX_CIPHER_BLOCK_SIZE 32
+
+/* Maximum HMAC digest size (bytes) */
+#define OPENVPN_MAX_HMAC_SIZE 	64
+
 /** Struct used in cipher name translation table */
 typedef struct {
   const char *openvpn_name;	/**< Cipher name used by OpenVPN */
src/openvpn/init.c +36 -15

104 lines not on this page · raw message

src/openvpn/init.h +2 -1
index a819bd2..73bcb74 100644
@@ -106,7 +106,8 @@ void inherit_context_top (struct context *dest,
 
 void close_context (struct context *c, int sig, unsigned int flags);
 
-struct context_buffers *init_context_buffers (const struct frame *frame);
+struct context_buffers *init_context_buffers (const struct frame *frame,
+					      size_t extra_bytes);
 
 void free_context_buffers (struct context_buffers *b);
 
src/openvpn/misc.h +7 -0
index 65a6e55..82d1d48 100644
@@ -102,6 +102,13 @@ openvpn_run_script (const struct argv *a, const struct env_set *es, const unsign
   return openvpn_execve_check(a, es, flags | S_SCRIPT, msg);
 }
 
+#ifndef MIN
+#define MIN(a,b) ((a) < (b) ? (a) : (b))
+#endif
+
+#ifndef MAX
+#define MAX(a,b) ((a) > (b) ? (a) : (b))
+#endif
 
 #ifdef HAVE_STRERROR
 /* a thread-safe version of strerror */
src/openvpn/mtu.c +0 -2
index 24531c9..64d1cf3 100644
@@ -78,8 +78,6 @@ frame_finalize (struct frame *frame,
     }
 
   frame->link_mtu_dynamic = frame->link_mtu;
-
-  frame->extra_buffer += PAYLOAD_ALIGN;
 }
 
 /*
src/openvpn/multi.c +1 -1
index ba7f2c0..117f768 100644
@@ -2862,7 +2862,7 @@ void
 multi_top_init (struct multi_context *m, const struct context *top)
 {
   inherit_context_top (&m->top, top);
-  m->top.c2.buffers = init_context_buffers (&top->c2.frame);
+  m->top.c2.buffers = init_context_buffers (&top->c2.frame, 0);
 }
 
 void
src/openvpn/route.c +1 -3
index a90195f..578a708 100644
@@ -3265,8 +3265,6 @@ struct rtmsg {
 #define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
 #endif
 
-#define max(a,b) ((a) > (b) ? (a) : (b))
-
 void
 get_default_gateway (struct route_gateway_info *rgi)
 {
@@ -3433,7 +3431,7 @@ get_default_gateway (struct route_gateway_info *rgi)
 #if defined(TARGET_SOLARIS)
 	  const size_t len = sizeof(ifr->ifr_name) + sizeof(ifr->ifr_addr);
 #else
-	  const size_t len = sizeof(ifr->ifr_name) + max(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
+	  const size_t len = sizeof(ifr->ifr_name) + MAX(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
 #endif
 
 	  if (!ifr->ifr_addr.sa_family)
src/openvpn/ssl.c +60 -7

116 lines not on this page · raw message

src/openvpn/ssl.h +14 -0
index d9ff8d0..416f426 100644
@@ -475,6 +475,20 @@ bool tls_rec_payload (struct tls_multi *multi,
 void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
+/**
+ * Update TLS session crypto parameters (cipher and auth) and derive data
+ * channel keys based on the supplied options.
+ *
+ * @param session	The TLS session to update.
+ * @param options	The options to use when updating session.
+ * @param frame		The frame options for this session (frame overhead is
+ * 			adjusted based on the selected cipher/auth).
+ *
+ * @return true if updating succeeded, false otherwise.
+ */
+bool tls_session_update_crypto_params(struct tls_session *session,
+    const struct options *options, struct frame *frame);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
src/openvpn/ssl_common.h +14 -2
index a0df0ff..9183dab 100644
@@ -149,7 +149,12 @@ struct key_source2 {
 struct key_state
 {
   int state;
-  int key_id;			/* inherited from struct tls_session below */
+
+  /**
+   * Key id for this key_state,  inherited from struct tls_session.
+   * @see tls_session::key_id.
+   */
+  int key_id;
 
   struct key_state_ssl ks_ssl;	/* contains SSL object and BIOs for the control channel */
 
@@ -231,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
 #endif
@@ -367,7 +373,13 @@ struct tls_session
 
   int initial_opcode;		/* our initial P_ opcode */
   struct session_id session_id;	/* our random session ID */
-  int key_id;			/* increments with each soft reset (for key renegotiation) */
+
+  /**
+   * The current active key id, used to keep track of renegotiations.
+   * key_id increments with each soft reset to KEY_ID_MASK then recycles back
+   * to 1.  This way you know that if key_id is 0, it is the first key.
+   */
+  int key_id;
 
   int limit_next;               /* used for traffic shaping on the control channel */
 
Signature · 1 line
2.7.4
#8 lore raw

Re: [Openvpn-devel] [PATCH 3/5] Add client-side support for cipher negotiation

src/openvpn/init.c free_context_buffers (struct context_buffers *b) · line 2760

Quoted 6 lines
src/openvpn/init.c +5 -0
+#ifdef ENABLE_CRYPTO
+  if (c->options.pull)
+    {
+      /* If we use --pull, we don't know the crypto overhead yet. */
+      msg (M_WARN, "*** Account for max crypto overhead ***");

That message will surely confuse users.

Arne

#9 lore raw

Re: [Openvpn-devel] [PATCH 3/5] Add client-side support for cipher negotiation

Quoted 8 lines · Arne Schwabe

src/openvpn/init.c free_context_buffers (struct context_buffers *b) · line 2760

Quoted 6 lines
src/openvpn/init.c +5 -0
+#ifdef ENABLE_CRYPTO
+  if (c->options.pull)
+    {
+      /* If we use --pull, we don't know the crypto overhead yet. */
+      msg (M_WARN, "*** Account for max crypto overhead ***");

That message will surely confuse users.

Oops, that should not have been there anymore.

FYI - (as reported by Arne on IRC) this patch still has some issues when used with TCP mode. I'm working on a v2, coming soon :)

-Steffan

#10 lore raw

PATCH v2 3/5 Add client-side support for cipher negotiation

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negotiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

v2:
* Account for crypto overhead through struct frame. This is less transparant, but the code has been built to work this way. The previous approach didn't work with TCP mode (or --port-share).
* Calculate the link-mtu sent in the options string based on the crypto parameters specified in the config file (prevents link-mtu warnings in older peers when connecting).

---
 src/openvpn/crypto.c         | 18 +++++++++---
 src/openvpn/crypto.h         |  4 +++
 src/openvpn/crypto_backend.h |  6 ++++
 src/openvpn/init.c           | 28 ++++++++++++++----
 src/openvpn/misc.h           |  7 +++++
 src/openvpn/mtu.c            |  2 --
 src/openvpn/options.c        | 35 ++++++++++++++++++++--
 src/openvpn/route.c          |  4 +--
 src/openvpn/ssl.c            | 69 +++++++++++++++++++++++++++++++++++++++-----
 src/openvpn/ssl.h            | 14 +++++++++
 src/openvpn/ssl_common.h     | 16 ++++++++--
 11 files changed, 177 insertions(+), 26 deletions(-)
src/openvpn/crypto.c +14 -4
index 5c392aa..e43c30b 100644
@@ -709,10 +709,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
   return ret;
 }
 
-/*
- * How many bytes will we add to frame buffer for a given
- * set of crypto options?
- */
 void
 crypto_adjust_frame_parameters(struct frame *frame,
 			       const struct key_type* kt,
@@ -746,6 +742,14 @@ crypto_adjust_frame_parameters(struct frame *frame,
       __func__, crypto_overhead);
 }
 
+size_t
+crypto_max_overhead(void)
+{
+  return packet_id_size(true) + OPENVPN_MAX_IV_LENGTH +
+      OPENVPN_MAX_CIPHER_BLOCK_SIZE +
+      MAX(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
+}
+
 /*
  * Build a struct key_type.
  */
@@ -774,6 +778,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
 #endif
 	    ))
 	msg (M_FATAL, "Cipher '%s' mode not supported", ciphername);
+
+      if (OPENVPN_MAX_CIPHER_BLOCK_SIZE < cipher_kt_block_size(kt->cipher))
+	msg (M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
     }
   else
     {
@@ -785,6 +792,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
 	kt->digest = md_kt_get (authname);
 	kt->hmac_length = md_kt_size (kt->digest);
+
+	if (OPENVPN_MAX_HMAC_SIZE < kt->hmac_length)
+	  msg (M_FATAL, "HMAC '%s' not allowed: digest size too big.", authname);
       }
     }
   else if (!aead_cipher)
src/openvpn/crypto.h +4 -0
index 63d7040..de433ae 100644
@@ -226,6 +226,7 @@ struct key_ctx_bi
 				 *   direction. */
   struct key_ctx decrypt;       /**< cipher and/or HMAC contexts for
                                  *   receiving direction. */
+  bool initialized;
 };
 
 /**
@@ -385,6 +386,7 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
 
 /** @} name Functions for performing security operations on data channel packets */
 
+/** 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,
@@ -392,6 +394,8 @@ void crypto_adjust_frame_parameters(struct frame *frame,
 				    bool packet_id,
 				    bool packet_id_long_form);
 
+/** Return the worst-case OpenVPN crypto overhead (in bytes) */
+size_t crypto_max_overhead(void);
 
 /* Minimum length of the nonce used by the PRNG */
 #define NONCE_SECRET_LEN_MIN 16
src/openvpn/crypto_backend.h +6 -0
index 3893f14..a699673 100644
@@ -41,6 +41,12 @@
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16
 
+/* Maximum cipher block size (bytes) */
+#define OPENVPN_MAX_CIPHER_BLOCK_SIZE 32
+
+/* Maximum HMAC digest size (bytes) */
+#define OPENVPN_MAX_HMAC_SIZE 	64
+
 /** Struct used in cipher name translation table */
 typedef struct {
   const char *openvpn_name;	/**< Cipher name used by OpenVPN */
src/openvpn/init.c +22 -6
index 50cbf90..8f81b09 100644
@@ -1819,6 +1819,7 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
+    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1922,6 +1923,13 @@ do_deferred_options (struct context *c, const unsigned int found)
                        " MTU problems", TUN_MTU_SIZE(&c->c2.frame) );
 	}
     }
+
+  /* process (potenitally pushed) crypto options */
+  if (c->options.pull)
+    {
+      tls_session_update_crypto_params(&c->c2.tls_multi->session[TM_ACTIVE],
+          &c->options, &c->c2.frame);
+    }
 #endif
 }
 
@@ -2027,6 +2035,7 @@ frame_finalize_options (struct context *c, const struct options *o)
 			    |FRAME_HEADROOM_MARKER_READ_STREAM);
     }
   
+  frame_add_to_extra_buffer (&c->c2.frame, PAYLOAD_ALIGN);
   frame_finalize (&c->c2.frame,
 		  o->ce.link_mtu_defined,
 		  o->ce.link_mtu,
@@ -2282,12 +2291,18 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters */
-  crypto_adjust_frame_parameters (&c->c2.frame,
-				  &c->c1.ks.key_type,
-				  options->ciphername_defined,
-				  options->use_iv,
-				  options->replay, packet_id_long_form);
+  /* Compute MTU parameters (postpone if we pull options) */
+  if (c->options.pull)
+    {
+      /* Account for worst-case crypto overhead before allocating buffers */
+      frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
+    }
+  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);
+    }
   tls_adjust_frame_parameters (&c->c2.frame);
 
   /* Set all command-line TLS-related options */
@@ -2318,6 +2333,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
     to.push_peer_info_detail = 2;
src/openvpn/misc.h +7 -0
index 65a6e55..82d1d48 100644
@@ -102,6 +102,13 @@ openvpn_run_script (const struct argv *a, const struct env_set *es, const unsign
   return openvpn_execve_check(a, es, flags | S_SCRIPT, msg);
 }
 
+#ifndef MIN
+#define MIN(a,b) ((a) < (b) ? (a) : (b))
+#endif
+
+#ifndef MAX
+#define MAX(a,b) ((a) > (b) ? (a) : (b))
+#endif
 
 #ifdef HAVE_STRERROR
 /* a thread-safe version of strerror */
src/openvpn/mtu.c +0 -2
index 24531c9..64d1cf3 100644
@@ -78,8 +78,6 @@ frame_finalize (struct frame *frame,
     }
 
   frame->link_mtu_dynamic = frame->link_mtu;
-
-  frame->extra_buffer += PAYLOAD_ALIGN;
 }
 
 /*
src/openvpn/options.c +33 -2
index 23f407c..a595dff 100644
@@ -2974,6 +2974,38 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
 
 #ifdef ENABLE_OCC
 
+/**
+ * Calculate the link-mtu to advertise to our peer.  The actual value is not
+ * relevant, because we will possibly perform data channel cipher negotiation
+ * after this, but older clients will log warnings if we do not supply them the
+ * value they expect.  This assumes that the traditional cipher/auth directives
+ * in the config match the config of the peer.
+ */
+static size_t
+calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
+{
+  size_t link_mtu = EXPANDED_SIZE (frame);
+#ifdef ENABLE_CRYPTO
+  if (o->pull || o->mode == MODE_SERVER)
+    {
+      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);
+      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));
+      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,
+	  EXPANDED_SIZE (&fake_frame));
+      link_mtu = EXPANDED_SIZE (&fake_frame);
+    }
+#endif
+  return link_mtu;
+}
+
 /*
  * Build an options string to represent data channel encryption options.
  * This string must match exactly between peers.  The keysize is checked
@@ -3018,7 +3050,6 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
  * --tls-server [matched with --tls-client on
  *               the other end of the connection]
  */
-
 char *
 options_string (const struct options *o,
 		const struct frame *frame,
@@ -3036,7 +3067,7 @@ options_string (const struct options *o,
    */
 
   buf_printf (&out, ",dev-type %s", dev_type_string (o->dev, o->dev_type));
-  buf_printf (&out, ",link-mtu %d", EXPANDED_SIZE (frame));
+  buf_printf (&out, ",link-mtu %zu", calc_options_string_link_mtu(o, frame));
   buf_printf (&out, ",tun-mtu %d", PAYLOAD_SIZE (frame));
   buf_printf (&out, ",proto %s",  proto_remote (o->ce.proto, remote));
 
src/openvpn/route.c +1 -3
index a90195f..578a708 100644
@@ -3265,8 +3265,6 @@ struct rtmsg {
 #define ADVANCE(x, n) (x += ROUNDUP((n)->sa_len))
 #endif
 
-#define max(a,b) ((a) > (b) ? (a) : (b))
-
 void
 get_default_gateway (struct route_gateway_info *rgi)
 {
@@ -3433,7 +3431,7 @@ get_default_gateway (struct route_gateway_info *rgi)
 #if defined(TARGET_SOLARIS)
 	  const size_t len = sizeof(ifr->ifr_name) + sizeof(ifr->ifr_addr);
 #else
-	  const size_t len = sizeof(ifr->ifr_name) + max(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
+	  const size_t len = sizeof(ifr->ifr_name) + MAX(sizeof(ifr->ifr_addr), ifr->ifr_addr.sa_len);
 #endif
 
 	  if (!ifr->ifr_addr.sa_family)
src/openvpn/ssl.c +62 -7

118 lines not on this page · raw message

src/openvpn/ssl.h +14 -0
index d9ff8d0..416f426 100644
@@ -475,6 +475,20 @@ bool tls_rec_payload (struct tls_multi *multi,
 void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
+/**
+ * Update TLS session crypto parameters (cipher and auth) and derive data
+ * channel keys based on the supplied options.
+ *
+ * @param session	The TLS session to update.
+ * @param options	The options to use when updating session.
+ * @param frame		The frame options for this session (frame overhead is
+ * 			adjusted based on the selected cipher/auth).
+ *
+ * @return true if updating succeeded, false otherwise.
+ */
+bool tls_session_update_crypto_params(struct tls_session *session,
+    const struct options *options, struct frame *frame);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
src/openvpn/ssl_common.h +14 -2
index a0df0ff..9183dab 100644
@@ -149,7 +149,12 @@ struct key_source2 {
 struct key_state
 {
   int state;
-  int key_id;			/* inherited from struct tls_session below */
+
+  /**
+   * Key id for this key_state,  inherited from struct tls_session.
+   * @see tls_session::key_id.
+   */
+  int key_id;
 
   struct key_state_ssl ks_ssl;	/* contains SSL object and BIOs for the control channel */
 
@@ -231,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
 #endif
@@ -367,7 +373,13 @@ struct tls_session
 
   int initial_opcode;		/* our initial P_ opcode */
   struct session_id session_id;	/* our random session ID */
-  int key_id;			/* increments with each soft reset (for key renegotiation) */
+
+  /**
+   * The current active key id, used to keep track of renegotiations.
+   * key_id increments with each soft reset to KEY_ID_MASK then recycles back
+   * to 1.  This way you know that if key_id is 0, it is the first key.
+   */
+  int key_id;
 
   int limit_next;               /* used for traffic shaping on the control channel */
 
Signature · 1 line
2.7.4
#11 lore raw

PATCH v3 3/5 Add client-side support for cipher negotiation

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negotiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

v2:
* Account for crypto overhead through struct frame. This is less transparant, but the code has been built to work this way. The previous approach didn't work with TCP mode (or --port-share).
* Calculate the link-mtu sent in the options string based on the crypto parameters specified in the config file (prevents link-mtu warnings in older peers when connecting).

v3:
* Use existing max_int() function, instead of new MAX() macro.
* Fix typo in comment.
* Do not regenerate keys if the server sends a second push msg.
* Only advertise IV_NCP=2 if we're a pull-client (and thus can do NCP).

---
 src/openvpn/crypto.c         | 19 +++++++---
 src/openvpn/crypto.h         |  4 +++
 src/openvpn/crypto_backend.h |  6 ++++
 src/openvpn/init.c           | 35 ++++++++++++++----
 src/openvpn/mtu.c            |  2 --
 src/openvpn/options.c        | 35 ++++++++++++++++--
 src/openvpn/ssl.c            | 84 ++++++++++++++++++++++++++++++++++++++++----
 src/openvpn/ssl.h            | 20 +++++++++++
 src/openvpn/ssl_common.h     | 16 +++++++--
 9 files changed, 198 insertions(+), 23 deletions(-)
src/openvpn/crypto.c +15 -4
index 5c392aa..f9cf62a 100644
@@ -35,6 +35,7 @@
 
 #include "crypto.h"
 #include "error.h"
+#include "integer.h"
 #include "misc.h"
 
 #include "memdbg.h"
@@ -709,10 +710,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
   return ret;
 }
 
-/*
- * How many bytes will we add to frame buffer for a given
- * set of crypto options?
- */
 void
 crypto_adjust_frame_parameters(struct frame *frame,
 			       const struct key_type* kt,
@@ -746,6 +743,14 @@ crypto_adjust_frame_parameters(struct frame *frame,
       __func__, crypto_overhead);
 }
 
+size_t
+crypto_max_overhead(void)
+{
+  return packet_id_size(true) + OPENVPN_MAX_IV_LENGTH +
+      OPENVPN_MAX_CIPHER_BLOCK_SIZE +
+      max_int (OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
+}
+
 /*
  * Build a struct key_type.
  */
@@ -774,6 +779,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
 #endif
 	    ))
 	msg (M_FATAL, "Cipher '%s' mode not supported", ciphername);
+
+      if (OPENVPN_MAX_CIPHER_BLOCK_SIZE < cipher_kt_block_size(kt->cipher))
+	msg (M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
     }
   else
     {
@@ -785,6 +793,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
 	kt->digest = md_kt_get (authname);
 	kt->hmac_length = md_kt_size (kt->digest);
+
+	if (OPENVPN_MAX_HMAC_SIZE < kt->hmac_length)
+	  msg (M_FATAL, "HMAC '%s' not allowed: digest size too big.", authname);
       }
     }
   else if (!aead_cipher)
src/openvpn/crypto.h +4 -0
index 63d7040..de433ae 100644
@@ -226,6 +226,7 @@ struct key_ctx_bi
 				 *   direction. */
   struct key_ctx decrypt;       /**< cipher and/or HMAC contexts for
                                  *   receiving direction. */
+  bool initialized;
 };
 
 /**
@@ -385,6 +386,7 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
 
 /** @} name Functions for performing security operations on data channel packets */
 
+/** 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,
@@ -392,6 +394,8 @@ void crypto_adjust_frame_parameters(struct frame *frame,
 				    bool packet_id,
 				    bool packet_id_long_form);
 
+/** Return the worst-case OpenVPN crypto overhead (in bytes) */
+size_t crypto_max_overhead(void);
 
 /* Minimum length of the nonce used by the PRNG */
 #define NONCE_SECRET_LEN_MIN 16
src/openvpn/crypto_backend.h +6 -0
index 3893f14..a699673 100644
@@ -41,6 +41,12 @@
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16
 
+/* Maximum cipher block size (bytes) */
+#define OPENVPN_MAX_CIPHER_BLOCK_SIZE 32
+
+/* Maximum HMAC digest size (bytes) */
+#define OPENVPN_MAX_HMAC_SIZE 	64
+
 /** Struct used in cipher name translation table */
 typedef struct {
   const char *openvpn_name;	/**< Cipher name used by OpenVPN */
src/openvpn/init.c +29 -6
index 50cbf90..70c74a3 100644
@@ -1819,6 +1819,7 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
+    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1922,6 +1923,20 @@ do_deferred_options (struct context *c, const unsigned int found)
                        " MTU problems", TUN_MTU_SIZE(&c->c2.frame) );
 	}
     }
+
+  /* process (potentially pushed) crypto options */
+  if (c->options.pull)
+    {
+      struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+      if (found & OPT_P_NCP)
+	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+      /* Do not regenerate keys if server sends an extra push request */
+      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+	{
+	  tls_session_update_crypto_params(&c->c2.tls_multi->session[TM_ACTIVE],
+	      &c->options, &c->c2.frame);
+	}
+    }
 #endif
 }
 
@@ -2027,6 +2042,7 @@ frame_finalize_options (struct context *c, const struct options *o)
 			    |FRAME_HEADROOM_MARKER_READ_STREAM);
     }
   
+  frame_add_to_extra_buffer (&c->c2.frame, PAYLOAD_ALIGN);
   frame_finalize (&c->c2.frame,
 		  o->ce.link_mtu_defined,
 		  o->ce.link_mtu,
@@ -2282,12 +2298,18 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters */
-  crypto_adjust_frame_parameters (&c->c2.frame,
-				  &c->c1.ks.key_type,
-				  options->ciphername_defined,
-				  options->use_iv,
-				  options->replay, packet_id_long_form);
+  /* Compute MTU parameters (postpone if we pull options) */
+  if (c->options.pull)
+    {
+      /* Account for worst-case crypto overhead before allocating buffers */
+      frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
+    }
+  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);
+    }
   tls_adjust_frame_parameters (&c->c2.frame);
 
   /* Set all command-line TLS-related options */
@@ -2318,6 +2340,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
     to.push_peer_info_detail = 2;
src/openvpn/mtu.c +0 -2
index 24531c9..64d1cf3 100644
@@ -78,8 +78,6 @@ frame_finalize (struct frame *frame,
     }
 
   frame->link_mtu_dynamic = frame->link_mtu;
-
-  frame->extra_buffer += PAYLOAD_ALIGN;
 }
 
 /*
src/openvpn/options.c +33 -2
index 23f407c..a595dff 100644
@@ -2974,6 +2974,38 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
 
 #ifdef ENABLE_OCC
 
+/**
+ * Calculate the link-mtu to advertise to our peer.  The actual value is not
+ * relevant, because we will possibly perform data channel cipher negotiation
+ * after this, but older clients will log warnings if we do not supply them the
+ * value they expect.  This assumes that the traditional cipher/auth directives
+ * in the config match the config of the peer.
+ */
+static size_t
+calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
+{
+  size_t link_mtu = EXPANDED_SIZE (frame);
+#ifdef ENABLE_CRYPTO
+  if (o->pull || o->mode == MODE_SERVER)
+    {
+      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);
+      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));
+      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,
+	  EXPANDED_SIZE (&fake_frame));
+      link_mtu = EXPANDED_SIZE (&fake_frame);
+    }
+#endif
+  return link_mtu;
+}
+
 /*
  * Build an options string to represent data channel encryption options.
  * This string must match exactly between peers.  The keysize is checked
@@ -3018,7 +3050,6 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
  * --tls-server [matched with --tls-client on
  *               the other end of the connection]
  */
-
 char *
 options_string (const struct options *o,
 		const struct frame *frame,
@@ -3036,7 +3067,7 @@ options_string (const struct options *o,
    */
 
   buf_printf (&out, ",dev-type %s", dev_type_string (o->dev, o->dev_type));
-  buf_printf (&out, ",link-mtu %d", EXPANDED_SIZE (frame));
+  buf_printf (&out, ",link-mtu %zu", calc_options_string_link_mtu(o, frame));
   buf_printf (&out, ",tun-mtu %d", PAYLOAD_SIZE (frame));
   buf_printf (&out, ",proto %s",  proto_remote (o->ce.proto, remote));
 
src/openvpn/ssl.c +77 -7

139 lines not on this page · raw message

src/openvpn/ssl.h +20 -0
index d9ff8d0..de68b69 100644
@@ -475,6 +475,20 @@ bool tls_rec_payload (struct tls_multi *multi,
 void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
+/**
+ * Update TLS session crypto parameters (cipher and auth) and derive data
+ * channel keys based on the supplied options.
+ *
+ * @param session	The TLS session to update.
+ * @param options	The options to use when updating session.
+ * @param frame		The frame options for this session (frame overhead is
+ * 			adjusted based on the selected cipher/auth).
+ *
+ * @return true if updating succeeded, false otherwise.
+ */
+bool tls_session_update_crypto_params(struct tls_session *session,
+    const struct options *options, struct frame *frame);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
@@ -483,6 +497,12 @@ tls_get_peer_info(const struct tls_multi *multi)
 }
 #endif
 
+/**
+ * Return the Negotiable Crypto Parameters version advertised in the peer info
+ * string, or 0 if none specified.
+ */
+int tls_peer_info_ncp_ver(const char *peer_info);
+
 /*
  * inline functions
  */
src/openvpn/ssl_common.h +14 -2
index a0df0ff..9183dab 100644
@@ -149,7 +149,12 @@ struct key_source2 {
 struct key_state
 {
   int state;
-  int key_id;			/* inherited from struct tls_session below */
+
+  /**
+   * Key id for this key_state,  inherited from struct tls_session.
+   * @see tls_session::key_id.
+   */
+  int key_id;
 
   struct key_state_ssl ks_ssl;	/* contains SSL object and BIOs for the control channel */
 
@@ -231,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
 #endif
@@ -367,7 +373,13 @@ struct tls_session
 
   int initial_opcode;		/* our initial P_ opcode */
   struct session_id session_id;	/* our random session ID */
-  int key_id;			/* increments with each soft reset (for key renegotiation) */
+
+  /**
+   * The current active key id, used to keep track of renegotiations.
+   * key_id increments with each soft reset to KEY_ID_MASK then recycles back
+   * to 1.  This way you know that if key_id is 0, it is the first key.
+   */
+  int key_id;
 
   int limit_next;               /* used for traffic shaping on the control channel */
 
Signature · 1 line
2.7.4
#12 lore raw

Re: [Openvpn-devel] [PATCHv3 3/5] Add client-side support for cipher negotiation

Hi,

Quoted 15 lines · Steffan Karger

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negotiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

v2:
* Account for crypto overhead through struct frame. This is less transparant, but the code has been built to work this way. The previous approach didn't work with TCP mode (or --port-share).
* Calculate the link-mtu sent in the options string based on the crypto parameters specified in the config file (prevents link-mtu warnings in older peers when connecting).

Quoted 5 lines

v3:
* Use existing max_int() function, instead of new MAX() macro.
* Fix typo in comment.
* Do not regenerate keys if the server sends a second push msg.
* Only advertise IV_NCP=2 if we're a pull-client (and thus can do NCP).

There might be surprises lurking here regarding explicitly configured --link-mtu, so this will need detailed testing in as many possible option variants as possible.

"Logic wise" this looks reasonable to me, so I'm willing to give an ACK to v4

"Memory wise", to address James' concerns, I do not see anything obvious where this would lead to memory leaks, given that it's mostly just moving code around that is run once before and afterwards. I think we're going to waste a handful of bytes on the frame overhead (like, "100"-ish) and resulting maximum buffer size, but that seems impossible to avoid without much more intrusive changes.

One small thing I did not like, but that's "style":

src/openvpn/ssl.c key_state_init (struct tls_session *session, struct key_state *ks) · line 772

Quoted 7 lines
src/openvpn/ssl.c +0 -4
-  /*
-   * key_id increments to KEY_ID_MASK then recycles back to 1.
-   * This way you know that if key_id is 0, it is the first key.
-   */
   ++session->key_id;
   session->key_id &= P_KEY_ID_MASK;
   if (!session->key_id)

I think we should leave this comment in - I saw you've added a more detailed comment to the associated .h file, but poor folks like me, without a decent "explain in a second window what the docs say about the line of code I'm staring at" IDE might benefit from this comment...

Quoted 4 lines
src/openvpn/ssl.c +4 -0
+int
+tls_peer_info_ncp_ver(const char *peer_info)
+{
+  const char *ncpstr = peer_info ? strstr (peer_info, "IV_NCP=") : NULL;

src/openvpn/ssl.c tls_update_remote_addr (struct tls_multi *multi, const struct link_socket_actual · line 3672

Quoted 10 lines
src/openvpn/ssl.c +10 -0
+  if (ncpstr)
+    {
+      int ncp = 0;
+      int r = sscanf(ncpstr, "IV_NCP=%d", &ncp);
+      if (r == 1)
+	return ncp;
+    }
+  return 0;
+}
+

This one should better go to 5/5 :-) - but you know that already.

thanks,

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]...
#13 lore raw

PATCH v4 3/5 Add client-side support for cipher negotiation

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negotiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

v2:
* Account for crypto overhead through struct frame. This is less transparant, but the code has been built to work this way. The previous approach didn't work with TCP mode (or --port-share).
* Calculate the link-mtu sent in the options string based on the crypto parameters specified in the config file (prevents link-mtu warnings in older peers when connecting).

v3:
* Use existing max_int() function, instead of new MAX() macro.
* Fix typo in comment.
* Do not regenerate keys if the server sends a second push msg
* Only push IV_NCP if we're pull-client (and thus can do NCP)

v4:
* Fix rebase errors (OPT_P_NCP sneaked in, but is not introduced till 4/5, and tls_peer_info_ncp_ver() is not needed until 5/5).
* Don't remove comment about key_id increment behaviour in init.c (but still add the extra comments in the .h files).

---
 src/openvpn/crypto.c         | 19 ++++++++++---
 src/openvpn/crypto.h         |  4 +++
 src/openvpn/crypto_backend.h |  6 ++++
 src/openvpn/init.c           | 34 +++++++++++++++++++----
 src/openvpn/mtu.c            |  2 --
 src/openvpn/options.c        | 35 +++++++++++++++++++++--
 src/openvpn/ssl.c            | 66 ++++++++++++++++++++++++++++++++++++++++++--
 src/openvpn/ssl.h            | 14 ++++++++++
 src/openvpn/ssl_common.h     | 16 +++++++++--
 9 files changed, 177 insertions(+), 19 deletions(-)
src/openvpn/crypto.c +15 -4
index 5c392aa..f9cf62a 100644
@@ -35,6 +35,7 @@
 
 #include "crypto.h"
 #include "error.h"
+#include "integer.h"
 #include "misc.h"
 
 #include "memdbg.h"
@@ -709,10 +710,6 @@ openvpn_decrypt (struct buffer *buf, struct buffer work,
   return ret;
 }
 
-/*
- * How many bytes will we add to frame buffer for a given
- * set of crypto options?
- */
 void
 crypto_adjust_frame_parameters(struct frame *frame,
 			       const struct key_type* kt,
@@ -746,6 +743,14 @@ crypto_adjust_frame_parameters(struct frame *frame,
       __func__, crypto_overhead);
 }
 
+size_t
+crypto_max_overhead(void)
+{
+  return packet_id_size(true) + OPENVPN_MAX_IV_LENGTH +
+      OPENVPN_MAX_CIPHER_BLOCK_SIZE +
+      max_int (OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
+}
+
 /*
  * Build a struct key_type.
  */
@@ -774,6 +779,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
 #endif
 	    ))
 	msg (M_FATAL, "Cipher '%s' mode not supported", ciphername);
+
+      if (OPENVPN_MAX_CIPHER_BLOCK_SIZE < cipher_kt_block_size(kt->cipher))
+	msg (M_FATAL, "Cipher '%s' not allowed: block size too big.", ciphername);
     }
   else
     {
@@ -785,6 +793,9 @@ init_key_type (struct key_type *kt, const char *ciphername,
       if (!aead_cipher) { /* Ignore auth for AEAD ciphers */
 	kt->digest = md_kt_get (authname);
 	kt->hmac_length = md_kt_size (kt->digest);
+
+	if (OPENVPN_MAX_HMAC_SIZE < kt->hmac_length)
+	  msg (M_FATAL, "HMAC '%s' not allowed: digest size too big.", authname);
       }
     }
   else if (!aead_cipher)
src/openvpn/crypto.h +4 -0
index 63d7040..de433ae 100644
@@ -226,6 +226,7 @@ struct key_ctx_bi
 				 *   direction. */
   struct key_ctx decrypt;       /**< cipher and/or HMAC contexts for
                                  *   receiving direction. */
+  bool initialized;
 };
 
 /**
@@ -385,6 +386,7 @@ bool openvpn_decrypt (struct buffer *buf, struct buffer work,
 
 /** @} name Functions for performing security operations on data channel packets */
 
+/** 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,
@@ -392,6 +394,8 @@ void crypto_adjust_frame_parameters(struct frame *frame,
 				    bool packet_id,
 				    bool packet_id_long_form);
 
+/** Return the worst-case OpenVPN crypto overhead (in bytes) */
+size_t crypto_max_overhead(void);
 
 /* Minimum length of the nonce used by the PRNG */
 #define NONCE_SECRET_LEN_MIN 16
src/openvpn/crypto_backend.h +6 -0
index 3893f14..a699673 100644
@@ -41,6 +41,12 @@
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16
 
+/* Maximum cipher block size (bytes) */
+#define OPENVPN_MAX_CIPHER_BLOCK_SIZE 32
+
+/* Maximum HMAC digest size (bytes) */
+#define OPENVPN_MAX_HMAC_SIZE 	64
+
 /** Struct used in cipher name translation table */
 typedef struct {
   const char *openvpn_name;	/**< Cipher name used by OpenVPN */
src/openvpn/init.c +28 -6
index 50cbf90..2f4c631 100644
@@ -1819,6 +1819,7 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
+    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1922,6 +1923,19 @@ do_deferred_options (struct context *c, const unsigned int found)
                        " MTU problems", TUN_MTU_SIZE(&c->c2.frame) );
 	}
     }
+
+  /* process (potentially pushed) crypto options */
+  if (c->options.pull)
+    {
+      struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+      if (found & OPT_P_CRYPTO)
+	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+      /* Do not regenerate keys if server sends an extra push request */
+      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+	{
+	  tls_session_update_crypto_params(session, &c->options, &c->c2.frame);
+	}
+    }
 #endif
 }
 
@@ -2027,6 +2041,7 @@ frame_finalize_options (struct context *c, const struct options *o)
 			    |FRAME_HEADROOM_MARKER_READ_STREAM);
     }
   
+  frame_add_to_extra_buffer (&c->c2.frame, PAYLOAD_ALIGN);
   frame_finalize (&c->c2.frame,
 		  o->ce.link_mtu_defined,
 		  o->ce.link_mtu,
@@ -2282,12 +2297,18 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters */
-  crypto_adjust_frame_parameters (&c->c2.frame,
-				  &c->c1.ks.key_type,
-				  options->ciphername_defined,
-				  options->use_iv,
-				  options->replay, packet_id_long_form);
+  /* Compute MTU parameters (postpone if we pull options) */
+  if (c->options.pull)
+    {
+      /* Account for worst-case crypto overhead before allocating buffers */
+      frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
+    }
+  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);
+    }
   tls_adjust_frame_parameters (&c->c2.frame);
 
   /* Set all command-line TLS-related options */
@@ -2318,6 +2339,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
     to.push_peer_info_detail = 2;
src/openvpn/mtu.c +0 -2
index 24531c9..64d1cf3 100644
@@ -78,8 +78,6 @@ frame_finalize (struct frame *frame,
     }
 
   frame->link_mtu_dynamic = frame->link_mtu;
-
-  frame->extra_buffer += PAYLOAD_ALIGN;
 }
 
 /*
src/openvpn/options.c +33 -2
index 23f407c..a595dff 100644
@@ -2974,6 +2974,38 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
 
 #ifdef ENABLE_OCC
 
+/**
+ * Calculate the link-mtu to advertise to our peer.  The actual value is not
+ * relevant, because we will possibly perform data channel cipher negotiation
+ * after this, but older clients will log warnings if we do not supply them the
+ * value they expect.  This assumes that the traditional cipher/auth directives
+ * in the config match the config of the peer.
+ */
+static size_t
+calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
+{
+  size_t link_mtu = EXPANDED_SIZE (frame);
+#ifdef ENABLE_CRYPTO
+  if (o->pull || o->mode == MODE_SERVER)
+    {
+      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);
+      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));
+      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,
+	  EXPANDED_SIZE (&fake_frame));
+      link_mtu = EXPANDED_SIZE (&fake_frame);
+    }
+#endif
+  return link_mtu;
+}
+
 /*
  * Build an options string to represent data channel encryption options.
  * This string must match exactly between peers.  The keysize is checked
@@ -3018,7 +3050,6 @@ pre_pull_restore (struct options *o, struct gc_arena *gc)
  * --tls-server [matched with --tls-client on
  *               the other end of the connection]
  */
-
 char *
 options_string (const struct options *o,
 		const struct frame *frame,
@@ -3036,7 +3067,7 @@ options_string (const struct options *o,
    */
 
   buf_printf (&out, ",dev-type %s", dev_type_string (o->dev, o->dev_type));
-  buf_printf (&out, ",link-mtu %d", EXPANDED_SIZE (frame));
+  buf_printf (&out, ",link-mtu %zu", calc_options_string_link_mtu(o, frame));
   buf_printf (&out, ",tun-mtu %d", PAYLOAD_SIZE (frame));
   buf_printf (&out, ",proto %s",  proto_remote (o->ce.proto, remote));
 
src/openvpn/ssl.c +63 -3

109 lines not on this page · raw message

src/openvpn/ssl.h +14 -0
index d9ff8d0..416f426 100644
@@ -475,6 +475,20 @@ bool tls_rec_payload (struct tls_multi *multi,
 void tls_update_remote_addr (struct tls_multi *multi,
 			     const struct link_socket_actual *addr);
 
+/**
+ * Update TLS session crypto parameters (cipher and auth) and derive data
+ * channel keys based on the supplied options.
+ *
+ * @param session	The TLS session to update.
+ * @param options	The options to use when updating session.
+ * @param frame		The frame options for this session (frame overhead is
+ * 			adjusted based on the selected cipher/auth).
+ *
+ * @return true if updating succeeded, false otherwise.
+ */
+bool tls_session_update_crypto_params(struct tls_session *session,
+    const struct options *options, struct frame *frame);
+
 #ifdef MANAGEMENT_DEF_AUTH
 static inline char *
 tls_get_peer_info(const struct tls_multi *multi)
src/openvpn/ssl_common.h +14 -2
index a0df0ff..9183dab 100644
@@ -149,7 +149,12 @@ struct key_source2 {
 struct key_state
 {
   int state;
-  int key_id;			/* inherited from struct tls_session below */
+
+  /**
+   * Key id for this key_state,  inherited from struct tls_session.
+   * @see tls_session::key_id.
+   */
+  int key_id;
 
   struct key_state_ssl ks_ssl;	/* contains SSL object and BIOs for the control channel */
 
@@ -231,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
 #endif
@@ -367,7 +373,13 @@ struct tls_session
 
   int initial_opcode;		/* our initial P_ opcode */
   struct session_id session_id;	/* our random session ID */
-  int key_id;			/* increments with each soft reset (for key renegotiation) */
+
+  /**
+   * The current active key id, used to keep track of renegotiations.
+   * key_id increments with each soft reset to KEY_ID_MASK then recycles back
+   * to 1.  This way you know that if key_id is 0, it is the first key.
+   */
+  int key_id;
 
   int limit_next;               /* used for traffic shaping on the control channel */
 
Signature · 1 line
2.7.4
#14 lore raw

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

Another ACK from me. Stared at the code for another while, still looks reasonable. Passes my t_client tests, t_server tests, and does not break Android :-)

Your patch has been applied to the master branch.

commit 97894360fa537945e07fd6a85d0659e094b693a5

Author: Steffan Karger
Date:   Tue Jun 28 23:33:55 2016 +0200
     Add client-side support for cipher negotiation

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/12007
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#15 lore raw

Re: [Openvpn-devel] [PATCHv3 3/5] Add client-side support for cipher negotiation

In reply to Gert Doering

Am 23.06.16 um 20:32 schrieb Gert Doering:

Quoted 30 lines

Hi,

Quoted 15 lines · Steffan Karger

Based on the 'IV_NCP=2' mechanism described in
http://permalink.gmane.org/gmane.network.openvpn.devel/9385.

This is the first patch of a set that adds support for cipher negotiation. Follow-up patches will add ways to restrict or disable the mechanism, and add server-side support.

v2:
* Account for crypto overhead through struct frame. This is less transparant, but the code has been built to work this way. The previous approach didn't work with TCP mode (or --port-share).
* Calculate the link-mtu sent in the options string based on the crypto parameters specified in the config file (prevents link-mtu warnings in older peers when connecting).

Quoted 5 lines

v3:
* Use existing max_int() function, instead of new MAX() macro.
* Fix typo in comment.
* Do not regenerate keys if the server sends a second push msg.
* Only advertise IV_NCP=2 if we're a pull-client (and thus can do NCP).

There might be surprises lurking here regarding explicitly configured --link-mtu, so this will need detailed testing in as many possible option variants as possible.

"Logic wise" this looks reasonable to me, so I'm willing to give an ACK to v4

Quoted 8 lines

"Memory wise", to address James' concerns, I do not see anything obvious where this would lead to memory leaks, given that it's mostly just moving code around that is run once before and afterwards. I think we're going to waste a handful of bytes on the frame overhead (like, "100"-ish) and resulting maximum buffer size, but that seems impossible to avoid without much more intrusive changes.

ACK from me aswell. The few extra kilobyts of memory should really hurt in my opinion as well.

Arne

#16 lore raw

PATCH 4/5 Add options to restrict cipher negotiation

In reply to Steffan Karger

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

---
 src/openvpn/init.c       | 34 ++++++++++++++++++++++++++++------
 src/openvpn/init.h       |  4 ++--
 src/openvpn/openvpn.h    |  3 +++
 src/openvpn/options.c    | 38 +++++++++++++++++++++++++++-----------
 src/openvpn/options.h    |  4 +++-
 src/openvpn/push.c       | 12 +++++++++++-
 src/openvpn/ssl.c        | 30 +++++++++++++++++++++++++++++-
 src/openvpn/ssl_common.h |  4 ++++
 8 files changed, 107 insertions(+), 22 deletions(-)
src/openvpn/init.c +28 -6
index 5eded09..af61045 100644
@@ -1739,7 +1739,7 @@ options_hash_changed_or_zero(const struct md5_digest *a,
 }
 #endif /* P2MP */
 
-void
+bool
 do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 {
   if (!c->c2.do_up_ran)
@@ -1747,7 +1747,13 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
       reset_coarse_timers (c);
 
       if (pulled_options && option_types_found)
-	do_deferred_options (c, option_types_found);
+	{
+	  if (!do_deferred_options (c, option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "ERROR: Failed to apply push options");
+	      return false;
+	    }
+	}
 
       /* if --up-delay specified, open tun, do ifconfig, and run up script now */
       if (c->options.up_delay || PULL_DEFINED (&c->options))
@@ -1802,6 +1808,7 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 	
       c->c2.do_up_ran = true;
     }
+  return true;
 }
 
 /*
@@ -1819,7 +1826,6 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
-    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1830,13 +1836,16 @@ pull_permission_mask (const struct context *c)
   if (!c->options.route_nopull)
     flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
 
+  if (c->options.ncp_enabled)
+    flags |= OPT_P_NCP;
+
   return flags;
 }
 
 /*
  * Handle non-tun-related pulled options.
  */
-void
+bool
 do_deferred_options (struct context *c, const unsigned int found)
 {
   if (found & OPT_P_MESSAGES)
@@ -1927,10 +1936,17 @@ do_deferred_options (struct context *c, const unsigned int found)
   /* process (potenitally pushed) crypto options */
   if (c->options.pull)
     {
-      tls_session_update_crypto_params(&c->c2.tls_multi->session[TM_ACTIVE],
-          &c->options, &c->c2.frame);
+      if (found & OPT_P_NCP)
+	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+      if (!tls_session_update_crypto_params(
+	  &c->c2.tls_multi->session[TM_ACTIVE], &c->options, &c->c2.frame))
+	{
+	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
+	  return false;
+	}
     }
 #endif
+  return true;
 }
 
 /*
@@ -2250,6 +2266,9 @@ do_init_crypto_tls_c1 (struct context *c)
 	      &c->c1.ks.tls_auth_key, file, options->key_direction, flags);
 	}
 
+      c->c1.ciphername = options->ciphername;
+      c->c1.authname = options->authname;
+
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
       if (options->priv_key_file_inline)
 	{
@@ -2321,6 +2340,9 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.replay_window = options->replay_window;
   to.replay_time = options->replay_time;
   to.tcp_mode = link_socket_proto_connection_oriented (options->ce.proto);
+  to.config_ciphername = c->c1.ciphername;
+  to.config_authname = c->c1.authname;
+  to.ncp_enabled = options->ncp_enabled;
   to.transition_window = options->transition_window;
   to.handshake_window = options->handshake_window;
   to.packet_timeout = options->tls_timeout;
src/openvpn/init.h +2 -2
index 73bcb74..1ad4a8e 100644
@@ -81,7 +81,7 @@ bool do_test_crypto (const struct options *o);
 
 void context_gc_free (struct context *c);
 
-void do_up (struct context *c,
+bool do_up (struct context *c,
 	    bool pulled_options,
 	    unsigned int option_types_found);
 
@@ -91,7 +91,7 @@ const char *format_common_name (struct context *c, struct gc_arena *gc);
 
 void reset_coarse_timers (struct context *c);
 
-void do_deferred_options (struct context *c, const unsigned int found);
+bool do_deferred_options (struct context *c, const unsigned int found);
 
 void inherit_context_child (struct context *dest,
 			    const struct context *src);
src/openvpn/openvpn.h +3 -0
index 3281fd7..395195f 100644
@@ -210,6 +210,9 @@ struct context_1
   struct user_pass *auth_user_pass;
                                 /**< Username and password for
                                  *   authentication. */
+
+  const char *ciphername;	/**< Data channel cipher from config file */
+  const char *authname;		/**< Data channel auth from config file */
 #endif
 };
 
src/openvpn/options.c +27 -11
index 23f407c..87255d0 100644
@@ -830,6 +830,12 @@ init_options (struct options *o, const bool init_gc)
 #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
+  o->ncp_enabled = false;
+#endif
+  o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
   o->authname = "SHA1";
   o->authname_defined = true;
   o->prng_hash = "SHA1";
@@ -6601,7 +6607,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
       options->authname = p[1];
       if (streq (options->authname, "none"))
@@ -6612,12 +6618,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       options->ciphername_defined = true;
       options->ciphername = p[1];
       if (streq (options->ciphername, "none"))
@@ -6628,12 +6634,22 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "cipher") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      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);
+      options->ncp_ciphers = p[1];
+    }
+  else if (streq (p[0], "ncp-disable") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->ncp_enabled = false;
+    }
   else if (streq (p[0], "prng") && p[1] && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (streq (p[1], "none"))
 	options->prng_hash = NULL;
       else
@@ -6655,12 +6671,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "no-replay") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->replay = false;
     }
   else if (streq (p[0], "replay-window") && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (p[1])
 	{
 	  int replay_window;
@@ -6700,12 +6716,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "mute-replay-warnings") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->mute_replay_warnings = true;
     }
   else if (streq (p[0], "no-iv") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->use_iv = false;
     }
   else if (streq (p[0], "replay-persist") && p[1] && !p[2])
@@ -6735,7 +6751,7 @@ add_option (struct options *options,
     {
       int keysize;
 
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       keysize = atoi (p[1]) / 8;
       if (keysize < 0 || keysize > MAX_CIPHER_KEY_LENGTH)
 	{
@@ -6764,7 +6780,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "ecdh-curve") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->ecdh_curve= p[1];
     }
   else if (streq (p[0], "tls-server") && !p[1])
src/openvpn/options.h +3 -1
index 514511b..ea63c4c 100644
@@ -474,6 +474,8 @@ struct options
   int key_direction;
   bool ciphername_defined;
   const char *ciphername;
+  bool ncp_enabled;
+  const char *ncp_ciphers;
   bool authname_defined;
   const char *authname;
   int keysize;
@@ -618,7 +620,7 @@ struct options
 #define OPT_P_PERSIST_IP      (1<<9)
 #define OPT_P_COMP            (1<<10) /* TODO */
 #define OPT_P_MESSAGES        (1<<11)
-#define OPT_P_CRYPTO          (1<<12) /* TODO */
+#define OPT_P_NCP             (1<<12) /**< Negotiable crypto parameters */
 #define OPT_P_TLS_PARMS       (1<<13) /* TODO */
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
src/openvpn/push.c +11 -1
index 38ac59e..5324676 100644
@@ -239,11 +239,21 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from remote */
       if (status == PUSH_MSG_REPLY)
-	do_up (c, true, c->options.push_option_types_found ); /* delay bringing tun/tap up until --push parms received from remote */
+	{
+	  if (!do_up (c, true, c->options.push_option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+	      c->sig->signal_received = SIGUSR1; /* connection reset */
+	      c->sig->signal_text = "open-tuntap-failed";
+	      goto cleanup;
+	    }
+	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
 
+cleanup:
   gc_free (&gc);
 }
 
src/openvpn/ssl.c +29 -1
index 8a87db2..3da254d 100644
@@ -1636,6 +1636,24 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
+static bool
+item_in_list(const char *item, const char *list)
+{
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while(token)
+    {
+      if (0 == strcmp (token, item))
+	break;
+      token = strtok (NULL, ":");
+    }
+  free(tmp_ciphers_orig);
+
+  return token != NULL;
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)
@@ -1646,6 +1664,15 @@ tls_session_update_crypto_params(struct tls_session *session,
   ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
+  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+      !item_in_list(options->ciphername, options->ncp_ciphers))
+    {
+      msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
+	  options->ciphername, session->opt->config_ciphername,
+	  options->ncp_ciphers);
+      return false;
+    }
+
   init_key_type (&session->opt->key_type, options->ciphername,
     options->ciphername_defined, options->authname, options->authname_defined,
     options->keysize, true, true);
@@ -1925,7 +1952,8 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
       buf_printf(&out, "IV_PROTO=2\n");
 
       /* support for Negotiable Crypto Paramters */
-      buf_printf(&out, "IV_NCP=2\n");
+      if (session->opt->ncp_enabled)
+	buf_printf(&out, "IV_NCP=2\n");
 
       /* push compression status */
 #ifdef USE_COMP
src/openvpn/ssl_common.h +4 -0
index 9183dab..6bfde6b 100644
@@ -273,6 +273,10 @@ struct tls_options
   int replay_time;                     /* --replay-window parm */
   bool tcp_mode;
 
+  const char *config_ciphername;
+  const char *config_authname;
+  bool ncp_enabled;
+
   /* packet authentication for TLS handshake */
   struct crypto_options tls_auth;
 
Signature · 1 line
2.7.4
#17 lore raw

Re: [Openvpn-devel] [PATCH 4/5] Add options to restrict cipher negotiation

Quoted 13 lines · Steffan Karger
src/openvpn/push.c +1 -0
@@ -239,11 +239,21 @@ incoming_push_message (struct context *c, const
 struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from

remote */

       if (status == PUSH_MSG_REPLY)

- do_up (c, true, c->options.push_option_types_found ); /* delay
bringing tun/tap up until --push parms received from remote */

src/openvpn/push.c incoming_push_message (struct context *c, const struct buffer *buffer) · line 249

Quoted 6 lines
src/openvpn/push.c +6 -0
+       {
+         if (!do_up (c, true, c->options.push_option_types_found))
+           {
+             msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+             c->sig->signal_received = SIGUSR1; /* connection reset */
+             c->sig->signal_text = "open-tuntap-failed";

Please try to avoid that potential overwrite of a pending signal --- as this runs on the client, c->sig here is a pointer to the global siginfo_static.

Use register_signal (c, SIGUSR1, "open-tuntap-failed") instead -- it will at least protect SIGTERM. I'm looking into how to prioritize signals, and it would help to have all signals set through a few functions such as register_signal, throw_signal_soft etc ..

By the way "open-tun-tap failed" may be a confusing as a real open tun/tap error would have resulted in a FATAL exit -- the error handled here could happen even if tun/tap has been successfully opened, isn't it?

Selva

#18 lore raw

Re: [Openvpn-devel] [PATCH 4/5] Add options to restrict cipher negotiation

Quoted 25 lines · Selva Nair
Quoted 13 lines · Steffan Karger
src/openvpn/push.c +1 -0
@@ -239,11 +239,21 @@ incoming_push_message (struct context *c, const
 struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from

remote */

       if (status == PUSH_MSG_REPLY)

- do_up (c, true, c->options.push_option_types_found ); /* delay
bringing tun/tap up until --push parms received from remote */

src/openvpn/push.c incoming_push_message (struct context *c, const struct buffer *buffer) · line 249

Quoted 6 lines
src/openvpn/push.c +6 -0
+       {
+         if (!do_up (c, true, c->options.push_option_types_found))
+           {
+             msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+             c->sig->signal_received = SIGUSR1; /* connection reset */
+             c->sig->signal_text = "open-tuntap-failed";

Please try to avoid that potential overwrite of a pending signal --- as this runs on the client, c->sig here is a pointer to the global siginfo_static.

Quoted 4 lines

Use register_signal (c, SIGUSR1, "open-tuntap-failed") instead -- it will at least protect SIGTERM. I'm looking into how to prioritize signals, and it would help to have all signals set through a few functions such as register_signal, throw_signal_soft etc ..

Sure, please tell me what to use. I never had to deal with our signals before, so I simply copied this from a few lines below, where (as far as I could see) the did what I wanted it to do.

I had already replaced this with

   throw_signal_soft( SIGUSR1, "process-push-msg-failed" );

based on feedback from Gert (v2, yet to come). In my tests that also works as I want it to (abort the connection attempt, try again). Can you explain when I need to use register_signal(), throw_signal() or throw_signal_soft()?

Quoted 3 lines

By the way "open-tun-tap failed" may be a confusing as a real open tun/tap error would have resulted in a FATAL exit -- the error handled here could happen even if tun/tap has been successfully opened, isn't it?

Yes, noticed that too, and is actually fixed by 5/5. I didn't notice 4/5 still got the confusing text.

-Steffan

#19 lore raw

Re: [Openvpn-devel] [PATCH 4/5] Add options to restrict cipher negotiation

Quoted 8 lines · Steffan Karger

Sure, please tell me what to use. I never had to deal with our signals before, so I simply copied this from a few lines below, where (as far as I could see) the did what I wanted it to do.

I had already replaced this with

   throw_signal_soft( SIGUSR1, "process-push-msg-failed" );

In this particular case both throw_signal_soft and register_signal does almost the same thing, but the latter protects SIGTERM out of the box [*]. But, in general, its better to use register signal for setting signals inside the context struct. That is, when all that one has is a pointer to a context which could be in a multi_instance, or the top context of the server or a the context of a client, throw_signal_soft may not be what one wants.

Basically, register_signal set the signal_received member of sig in a context, while throw_signal_soft sets the process-wide signal_received. Both are the same for a client or for the top level context of the server.

Selva

[*] I don't prefer one over the other as both of those functions will have to be improved up on once we have a clear plan for 'signal prioritization'

#20 lore raw

Re: [Openvpn-devel] [PATCH 4/5] Add options to restrict cipher negotiation

Quoted 4 lines · Selva Nair

But, in general, its better to use register signal for setting signals inside the context struct. That is, when all that one has is a pointer to a context which could be in a multi_instance, or the top context of the server or a the context of a client, throw_signal_soft may not be what one wants.

Thanks. register_signal() it is.

-Steffan

#21 lore raw

PATCH v2 4/5 Add options to restrict cipher negotiation

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

v2:
* fix --disable-crypto builds
* use register_signal() instead of operating directly on c->sig
* add man-page entry for new options

restrict man fix

---
 doc/openvpn.8            | 13 +++++++++++++
 src/openvpn/init.c       | 36 ++++++++++++++++++++++++++++++------
 src/openvpn/init.h       |  4 ++--
 src/openvpn/openvpn.h    |  3 +++
 src/openvpn/options.c    | 40 +++++++++++++++++++++++++++++-----------
 src/openvpn/options.h    |  4 +++-
 src/openvpn/push.c       | 11 ++++++++++-
 src/openvpn/ssl.c        | 30 +++++++++++++++++++++++++++++-
 src/openvpn/ssl_common.h |  4 ++++
 9 files changed, 123 insertions(+), 22 deletions(-)
doc/openvpn.8 +13 -0
index 03f31bb..e349b77 100644
@@ -4128,6 +4128,19 @@ Set
 to disable encryption.
 .\"*********************************************************
 .TP
+.B \-\-ncp\-ciphers cipher_list
+Restrict the allowed ciphers to be negotiated to the ciphers in
+.B cipher_list\fR.
+.B cipher_list
+is a colon-separated list of ciphers, and defaults to
+"AES-256-GCM:AES-128-GCM".
+.\"*********************************************************
+.TP
+.B \-\-ncp\-disable
+Disable "negotiable crypto parameters".  This completely disables cipher
+negotiation.
+.\"*********************************************************
+.TP
 .B \-\-keysize n
 Size of cipher key in bits (optional).
 If unspecified, defaults to cipher-specific default.  The
src/openvpn/init.c +30 -6
index 8f81b09..c2e3b2e 100644
@@ -1739,7 +1739,7 @@ options_hash_changed_or_zero(const struct md5_digest *a,
 }
 #endif /* P2MP */
 
-void
+bool
 do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 {
   if (!c->c2.do_up_ran)
@@ -1747,7 +1747,13 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
       reset_coarse_timers (c);
 
       if (pulled_options && option_types_found)
-	do_deferred_options (c, option_types_found);
+	{
+	  if (!do_deferred_options (c, option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "ERROR: Failed to apply push options");
+	      return false;
+	    }
+	}
 
       /* if --up-delay specified, open tun, do ifconfig, and run up script now */
       if (c->options.up_delay || PULL_DEFINED (&c->options))
@@ -1802,6 +1808,7 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 	
       c->c2.do_up_ran = true;
     }
+  return true;
 }
 
 /*
@@ -1819,7 +1826,6 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
-    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1830,13 +1836,18 @@ pull_permission_mask (const struct context *c)
   if (!c->options.route_nopull)
     flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
 
+#ifdef ENABLE_CRYPTO
+  if (c->options.ncp_enabled)
+    flags |= OPT_P_NCP;
+#endif
+
   return flags;
 }
 
 /*
  * Handle non-tun-related pulled options.
  */
-void
+bool
 do_deferred_options (struct context *c, const unsigned int found)
 {
   if (found & OPT_P_MESSAGES)
@@ -1927,10 +1938,17 @@ do_deferred_options (struct context *c, const unsigned int found)
   /* process (potenitally pushed) crypto options */
   if (c->options.pull)
     {
-      tls_session_update_crypto_params(&c->c2.tls_multi->session[TM_ACTIVE],
-          &c->options, &c->c2.frame);
+      if (found & OPT_P_NCP)
+	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
+      if (!tls_session_update_crypto_params(
+	  &c->c2.tls_multi->session[TM_ACTIVE], &c->options, &c->c2.frame))
+	{
+	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
+	  return false;
+	}
     }
 #endif
+  return true;
 }
 
 /*
@@ -2250,6 +2268,9 @@ do_init_crypto_tls_c1 (struct context *c)
 	      &c->c1.ks.tls_auth_key, file, options->key_direction, flags);
 	}
 
+      c->c1.ciphername = options->ciphername;
+      c->c1.authname = options->authname;
+
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
       if (options->priv_key_file_inline)
 	{
@@ -2326,6 +2347,9 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.replay_window = options->replay_window;
   to.replay_time = options->replay_time;
   to.tcp_mode = link_socket_proto_connection_oriented (options->ce.proto);
+  to.config_ciphername = c->c1.ciphername;
+  to.config_authname = c->c1.authname;
+  to.ncp_enabled = options->ncp_enabled;
   to.transition_window = options->transition_window;
   to.handshake_window = options->handshake_window;
   to.packet_timeout = options->tls_timeout;
src/openvpn/init.h +2 -2
index a819bd2..524bc64 100644
@@ -81,7 +81,7 @@ bool do_test_crypto (const struct options *o);
 
 void context_gc_free (struct context *c);
 
-void do_up (struct context *c,
+bool do_up (struct context *c,
 	    bool pulled_options,
 	    unsigned int option_types_found);
 
@@ -91,7 +91,7 @@ const char *format_common_name (struct context *c, struct gc_arena *gc);
 
 void reset_coarse_timers (struct context *c);
 
-void do_deferred_options (struct context *c, const unsigned int found);
+bool do_deferred_options (struct context *c, const unsigned int found);
 
 void inherit_context_child (struct context *dest,
 			    const struct context *src);
src/openvpn/openvpn.h +3 -0
index 3281fd7..395195f 100644
@@ -210,6 +210,9 @@ struct context_1
   struct user_pass *auth_user_pass;
                                 /**< Username and password for
                                  *   authentication. */
+
+  const char *ciphername;	/**< Data channel cipher from config file */
+  const char *authname;		/**< Data channel auth from config file */
 #endif
 };
 
src/openvpn/options.c +29 -11
index a595dff..d17a2ce 100644
@@ -523,6 +523,8 @@ static const char usage_message[] =
   "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
   "                  (default=%s).\n"
   "                  Set alg=none to disable encryption.\n"
+  "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
+  "--ncp-disable   : Disable cipher negotiation.\n"
   "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
   "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
@@ -830,6 +832,12 @@ init_options (struct options *o, const bool init_gc)
 #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
+  o->ncp_enabled = false;
+#endif
+  o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
   o->authname = "SHA1";
   o->authname_defined = true;
   o->prng_hash = "SHA1";
@@ -6632,7 +6640,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
       options->authname = p[1];
       if (streq (options->authname, "none"))
@@ -6643,12 +6651,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       options->ciphername_defined = true;
       options->ciphername = p[1];
       if (streq (options->ciphername, "none"))
@@ -6659,12 +6667,22 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "cipher") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      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);
+      options->ncp_ciphers = p[1];
+    }
+  else if (streq (p[0], "ncp-disable") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->ncp_enabled = false;
+    }
   else if (streq (p[0], "prng") && p[1] && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (streq (p[1], "none"))
 	options->prng_hash = NULL;
       else
@@ -6686,12 +6704,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "no-replay") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->replay = false;
     }
   else if (streq (p[0], "replay-window") && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (p[1])
 	{
 	  int replay_window;
@@ -6731,12 +6749,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "mute-replay-warnings") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->mute_replay_warnings = true;
     }
   else if (streq (p[0], "no-iv") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->use_iv = false;
     }
   else if (streq (p[0], "replay-persist") && p[1] && !p[2])
@@ -6766,7 +6784,7 @@ add_option (struct options *options,
     {
       int keysize;
 
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       keysize = atoi (p[1]) / 8;
       if (keysize < 0 || keysize > MAX_CIPHER_KEY_LENGTH)
 	{
@@ -6795,7 +6813,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "ecdh-curve") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->ecdh_curve= p[1];
     }
   else if (streq (p[0], "tls-server") && !p[1])
src/openvpn/options.h +3 -1
index 514511b..ea63c4c 100644
@@ -474,6 +474,8 @@ struct options
   int key_direction;
   bool ciphername_defined;
   const char *ciphername;
+  bool ncp_enabled;
+  const char *ncp_ciphers;
   bool authname_defined;
   const char *authname;
   int keysize;
@@ -618,7 +620,7 @@ struct options
 #define OPT_P_PERSIST_IP      (1<<9)
 #define OPT_P_COMP            (1<<10) /* TODO */
 #define OPT_P_MESSAGES        (1<<11)
-#define OPT_P_CRYPTO          (1<<12) /* TODO */
+#define OPT_P_NCP             (1<<12) /**< Negotiable crypto parameters */
 #define OPT_P_TLS_PARMS       (1<<13) /* TODO */
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
src/openvpn/push.c +10 -1
index 38ac59e..4239e3e 100644
@@ -239,11 +239,20 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from remote */
       if (status == PUSH_MSG_REPLY)
-	do_up (c, true, c->options.push_option_types_found ); /* delay bringing tun/tap up until --push parms received from remote */
+	{
+	  if (!do_up (c, true, c->options.push_option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+	      register_signal (c, SIGUSR1, "do_up-failed");
+	      goto cleanup;
+	    }
+	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
 
+cleanup:
   gc_free (&gc);
 }
 
src/openvpn/ssl.c +29 -1
index 0c0061c..7ced41d 100644
@@ -1636,6 +1636,24 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
+static bool
+item_in_list(const char *item, const char *list)
+{
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while(token)
+    {
+      if (0 == strcmp (token, item))
+	break;
+      token = strtok (NULL, ":");
+    }
+  free(tmp_ciphers_orig);
+
+  return token != NULL;
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)
@@ -1646,6 +1664,15 @@ tls_session_update_crypto_params(struct tls_session *session,
   ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
+  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+      !item_in_list(options->ciphername, options->ncp_ciphers))
+    {
+      msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
+	  options->ciphername, session->opt->config_ciphername,
+	  options->ncp_ciphers);
+      return false;
+    }
+
   init_key_type (&session->opt->key_type, options->ciphername,
     options->ciphername_defined, options->authname, options->authname_defined,
     options->keysize, true, true);
@@ -1927,7 +1954,8 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
       buf_printf(&out, "IV_PROTO=2\n");
 
       /* support for Negotiable Crypto Paramters */
-      buf_printf(&out, "IV_NCP=2\n");
+      if (session->opt->ncp_enabled)
+	buf_printf(&out, "IV_NCP=2\n");
 
       /* push compression status */
 #ifdef USE_COMP
src/openvpn/ssl_common.h +4 -0
index 9183dab..6bfde6b 100644
@@ -273,6 +273,10 @@ struct tls_options
   int replay_time;                     /* --replay-window parm */
   bool tcp_mode;
 
+  const char *config_ciphername;
+  const char *config_authname;
+  bool ncp_enabled;
+
   /* packet authentication for TLS handshake */
   struct crypto_options tls_auth;
 
Signature · 1 line
2.7.4
#22 lore raw

PATCH v3 4/5 Add options to restrict cipher negotiation

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

v2:
* fix --disable-crypto builds
* use register_signal() instead of operating directly on c->sig
* add man-page entry for new options

v3:
* rebased on client-side NCP v3

---
 doc/openvpn.8            | 13 +++++++++++++
 src/openvpn/init.c       | 33 ++++++++++++++++++++++++++-------
 src/openvpn/init.h       |  4 ++--
 src/openvpn/openvpn.h    |  3 +++
 src/openvpn/options.c    | 40 +++++++++++++++++++++++++++++-----------
 src/openvpn/options.h    |  4 +++-
 src/openvpn/push.c       | 11 ++++++++++-
 src/openvpn/ssl.c        | 29 ++++++++++++++++++++++++++++-
 src/openvpn/ssl_common.h |  4 ++++
 9 files changed, 118 insertions(+), 23 deletions(-)
doc/openvpn.8 +13 -0
index 03f31bb..e349b77 100644
@@ -4128,6 +4128,19 @@ Set
 to disable encryption.
 .\"*********************************************************
 .TP
+.B \-\-ncp\-ciphers cipher_list
+Restrict the allowed ciphers to be negotiated to the ciphers in
+.B cipher_list\fR.
+.B cipher_list
+is a colon-separated list of ciphers, and defaults to
+"AES-256-GCM:AES-128-GCM".
+.\"*********************************************************
+.TP
+.B \-\-ncp\-disable
+Disable "negotiable crypto parameters".  This completely disables cipher
+negotiation.
+.\"*********************************************************
+.TP
 .B \-\-keysize n
 Size of cipher key in bits (optional).
 If unspecified, defaults to cipher-specific default.  The
src/openvpn/init.c +26 -7
index 70c74a3..b4efaf0 100644
@@ -1739,7 +1739,7 @@ options_hash_changed_or_zero(const struct md5_digest *a,
 }
 #endif /* P2MP */
 
-void
+bool
 do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 {
   if (!c->c2.do_up_ran)
@@ -1747,7 +1747,13 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
       reset_coarse_timers (c);
 
       if (pulled_options && option_types_found)
-	do_deferred_options (c, option_types_found);
+	{
+	  if (!do_deferred_options (c, option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "ERROR: Failed to apply push options");
+	      return false;
+	    }
+	}
 
       /* if --up-delay specified, open tun, do ifconfig, and run up script now */
       if (c->options.up_delay || PULL_DEFINED (&c->options))
@@ -1802,6 +1808,7 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 	
       c->c2.do_up_ran = true;
     }
+  return true;
 }
 
 /*
@@ -1819,7 +1826,6 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
-    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1830,13 +1836,18 @@ pull_permission_mask (const struct context *c)
   if (!c->options.route_nopull)
     flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
 
+#ifdef ENABLE_CRYPTO
+  if (c->options.ncp_enabled)
+    flags |= OPT_P_NCP;
+#endif
+
   return flags;
 }
 
 /*
  * Handle non-tun-related pulled options.
  */
-void
+bool
 do_deferred_options (struct context *c, const unsigned int found)
 {
   if (found & OPT_P_MESSAGES)
@@ -1931,13 +1942,15 @@ do_deferred_options (struct context *c, const unsigned int found)
       if (found & OPT_P_NCP)
 	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
       /* Do not regenerate keys if server sends an extra push request */
-      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
+	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
 	{
-	  tls_session_update_crypto_params(&c->c2.tls_multi->session[TM_ACTIVE],
-	      &c->options, &c->c2.frame);
+	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
+	  return false;
 	}
     }
 #endif
+  return true;
 }
 
 /*
@@ -2257,6 +2270,9 @@ do_init_crypto_tls_c1 (struct context *c)
 	      &c->c1.ks.tls_auth_key, file, options->key_direction, flags);
 	}
 
+      c->c1.ciphername = options->ciphername;
+      c->c1.authname = options->authname;
+
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
       if (options->priv_key_file_inline)
 	{
@@ -2333,6 +2349,9 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.replay_window = options->replay_window;
   to.replay_time = options->replay_time;
   to.tcp_mode = link_socket_proto_connection_oriented (options->ce.proto);
+  to.config_ciphername = c->c1.ciphername;
+  to.config_authname = c->c1.authname;
+  to.ncp_enabled = options->ncp_enabled;
   to.transition_window = options->transition_window;
   to.handshake_window = options->handshake_window;
   to.packet_timeout = options->tls_timeout;
src/openvpn/init.h +2 -2
index a819bd2..524bc64 100644
@@ -81,7 +81,7 @@ bool do_test_crypto (const struct options *o);
 
 void context_gc_free (struct context *c);
 
-void do_up (struct context *c,
+bool do_up (struct context *c,
 	    bool pulled_options,
 	    unsigned int option_types_found);
 
@@ -91,7 +91,7 @@ const char *format_common_name (struct context *c, struct gc_arena *gc);
 
 void reset_coarse_timers (struct context *c);
 
-void do_deferred_options (struct context *c, const unsigned int found);
+bool do_deferred_options (struct context *c, const unsigned int found);
 
 void inherit_context_child (struct context *dest,
 			    const struct context *src);
src/openvpn/openvpn.h +3 -0
index 3281fd7..395195f 100644
@@ -210,6 +210,9 @@ struct context_1
   struct user_pass *auth_user_pass;
                                 /**< Username and password for
                                  *   authentication. */
+
+  const char *ciphername;	/**< Data channel cipher from config file */
+  const char *authname;		/**< Data channel auth from config file */
 #endif
 };
 
src/openvpn/options.c +29 -11
index a595dff..d17a2ce 100644
@@ -523,6 +523,8 @@ static const char usage_message[] =
   "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
   "                  (default=%s).\n"
   "                  Set alg=none to disable encryption.\n"
+  "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
+  "--ncp-disable   : Disable cipher negotiation.\n"
   "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
   "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
@@ -830,6 +832,12 @@ init_options (struct options *o, const bool init_gc)
 #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
+  o->ncp_enabled = false;
+#endif
+  o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
   o->authname = "SHA1";
   o->authname_defined = true;
   o->prng_hash = "SHA1";
@@ -6632,7 +6640,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
       options->authname = p[1];
       if (streq (options->authname, "none"))
@@ -6643,12 +6651,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       options->ciphername_defined = true;
       options->ciphername = p[1];
       if (streq (options->ciphername, "none"))
@@ -6659,12 +6667,22 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "cipher") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      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);
+      options->ncp_ciphers = p[1];
+    }
+  else if (streq (p[0], "ncp-disable") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->ncp_enabled = false;
+    }
   else if (streq (p[0], "prng") && p[1] && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (streq (p[1], "none"))
 	options->prng_hash = NULL;
       else
@@ -6686,12 +6704,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "no-replay") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->replay = false;
     }
   else if (streq (p[0], "replay-window") && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (p[1])
 	{
 	  int replay_window;
@@ -6731,12 +6749,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "mute-replay-warnings") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->mute_replay_warnings = true;
     }
   else if (streq (p[0], "no-iv") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->use_iv = false;
     }
   else if (streq (p[0], "replay-persist") && p[1] && !p[2])
@@ -6766,7 +6784,7 @@ add_option (struct options *options,
     {
       int keysize;
 
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       keysize = atoi (p[1]) / 8;
       if (keysize < 0 || keysize > MAX_CIPHER_KEY_LENGTH)
 	{
@@ -6795,7 +6813,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "ecdh-curve") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->ecdh_curve= p[1];
     }
   else if (streq (p[0], "tls-server") && !p[1])
src/openvpn/options.h +3 -1
index 514511b..ea63c4c 100644
@@ -474,6 +474,8 @@ struct options
   int key_direction;
   bool ciphername_defined;
   const char *ciphername;
+  bool ncp_enabled;
+  const char *ncp_ciphers;
   bool authname_defined;
   const char *authname;
   int keysize;
@@ -618,7 +620,7 @@ struct options
 #define OPT_P_PERSIST_IP      (1<<9)
 #define OPT_P_COMP            (1<<10) /* TODO */
 #define OPT_P_MESSAGES        (1<<11)
-#define OPT_P_CRYPTO          (1<<12) /* TODO */
+#define OPT_P_NCP             (1<<12) /**< Negotiable crypto parameters */
 #define OPT_P_TLS_PARMS       (1<<13) /* TODO */
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
src/openvpn/push.c +10 -1
index 38ac59e..4239e3e 100644
@@ -239,11 +239,20 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from remote */
       if (status == PUSH_MSG_REPLY)
-	do_up (c, true, c->options.push_option_types_found ); /* delay bringing tun/tap up until --push parms received from remote */
+	{
+	  if (!do_up (c, true, c->options.push_option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+	      register_signal (c, SIGUSR1, "do_up-failed");
+	      goto cleanup;
+	    }
+	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
 
+cleanup:
   gc_free (&gc);
 }
 
src/openvpn/ssl.c +28 -1
index f9849da..124d458 100644
@@ -1636,6 +1636,24 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
+static bool
+item_in_list(const char *item, const char *list)
+{
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while(token)
+    {
+      if (0 == strcmp (token, item))
+	break;
+      token = strtok (NULL, ":");
+    }
+  free(tmp_ciphers_orig);
+
+  return token != NULL;
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)
@@ -1646,6 +1664,15 @@ tls_session_update_crypto_params(struct tls_session *session,
   ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
+  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+      !item_in_list(options->ciphername, options->ncp_ciphers))
+    {
+      msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
+	  options->ciphername, session->opt->config_ciphername,
+	  options->ncp_ciphers);
+      return false;
+    }
+
   init_key_type (&session->opt->key_type, options->ciphername,
     options->ciphername_defined, options->authname, options->authname_defined,
     options->keysize, true, true);
@@ -1927,7 +1954,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
       buf_printf(&out, "IV_PROTO=2\n");
 
       /* support for Negotiable Crypto Paramters */
-      if (session->opt->pull)
+      if (session->opt->ncp_enabled && session->opt->pull)
 	buf_printf(&out, "IV_NCP=2\n");
 
       /* push compression status */
src/openvpn/ssl_common.h +4 -0
index 9183dab..6bfde6b 100644
@@ -273,6 +273,10 @@ struct tls_options
   int replay_time;                     /* --replay-window parm */
   bool tcp_mode;
 
+  const char *config_ciphername;
+  const char *config_authname;
+  bool ncp_enabled;
+
   /* packet authentication for TLS handshake */
   struct crypto_options tls_auth;
 
Signature · 1 line
2.7.4
#23 lore raw

Re: [Openvpn-devel] [PATCHv3 4/5] Add options to restrict cipher negotiation

Hi,

Quoted 10 lines · Steffan Karger

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

v2:
* fix --disable-crypto builds
* use register_signal() instead of operating directly on c->sig
* add man-page entry for new options

v3:
* rebased on client-side NCP v3

Looks generally OK, too.

The OPT_P_NCP bit introduced here is used in 3/5, so the distribution of new bits and pieces on the 3 parts is not ideal (because applying 3/5 alone would make it not compile) - but you already know that :-)

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]...
#24 lore raw

PATCH v4 4/5 Add options to restrict cipher negotiation

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

v2:
* fix --disable-crypto builds
* use register_signal() instead of operating directly on c->sig
* add man-page entry for new options

v3:
* rebased on client-side NCP v3

v4:
* rebased on client-side NCP v4

---
 doc/openvpn.8            | 13 +++++++++++++
 src/openvpn/init.c       | 34 +++++++++++++++++++++++++++-------
 src/openvpn/init.h       |  4 ++--
 src/openvpn/openvpn.h    |  3 +++
 src/openvpn/options.c    | 40 +++++++++++++++++++++++++++++-----------
 src/openvpn/options.h    |  4 +++-
 src/openvpn/push.c       | 11 ++++++++++-
 src/openvpn/ssl.c        | 29 ++++++++++++++++++++++++++++-
 src/openvpn/ssl_common.h |  4 ++++
 9 files changed, 119 insertions(+), 23 deletions(-)
doc/openvpn.8 +13 -0
index 03f31bb..e349b77 100644
@@ -4128,6 +4128,19 @@ Set
 to disable encryption.
 .\"*********************************************************
 .TP
+.B \-\-ncp\-ciphers cipher_list
+Restrict the allowed ciphers to be negotiated to the ciphers in
+.B cipher_list\fR.
+.B cipher_list
+is a colon-separated list of ciphers, and defaults to
+"AES-256-GCM:AES-128-GCM".
+.\"*********************************************************
+.TP
+.B \-\-ncp\-disable
+Disable "negotiable crypto parameters".  This completely disables cipher
+negotiation.
+.\"*********************************************************
+.TP
 .B \-\-keysize n
 Size of cipher key in bits (optional).
 If unspecified, defaults to cipher-specific default.  The
src/openvpn/init.c +27 -7
index 2f4c631..b4efaf0 100644
@@ -1739,7 +1739,7 @@ options_hash_changed_or_zero(const struct md5_digest *a,
 }
 #endif /* P2MP */
 
-void
+bool
 do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 {
   if (!c->c2.do_up_ran)
@@ -1747,7 +1747,13 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
       reset_coarse_timers (c);
 
       if (pulled_options && option_types_found)
-	do_deferred_options (c, option_types_found);
+	{
+	  if (!do_deferred_options (c, option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "ERROR: Failed to apply push options");
+	      return false;
+	    }
+	}
 
       /* if --up-delay specified, open tun, do ifconfig, and run up script now */
       if (c->options.up_delay || PULL_DEFINED (&c->options))
@@ -1802,6 +1808,7 @@ do_up (struct context *c, bool pulled_options, unsigned int option_types_found)
 	
       c->c2.do_up_ran = true;
     }
+  return true;
 }
 
 /*
@@ -1819,7 +1826,6 @@ pull_permission_mask (const struct context *c)
     | OPT_P_SHAPER
     | OPT_P_TIMER
     | OPT_P_COMP
-    | OPT_P_CRYPTO
     | OPT_P_PERSIST
     | OPT_P_MESSAGES
     | OPT_P_EXPLICIT_NOTIFY
@@ -1830,13 +1836,18 @@ pull_permission_mask (const struct context *c)
   if (!c->options.route_nopull)
     flags |= (OPT_P_ROUTE | OPT_P_IPWIN32);
 
+#ifdef ENABLE_CRYPTO
+  if (c->options.ncp_enabled)
+    flags |= OPT_P_NCP;
+#endif
+
   return flags;
 }
 
 /*
  * Handle non-tun-related pulled options.
  */
-void
+bool
 do_deferred_options (struct context *c, const unsigned int found)
 {
   if (found & OPT_P_MESSAGES)
@@ -1928,15 +1939,18 @@ do_deferred_options (struct context *c, const unsigned int found)
   if (c->options.pull)
     {
       struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
-      if (found & OPT_P_CRYPTO)
+      if (found & OPT_P_NCP)
 	msg (D_PUSH, "OPTIONS IMPORT: data channel crypto options modified");
       /* Do not regenerate keys if server sends an extra push request */
-      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+      if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
+	  !tls_session_update_crypto_params(session, &c->options, &c->c2.frame))
 	{
-	  tls_session_update_crypto_params(session, &c->options, &c->c2.frame);
+	  msg (D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options");
+	  return false;
 	}
     }
 #endif
+  return true;
 }
 
 /*
@@ -2256,6 +2270,9 @@ do_init_crypto_tls_c1 (struct context *c)
 	      &c->c1.ks.tls_auth_key, file, options->key_direction, flags);
 	}
 
+      c->c1.ciphername = options->ciphername;
+      c->c1.authname = options->authname;
+
 #if 0 /* was: #if ENABLE_INLINE_FILES --  Note that enabling this code will break restarts */
       if (options->priv_key_file_inline)
 	{
@@ -2332,6 +2349,9 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.replay_window = options->replay_window;
   to.replay_time = options->replay_time;
   to.tcp_mode = link_socket_proto_connection_oriented (options->ce.proto);
+  to.config_ciphername = c->c1.ciphername;
+  to.config_authname = c->c1.authname;
+  to.ncp_enabled = options->ncp_enabled;
   to.transition_window = options->transition_window;
   to.handshake_window = options->handshake_window;
   to.packet_timeout = options->tls_timeout;
src/openvpn/init.h +2 -2
index a819bd2..524bc64 100644
@@ -81,7 +81,7 @@ bool do_test_crypto (const struct options *o);
 
 void context_gc_free (struct context *c);
 
-void do_up (struct context *c,
+bool do_up (struct context *c,
 	    bool pulled_options,
 	    unsigned int option_types_found);
 
@@ -91,7 +91,7 @@ const char *format_common_name (struct context *c, struct gc_arena *gc);
 
 void reset_coarse_timers (struct context *c);
 
-void do_deferred_options (struct context *c, const unsigned int found);
+bool do_deferred_options (struct context *c, const unsigned int found);
 
 void inherit_context_child (struct context *dest,
 			    const struct context *src);
src/openvpn/openvpn.h +3 -0
index 3281fd7..395195f 100644
@@ -210,6 +210,9 @@ struct context_1
   struct user_pass *auth_user_pass;
                                 /**< Username and password for
                                  *   authentication. */
+
+  const char *ciphername;	/**< Data channel cipher from config file */
+  const char *authname;		/**< Data channel auth from config file */
 #endif
 };
 
src/openvpn/options.c +29 -11
index a595dff..d17a2ce 100644
@@ -523,6 +523,8 @@ static const char usage_message[] =
   "--cipher alg    : Encrypt packets with cipher algorithm alg\n"
   "                  (default=%s).\n"
   "                  Set alg=none to disable encryption.\n"
+  "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
+  "--ncp-disable   : Disable cipher negotiation.\n"
   "--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
   "                   nonce_secret_len=nsl.  Set alg=none to disable PRNG.\n"
 #ifdef HAVE_EVP_CIPHER_CTX_SET_KEY_LENGTH
@@ -830,6 +832,12 @@ init_options (struct options *o, const bool init_gc)
 #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
+  o->ncp_enabled = false;
+#endif
+  o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
   o->authname = "SHA1";
   o->authname_defined = true;
   o->prng_hash = "SHA1";
@@ -6632,7 +6640,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
       options->authname = p[1];
       if (streq (options->authname, "none"))
@@ -6643,12 +6651,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "auth") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->authname_defined = true;
     }
   else if (streq (p[0], "cipher") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       options->ciphername_defined = true;
       options->ciphername = p[1];
       if (streq (options->ciphername, "none"))
@@ -6659,12 +6667,22 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "cipher") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      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);
+      options->ncp_ciphers = p[1];
+    }
+  else if (streq (p[0], "ncp-disable") && !p[1])
+    {
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      options->ncp_enabled = false;
+    }
   else if (streq (p[0], "prng") && p[1] && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (streq (p[1], "none"))
 	options->prng_hash = NULL;
       else
@@ -6686,12 +6704,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "no-replay") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->replay = false;
     }
   else if (streq (p[0], "replay-window") && !p[3])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       if (p[1])
 	{
 	  int replay_window;
@@ -6731,12 +6749,12 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "mute-replay-warnings") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->mute_replay_warnings = true;
     }
   else if (streq (p[0], "no-iv") && !p[1])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->use_iv = false;
     }
   else if (streq (p[0], "replay-persist") && p[1] && !p[2])
@@ -6766,7 +6784,7 @@ add_option (struct options *options,
     {
       int keysize;
 
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_NCP);
       keysize = atoi (p[1]) / 8;
       if (keysize < 0 || keysize > MAX_CIPHER_KEY_LENGTH)
 	{
@@ -6795,7 +6813,7 @@ add_option (struct options *options,
     }
   else if (streq (p[0], "ecdh-curve") && p[1] && !p[2])
     {
-      VERIFY_PERMISSION (OPT_P_CRYPTO);
+      VERIFY_PERMISSION (OPT_P_GENERAL);
       options->ecdh_curve= p[1];
     }
   else if (streq (p[0], "tls-server") && !p[1])
src/openvpn/options.h +3 -1
index 514511b..ea63c4c 100644
@@ -474,6 +474,8 @@ struct options
   int key_direction;
   bool ciphername_defined;
   const char *ciphername;
+  bool ncp_enabled;
+  const char *ncp_ciphers;
   bool authname_defined;
   const char *authname;
   int keysize;
@@ -618,7 +620,7 @@ struct options
 #define OPT_P_PERSIST_IP      (1<<9)
 #define OPT_P_COMP            (1<<10) /* TODO */
 #define OPT_P_MESSAGES        (1<<11)
-#define OPT_P_CRYPTO          (1<<12) /* TODO */
+#define OPT_P_NCP             (1<<12) /**< Negotiable crypto parameters */
 #define OPT_P_TLS_PARMS       (1<<13) /* TODO */
 #define OPT_P_MTU             (1<<14) /* TODO */
 #define OPT_P_NICE            (1<<15)
src/openvpn/push.c +10 -1
index 38ac59e..4239e3e 100644
@@ -239,11 +239,20 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
     {
       c->options.push_option_types_found |= option_types_found;
 
+      /* delay bringing tun/tap up until --push parms received from remote */
       if (status == PUSH_MSG_REPLY)
-	do_up (c, true, c->options.push_option_types_found ); /* delay bringing tun/tap up until --push parms received from remote */
+	{
+	  if (!do_up (c, true, c->options.push_option_types_found))
+	    {
+	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
+	      register_signal (c, SIGUSR1, "do_up-failed");
+	      goto cleanup;
+	    }
+	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
 
+cleanup:
   gc_free (&gc);
 }
 
src/openvpn/ssl.c +28 -1
index fafa4ad..7e97ba8 100644
@@ -1640,6 +1640,24 @@ key_ctx_update_implicit_iv(struct key_ctx *ctx, uint8_t *key, size_t key_len) {
     }
 }
 
+static bool
+item_in_list(const char *item, const char *list)
+{
+  char *tmp_ciphers = string_alloc (list, NULL);
+  char *tmp_ciphers_orig = tmp_ciphers;
+
+  const char *token = strtok (tmp_ciphers, ":");
+  while(token)
+    {
+      if (0 == strcmp (token, item))
+	break;
+      token = strtok (NULL, ":");
+    }
+  free(tmp_ciphers_orig);
+
+  return token != NULL;
+}
+
 bool
 tls_session_update_crypto_params(struct tls_session *session,
     const struct options *options, struct frame *frame)
@@ -1650,6 +1668,15 @@ tls_session_update_crypto_params(struct tls_session *session,
   ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
+  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+      !item_in_list(options->ciphername, options->ncp_ciphers))
+    {
+      msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
+	  options->ciphername, session->opt->config_ciphername,
+	  options->ncp_ciphers);
+      return false;
+    }
+
   init_key_type (&session->opt->key_type, options->ciphername,
     options->ciphername_defined, options->authname, options->authname_defined,
     options->keysize, true, true);
@@ -1931,7 +1958,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
       buf_printf(&out, "IV_PROTO=2\n");
 
       /* support for Negotiable Crypto Paramters */
-      if (session->opt->pull)
+      if (session->opt->ncp_enabled && session->opt->pull)
 	buf_printf(&out, "IV_NCP=2\n");
 
       /* push compression status */
src/openvpn/ssl_common.h +4 -0
index 9183dab..6bfde6b 100644
@@ -273,6 +273,10 @@ struct tls_options
   int replay_time;                     /* --replay-window parm */
   bool tcp_mode;
 
+  const char *config_ciphername;
+  const char *config_authname;
+  bool ncp_enabled;
+
   /* packet authentication for TLS handshake */
   struct crypto_options tls_auth;
 
Signature · 1 line
2.7.4
#25 lore raw

Re: [Openvpn-devel] [PATCH 4/5 v4] Add options to restrict cipher negotiation

Am 28.06.16 um 23:35 schrieb Steffan Karger:

Quoted 8 lines

Add --ncp-disable to completely disable cipher negotiation, and --ncp-ciphers to specify which ciphers to accept from the server.

v2:
* fix --disable-crypto builds
* use register_signal() instead of operating directly on c->sig
* add man-page entry for new options

Quoted 7 lines

v3:
* rebased on client-side NCP v3

v4:
* rebased on client-side NCP v4

After going through iterations I also have no more comments and this gets an ACK from me.

Arne

#26 lore raw

[Openvpn-devel] [PATCH applied] Re: Add options to restrict cipher negotiation

In reply to Steffan Karger

Another ACK from me. This is fairly trivial (compared to 3/5, at least).

As a side note: as discussed on IRC, the extra copy of ciphername and authname in c1.* is needed because "this is what is in the initial config" and the variables in "options->*" might be overwritten by PUSH_REPLY (I was confused, others might be as well, spelling it out here).

Your patch has been applied to the master branch.

commit d728ebeda8c94fe401dc41b9fbda682ea0d780c9

Author: Steffan Karger
Date:   Tue Jun 28 23:35:00 2016 +0200
     Add options to restrict cipher negotiation

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/12008
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#27 lore raw

PATCH 5/5 Add server-side support for cipher negotiation

In reply to Steffan Karger

Pushes AES-256-GCM when a connection client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

---
 src/openvpn/init.c       |  5 +++--
 src/openvpn/push.c       | 27 +++++++++++++++++++++++----
 src/openvpn/ssl.c        | 23 +++++++++++++----------
 src/openvpn/ssl_common.h |  1 +
 4 files changed, 40 insertions(+), 16 deletions(-)
src/openvpn/init.c +3 -2
index af61045..ec0e69e 100644
@@ -2310,8 +2310,8 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters (postpone if we pull options) */
-  if (!c->options.pull)
+  /* Compute MTU parameters (postpone if we push/pull options) */
+  if (!c->options.pull && c->options.mode != MODE_SERVER)
     {
       crypto_adjust_frame_parameters(&c->c2.frame, &c->c1.ks.key_type,
 	  options->ciphername_defined, options->use_iv, options->replay,
@@ -2350,6 +2350,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.mode = options->mode;
   to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
src/openvpn/push.c +23 -4
index 5324676..dc4d691 100644
@@ -245,14 +245,29 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  if (!do_up (c, true, c->options.push_option_types_found))
 	    {
 	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
-	      c->sig->signal_received = SIGUSR1; /* connection reset */
-	      c->sig->signal_text = "open-tuntap-failed";
-	      goto cleanup;
+	      goto error;
 	    }
 	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
+  else if (status == PUSH_MSG_REQUEST)
+    {
+      if (c->options.mode == MODE_SERVER)
+	{
+	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+	  if (!tls_session_update_crypto_params(session, &c->options,
+	      &c->c2.frame))
+	    {
+	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      goto error;
+	    }
+	}
+    }
 
+  goto cleanup;
+error:
+  c->sig->signal_received = SIGUSR1; /* connection reset */
+  c->sig->signal_text = "process-push-msg-failed";
 cleanup:
   gc_free (&gc);
 }
@@ -304,12 +319,16 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
 
   /* Push cipher if client supports Negotiable Crypto Parameters */
   optstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-  if (optstr)
+  if (optstr && o->ncp_enabled)
     {
       int ncp = 0;
       int r = sscanf(optstr, "IV_NCP=%d", &ncp);
       if ((r == 1) && (ncp == 2))
 	{
+	  /* Push the first cipher from --ncp-ciphers to the client.
+	   * TODO: actual negotiation, instead of server dictatorship. */
+	  char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
+	  o->ciphername = strtok (push_cipher, ":");
 	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
 	}
     }
src/openvpn/ssl.c +13 -10
index 3da254d..7180c3a 100644
@@ -1661,10 +1661,10 @@ tls_session_update_crypto_params(struct tls_session *session,
   bool ret = false;
   struct key_state *ks = &session->key[KS_PRIMARY];	/* primary key */
 
-  ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
-  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+  if (!session->opt->server &&
+      0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
       !item_in_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
@@ -1689,12 +1689,13 @@ tls_session_update_crypto_params(struct tls_session *session,
       options->ce.tun_mtu_defined, options->ce.tun_mtu);
   frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");
 
+  const struct session_id *client_sid = session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
+  const struct session_id *server_sid = !session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
   if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-			       &session->opt->key_type,
-			       ks->key_src,
-			       &session->session_id,
-			       &ks->session_id_remote,
-			       false))
+      &session->opt->key_type, ks->key_src, client_sid, server_sid,
+      session->opt->server))
     {
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
@@ -2055,10 +2056,12 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
   if (!push_peer_info (buf, session))
     goto error;
 
-  /*
-   * generate tunnel keys if server
+  /* Generate tunnel keys if we're a TLS server.
+   * If we're a p2mp server, the first key generation is postponed until after
+   * the pull/push, so we can process pushed cipher directives.
    */
-  if (session->opt->server)
+  if (session->opt->server &&
+      (session->opt->mode != MODE_SERVER || ks->key_id > 0))
     {
       if (ks->authenticated)
 	{
src/openvpn/ssl_common.h +1 -0
index 6bfde6b..eb2ad6f 100644
@@ -236,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  int mode;
   bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
Signature · 1 line
2.7.4
#28 lore raw

PATCH v2 5/5 Add server-side support for cipher negotiation

Pushes AES-256-GCM when a connection client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

v2:
* Update manpage
* Add Changes.rst entry

---
 Changes.rst              | 11 +++++++++++
 doc/openvpn.8            |  4 ++++
 src/openvpn/init.c       |  5 +++--
 src/openvpn/push.c       | 25 ++++++++++++++++++++++---
 src/openvpn/ssl.c        | 23 +++++++++++++----------
 src/openvpn/ssl_common.h |  1 +
 6 files changed, 54 insertions(+), 15 deletions(-)
Changes.rst +11 -0
index 1ac3c2b..eb6018f 100644
@@ -51,6 +51,13 @@ AEAD (GCM) data channel cipher support
     bytes per packet for AES-128-GCM instead of 36 bytes per packet for
     AES-128-CBC + HMAC-SHA1).
 
+Cipher negotiation
+    Data channel ciphers are now by default negotiated.  If a client advertises
+    support for Negotiable Crypto Parameters (NCP), the server will choose a
+    cipher (by default AES-256-GCM) for the data channel, and tell the client
+    to use that cipher.  Data channel cipher negotiation can be controlled
+    using --ncp-ciphers and --ncp-disable.
+
 
 User-visible Changes
 --------------------
@@ -106,6 +113,10 @@ User-visible Changes
 - mbed TLS builds: minimum RSA key size is now 2048 bits.  Shorter keys will
   not be accepted, both local and from the peer.
 
+- Data channel cipher negotiation (see New features section) can override
+  ciphers configured in the config file.  Use --ncp-disable if you don't want
+  that.
+
 
 Maintainer-visible changes
 --------------------------
doc/openvpn.8 +4 -0
index e349b77..5cb2885 100644
@@ -4134,6 +4134,10 @@ Restrict the allowed ciphers to be negotiated to the ciphers in
 .B cipher_list
 is a colon-separated list of ciphers, and defaults to
 "AES-256-GCM:AES-128-GCM".
+
+For servers, the first cipher from
+.B cipher_list
+will be pushed to clients that support cipher negotiation.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +3 -2
index c2e3b2e..5ed3f3c 100644
@@ -2312,8 +2312,8 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters (postpone if we pull options) */
-  if (c->options.pull)
+  /* Compute MTU parameters (postpone if we push/pull options) */
+  if (c->options.pull || c->options.mode == MODE_SERVER)
     {
       /* Account for worst-case crypto overhead before allocating buffers */
       frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
@@ -2357,6 +2357,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.mode = options->mode;
   to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
src/openvpn/push.c +22 -3
index 4239e3e..f763e65 100644
@@ -245,13 +245,28 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  if (!do_up (c, true, c->options.push_option_types_found))
 	    {
 	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
-	      register_signal (c, SIGUSR1, "do_up-failed");
-	      goto cleanup;
+	      goto error;
 	    }
 	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
+  else if (status == PUSH_MSG_REQUEST)
+    {
+      if (c->options.mode == MODE_SERVER)
+	{
+	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+	  if (!tls_session_update_crypto_params (session, &c->options,
+	      &c->c2.frame))
+	    {
+	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      goto error;
+	    }
+	}
+    }
 
+  goto cleanup;
+error:
+  register_signal (c, SIGUSR1, "process-push-msg-failed");
 cleanup:
   gc_free (&gc);
 }
@@ -303,12 +318,16 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
 
   /* Push cipher if client supports Negotiable Crypto Parameters */
   optstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-  if (optstr)
+  if (optstr && o->ncp_enabled)
     {
       int ncp = 0;
       int r = sscanf(optstr, "IV_NCP=%d", &ncp);
       if ((r == 1) && (ncp == 2))
 	{
+	  /* Push the first cipher from --ncp-ciphers to the client.
+	   * TODO: actual negotiation, instead of server dictatorship. */
+	  char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
+	  o->ciphername = strtok (push_cipher, ":");
 	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
 	}
     }
src/openvpn/ssl.c +13 -10
index 7ced41d..d5b52d7 100644
@@ -1661,10 +1661,10 @@ tls_session_update_crypto_params(struct tls_session *session,
   bool ret = false;
   struct key_state *ks = &session->key[KS_PRIMARY];	/* primary key */
 
-  ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
-  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+  if (!session->opt->server &&
+      0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
       !item_in_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
@@ -1691,12 +1691,13 @@ tls_session_update_crypto_params(struct tls_session *session,
       options->ce.tun_mtu_defined, options->ce.tun_mtu);
   frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");
 
+  const struct session_id *client_sid = session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
+  const struct session_id *server_sid = !session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
   if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-			       &session->opt->key_type,
-			       ks->key_src,
-			       &session->session_id,
-			       &ks->session_id_remote,
-			       false))
+      &session->opt->key_type, ks->key_src, client_sid, server_sid,
+      session->opt->server))
     {
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
@@ -2057,10 +2058,12 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
   if (!push_peer_info (buf, session))
     goto error;
 
-  /*
-   * generate tunnel keys if server
+  /* Generate tunnel keys if we're a TLS server.
+   * If we're a p2mp server, the first key generation is postponed until after
+   * the pull/push, so we can process pushed cipher directives.
    */
-  if (session->opt->server)
+  if (session->opt->server &&
+      (session->opt->mode != MODE_SERVER || ks->key_id > 0))
     {
       if (ks->authenticated)
 	{
src/openvpn/ssl_common.h +1 -0
index 6bfde6b..eb2ad6f 100644
@@ -236,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  int mode;
   bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
Signature · 1 line
2.7.4
#29 lore raw

PATCH v3 5/5 Add server-side support for cipher negotiation

Pushes AES-256-GCM when a connecting client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

v2:
* Update manpage
* Add Changes.rst entry

v3:
* Do not regenerate keys if the client sends a second pull request
* Don't postpone key generation if client has no IV_NCP support (to also support clients that do not send a push request)

---
 Changes.rst              | 11 +++++++++++
 doc/openvpn.8            |  4 ++++
 src/openvpn/init.c       |  5 +++--
 src/openvpn/push.c       | 35 +++++++++++++++++++++++++----------
 src/openvpn/ssl.c        | 37 +++++++++++++++++++++++++------------
 src/openvpn/ssl_common.h |  1 +
 6 files changed, 69 insertions(+), 24 deletions(-)
Changes.rst +11 -0
index 1ac3c2b..eb6018f 100644
@@ -51,6 +51,13 @@ AEAD (GCM) data channel cipher support
     bytes per packet for AES-128-GCM instead of 36 bytes per packet for
     AES-128-CBC + HMAC-SHA1).
 
+Cipher negotiation
+    Data channel ciphers are now by default negotiated.  If a client advertises
+    support for Negotiable Crypto Parameters (NCP), the server will choose a
+    cipher (by default AES-256-GCM) for the data channel, and tell the client
+    to use that cipher.  Data channel cipher negotiation can be controlled
+    using --ncp-ciphers and --ncp-disable.
+
 
 User-visible Changes
 --------------------
@@ -106,6 +113,10 @@ User-visible Changes
 - mbed TLS builds: minimum RSA key size is now 2048 bits.  Shorter keys will
   not be accepted, both local and from the peer.
 
+- Data channel cipher negotiation (see New features section) can override
+  ciphers configured in the config file.  Use --ncp-disable if you don't want
+  that.
+
 
 Maintainer-visible changes
 --------------------------
doc/openvpn.8 +4 -0
index e349b77..5cb2885 100644
@@ -4134,6 +4134,10 @@ Restrict the allowed ciphers to be negotiated to the ciphers in
 .B cipher_list
 is a colon-separated list of ciphers, and defaults to
 "AES-256-GCM:AES-128-GCM".
+
+For servers, the first cipher from
+.B cipher_list
+will be pushed to clients that support cipher negotiation.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +3 -2
index b4efaf0..eb40e5b 100644
@@ -2314,8 +2314,8 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters (postpone if we pull options) */
-  if (c->options.pull)
+  /* Compute MTU parameters (postpone if we push/pull options) */
+  if (c->options.pull || c->options.mode == MODE_SERVER)
     {
       /* Account for worst-case crypto overhead before allocating buffers */
       frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
@@ -2359,6 +2359,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.mode = options->mode;
   to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
src/openvpn/push.c +25 -10
index 4239e3e..000c82f 100644
@@ -245,13 +245,30 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  if (!do_up (c, true, c->options.push_option_types_found))
 	    {
 	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
-	      register_signal (c, SIGUSR1, "do_up-failed");
-	      goto cleanup;
+	      goto error;
 	    }
 	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
+  else if (status == PUSH_MSG_REQUEST)
+    {
+      if (c->options.mode == MODE_SERVER)
+	{
+	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+	  /* Do not regenerate keys if client send a second push request */
+	  if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
+	      !tls_session_update_crypto_params (session, &c->options,
+		  &c->c2.frame))
+	    {
+	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      goto error;
+	    }
+	}
+    }
 
+  goto cleanup;
+error:
+  register_signal (c, SIGUSR1, "process-push-msg-failed");
 cleanup:
   gc_free (&gc);
 }
@@ -302,15 +319,13 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
     }
 
   /* Push cipher if client supports Negotiable Crypto Parameters */
-  optstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-  if (optstr)
+  if (tls_peer_info_ncp_ver (peer_info) >= 2 && o->ncp_enabled)
     {
-      int ncp = 0;
-      int r = sscanf(optstr, "IV_NCP=%d", &ncp);
-      if ((r == 1) && (ncp == 2))
-	{
-	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
-	}
+      /* Push the first cipher from --ncp-ciphers to the client.
+       * TODO: actual negotiation, instead of server dictatorship. */
+      char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
+      o->ciphername = strtok (push_cipher, ":");
+      push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
     }
   return true;
 }
src/openvpn/ssl.c +25 -12
index 124d458..baf4085 100644
@@ -1661,10 +1661,10 @@ tls_session_update_crypto_params(struct tls_session *session,
   bool ret = false;
   struct key_state *ks = &session->key[KS_PRIMARY];	/* primary key */
 
-  ASSERT (!session->opt->server);
   ASSERT (ks->authenticated);
 
-  if (0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
+  if (!session->opt->server &&
+      0 != strcmp(options->ciphername, session->opt->config_ciphername) &&
       !item_in_list(options->ciphername, options->ncp_ciphers))
     {
       msg (D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
@@ -1691,12 +1691,13 @@ tls_session_update_crypto_params(struct tls_session *session,
       options->ce.tun_mtu_defined, options->ce.tun_mtu);
   frame_print (frame, D_MTU_INFO, "Data Channel MTU parms");
 
+  const struct session_id *client_sid = session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
+  const struct session_id *server_sid = !session->opt->server ?
+      &ks->session_id_remote : &session->session_id;
   if (!generate_key_expansion (&ks->crypto_options.key_ctx_bi,
-			       &session->opt->key_type,
-			       ks->key_src,
-			       &session->session_id,
-			       &ks->session_id_remote,
-			       false))
+      &session->opt->key_type, ks->key_src, client_sid, server_sid,
+      session->opt->server))
     {
       msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
       goto cleanup;
@@ -1954,8 +1955,11 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
       buf_printf(&out, "IV_PROTO=2\n");
 
       /* support for Negotiable Crypto Paramters */
-      if (session->opt->ncp_enabled && session->opt->pull)
-	buf_printf(&out, "IV_NCP=2\n");
+      if (session->opt->ncp_enabled &&
+	  (session->opt->mode == MODE_SERVER || session->opt->pull))
+	{
+	  buf_printf(&out, "IV_NCP=2\n");
+	}
 
       /* push compression status */
 #ifdef USE_COMP
@@ -2057,10 +2061,13 @@ key_method_2_write (struct buffer *buf, struct tls_session *session)
   if (!push_peer_info (buf, session))
     goto error;
 
-  /*
-   * generate tunnel keys if server
+  /* Generate tunnel keys if we're a TLS server.
+   * If we're a p2mp server and IV_NCP >= 2 is negotiated, the first key
+   * generation is postponed until after the pull/push, so we can process pushed
+   * cipher directives.
    */
-  if (session->opt->server)
+  if (session->opt->server && !(session->opt->ncp_enabled &&
+	session->opt->mode == MODE_SERVER && ks->key_id <= 0))
     {
       if (ks->authenticated)
 	{
@@ -2214,6 +2221,12 @@ key_method_2_read (struct buffer *buf, struct tls_multi *multi, struct tls_sessi
       output_peer_info_env (session->opt->es, multi->peer_info);
 #endif
 
+  if (tls_peer_info_ncp_ver (multi->peer_info) < 2)
+    {
+      /* Peer does not support NCP */
+      session->opt->ncp_enabled = false;
+    }
+
   if (tls_session_user_pass_enabled(session))
     {
       /* Perform username/password authentication */
src/openvpn/ssl_common.h +1 -0
index 6bfde6b..eb2ad6f 100644
@@ -236,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  int mode;
   bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
Signature · 1 line
2.7.4
#30 lore raw

Re: [Openvpn-devel] [PATCHv3 5/5] Add server-side support for cipher negotiation

Hi,

Quoted 4 lines · Steffan Karger

Pushes AES-256-GCM when a connecting client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

Quoted 8 lines

v2:
* Update manpage
* Add Changes.rst entry

v3:
* Do not regenerate keys if the client sends a second pull request
* Don't postpone key generation if client has no IV_NCP support (to also support clients that do not send a push request)

To me this looks generally OK.

I'll do 3/5 and 4/5 next, but would then ask for a v4 without the merge/rebase/fix/rewort artefacts we already know about :-)

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]...
#31 lore raw

PATCH v4 5/5 Add server-side support for cipher negotiation

Pushes AES-256-GCM when a connection client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

v2:
* Update manpage
* Add Changes.rst entry

v3:
* Do not regenerate keys if the client sends a second pull request
* Don't postpone key generation if client has no IV_NCP support

v4:
* rebase on client-side NCP v4

---
 Changes.rst              | 11 +++++++++++
 doc/openvpn.8            |  4 ++++
 src/openvpn/init.c       |  5 +++--
 src/openvpn/push.c       | 35 +++++++++++++++++++++++----------
 src/openvpn/ssl.c        | 51 ++++++++++++++++++++++++++++++++++++------------
 src/openvpn/ssl.h        |  6 ++++++
 src/openvpn/ssl_common.h |  1 +
 7 files changed, 89 insertions(+), 24 deletions(-)
Changes.rst +11 -0
index 1ac3c2b..eb6018f 100644
@@ -51,6 +51,13 @@ AEAD (GCM) data channel cipher support
     bytes per packet for AES-128-GCM instead of 36 bytes per packet for
     AES-128-CBC + HMAC-SHA1).
 
+Cipher negotiation
+    Data channel ciphers are now by default negotiated.  If a client advertises
+    support for Negotiable Crypto Parameters (NCP), the server will choose a
+    cipher (by default AES-256-GCM) for the data channel, and tell the client
+    to use that cipher.  Data channel cipher negotiation can be controlled
+    using --ncp-ciphers and --ncp-disable.
+
 
 User-visible Changes
 --------------------
@@ -106,6 +113,10 @@ User-visible Changes
 - mbed TLS builds: minimum RSA key size is now 2048 bits.  Shorter keys will
   not be accepted, both local and from the peer.
 
+- Data channel cipher negotiation (see New features section) can override
+  ciphers configured in the config file.  Use --ncp-disable if you don't want
+  that.
+
 
 Maintainer-visible changes
 --------------------------
doc/openvpn.8 +4 -0
index e349b77..5cb2885 100644
@@ -4134,6 +4134,10 @@ Restrict the allowed ciphers to be negotiated to the ciphers in
 .B cipher_list
 is a colon-separated list of ciphers, and defaults to
 "AES-256-GCM:AES-128-GCM".
+
+For servers, the first cipher from
+.B cipher_list
+will be pushed to clients that support cipher negotiation.
 .\"*********************************************************
 .TP
 .B \-\-ncp\-disable
src/openvpn/init.c +3 -2
index b4efaf0..eb40e5b 100644
@@ -2314,8 +2314,8 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   /* In short form, unique datagram identifier is 32 bits, in long form 64 bits */
   packet_id_long_form = cipher_kt_mode_ofb_cfb (c->c1.ks.key_type.cipher);
 
-  /* Compute MTU parameters (postpone if we pull options) */
-  if (c->options.pull)
+  /* Compute MTU parameters (postpone if we push/pull options) */
+  if (c->options.pull || c->options.mode == MODE_SERVER)
     {
       /* Account for worst-case crypto overhead before allocating buffers */
       frame_add_to_extra_frame (&c->c2.frame, crypto_max_overhead());
@@ -2359,6 +2359,7 @@ do_init_crypto_tls (struct context *c, const unsigned int flags)
   to.renegotiate_packets = options->renegotiate_packets;
   to.renegotiate_seconds = options->renegotiate_seconds;
   to.single_session = options->single_session;
+  to.mode = options->mode;
   to.pull = options->pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   if (options->push_peer_info)		/* all there is */
src/openvpn/push.c +25 -10
index 4239e3e..000c82f 100644
@@ -245,13 +245,30 @@ incoming_push_message (struct context *c, const struct buffer *buffer)
 	  if (!do_up (c, true, c->options.push_option_types_found))
 	    {
 	      msg (D_PUSH_ERRORS, "Failed to open tun/tap interface");
-	      register_signal (c, SIGUSR1, "do_up-failed");
-	      goto cleanup;
+	      goto error;
 	    }
 	}
       event_timeout_clear (&c->c2.push_request_interval);
     }
+  else if (status == PUSH_MSG_REQUEST)
+    {
+      if (c->options.mode == MODE_SERVER)
+	{
+	  struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE];
+	  /* Do not regenerate keys if client send a second push request */
+	  if (!session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized &&
+	      !tls_session_update_crypto_params (session, &c->options,
+		  &c->c2.frame))
+	    {
+	      msg (D_TLS_ERRORS, "TLS Error: server generate_key_expansion failed");
+	      goto error;
+	    }
+	}
+    }
 
+  goto cleanup;
+error:
+  register_signal (c, SIGUSR1, "process-push-msg-failed");
 cleanup:
   gc_free (&gc);
 }
@@ -302,15 +319,13 @@ prepare_push_reply (struct options *o, struct tls_multi *tls_multi)
     }
 
   /* Push cipher if client supports Negotiable Crypto Parameters */
-  optstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL;
-  if (optstr)
+  if (tls_peer_info_ncp_ver (peer_info) >= 2 && o->ncp_enabled)
     {
-      int ncp = 0;
-      int r = sscanf(optstr, "IV_NCP=%d", &ncp);
-      if ((r == 1) && (ncp == 2))
-	{
-	  push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
-	}
+      /* Push the first cipher from --ncp-ciphers to the client.
+       * TODO: actual negotiation, instead of server dictatorship. */
+      char *push_cipher = string_alloc(o->ncp_ciphers, &o->gc);
+      o->ciphername = strtok (push_cipher, ":");
+      push_option_fmt(o, M_USAGE, "cipher %s", o->ciphername);
     }
   return true;
 }
src/openvpn/ssl.c +39 -12

91 lines not on this page · raw message

src/openvpn/ssl.h +6 -0
index 416f426..de68b69 100644
@@ -497,6 +497,12 @@ tls_get_peer_info(const struct tls_multi *multi)
 }
 #endif
 
+/**
+ * Return the Negotiable Crypto Parameters version advertised in the peer info
+ * string, or 0 if none specified.
+ */
+int tls_peer_info_ncp_ver(const char *peer_info);
+
 /*
  * inline functions
  */
src/openvpn/ssl_common.h +1 -0
index 6bfde6b..eb2ad6f 100644
@@ -236,6 +236,7 @@ struct tls_options
 #ifdef ENABLE_OCC
   bool disable_occ;
 #endif
+  int mode;
   bool pull;
 #ifdef ENABLE_PUSH_PEER_INFO
   int push_peer_info_detail;
Signature · 1 line
2.7.4
#32 lore raw

Re: [Openvpn-devel] [PATCH 5/5 v4] Add server-side support for cipher negotiation

Am 28.06.16 um 23:36 schrieb Steffan Karger:

Quoted 8 lines

Pushes AES-256-GCM when a connection client advertises IV_NCP=2, and supports serving connections to clients with different data channel cipher configuration simultaneously.

v2:
* Update manpage
* Add Changes.rst entry

Quoted 7 lines

v3:
* Do not regenerate keys if the client sends a second pull request
* Don't postpone key generation if client has no IV_NCP support

v4:
* rebase on client-side NCP v4

ACK from me aswell.

Arne

#33 lore raw

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

In reply to Steffan Karger

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
Date:   Tue Jun 28 23:36:11 2016 +0200
     Add server-side support for cipher negotiation

     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: http://article.gmane.org/gmane.network.openvpn.devel/12009
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

#34 lore raw
In reply to Steffan Karger
Quoted 4 lines · Steffan Karger

Please test and provide comments. I ran the 'make check' test suite, including t_client tests, and performed manual testing. But, as Gert eloquently puts it, this is likely to 'break stuff in interesting ways', so more testing (and review) is very welcome!

For those of you that rather check out a github branch instead of applying patches, I pushed my branch - plus some extra debug commits - to https://github.com/syzzer/openvpn/tree/nego-squashed

-Steffan

#35 lore raw

Hi

Quoted 8 lines · Steffan Karger
Quoted 4 lines · Steffan Karger

Please test and provide comments. I ran the 'make check' test suite, including t_client tests, and performed manual testing. But, as Gert eloquently puts it, this is likely to 'break stuff in interesting ways', so more testing (and review) is very welcome!

For those of you that rather check out a github branch instead of applying patches, I pushed my branch - plus some extra debug commits - to https://github.com/syzzer/openvpn/tree/nego-squashed

I decided to give this a go from git .. but here snappy is still used. I am happy to get snappy myself but I thought snappy had been dropped ? thanks

#36 lore raw
Quoted 8 lines · On 08.06.2016 17:09, debbie10t wrote:

Hi

Quoted 5 lines · Steffan Karger
Quoted 4 lines · Steffan Karger

Please test and provide comments. I ran the 'make check' test suite, including t_client tests, and performed manual testing. But, as Gert eloquently puts it, this is likely to 'break stuff in interesting ways', so more testing (and review) is very welcome!

Quoted 7 lines
Quoted 3 lines

For those of you that rather check out a github branch instead of applying patches, I pushed my branch - plus some extra debug commits - to https://github.com/syzzer/openvpn/tree/nego-squashed

I decided to give this a go from git .. but here snappy is still used. I am happy to get snappy myself but I thought snappy had been dropped ? thanks

Since it is on git master there is no snappy in there, can you double check that you used the right branch and not some other outdated branch of syzzer repo?

Arne

#37 lore raw
Quoted 12 lines · Arne Schwabe
Quoted 11 lines · On 08.06.2016 17:09, debbie10t wrote:

Hi

Quoted 8 lines · Steffan Karger
Quoted 4 lines · Steffan Karger

Please test and provide comments. I ran the 'make check' test suite, including t_client tests, and performed manual testing. But, as Gert eloquently puts it, this is likely to 'break stuff in interesting ways', so more testing (and review) is very welcome!

For those of you that rather check out a github branch instead of applying patches, I pushed my branch - plus some extra debug commits - to https://github.com/syzzer/openvpn/tree/nego-squashed

Quoted 8 lines
Quoted 4 lines

I decided to give this a go from git .. but here snappy is still used. I am happy to get snappy myself but I thought snappy had been dropped ? thanks

Since it is on git master there is no snappy in there, can you double check that you used the right branch and not some other outdated branch of syzzer repo?

First downloaded the wrong repo .. Not sure what I did wrong .. nvm

Second: This is where my git foo falls down .. tried:

$ git clone https://github.com/syzzer/openvpn/tree/nego-squashed
Cloning into 'nego-squashed'...
fatal: repository 'https://github.com/syzzer/openvpn/tree/nego-squashed/' not found

Finally i downloaded https://github.com/syzzer/openvpn/archive/nego-squashed.zip
built ok .. (no snappy)

Sorry to waste your time and thanks for you help

#38 lore raw
Quoted 8 lines · On Wed, Jun 8, 2016 at 6:13 PM, debbie10t <[email protected]...> wrote:

First downloaded the wrong repo .. Not sure what I did wrong .. nvm

Second: This is where my git foo falls down .. tried:

$ git clone https://github.com/syzzer/openvpn/tree/nego-squashed
Cloning into 'nego-squashed'...
fatal: repository
'https://github.com/syzzer/openvpn/tree/nego-squashed/' not found
Quoted 5 lines

Finally i downloaded
https://github.com/syzzer/openvpn/archive/nego-squashed.zip
built ok .. (no snappy)

Sorry to waste your time and thanks for you help

For future reference, you should have done:

   git clone https://github.com/syzzer/openvpn --branch nego-squashed

-Steffan

#39 lore raw
Quoted 13 lines · Steffan Karger
Quoted 12 lines · On Wed, Jun 8, 2016 at 6:13 PM, debbie10t <[email protected]...> wrote:

First downloaded the wrong repo .. Not sure what I did wrong .. nvm

Second: This is where my git foo falls down .. tried:

$ git clone https://github.com/syzzer/openvpn/tree/nego-squashed
Cloning into 'nego-squashed'...
fatal: repository
'https://github.com/syzzer/openvpn/tree/nego-squashed/' not found

Finally i downloaded
https://github.com/syzzer/openvpn/archive/nego-squashed.zip
built ok .. (no snappy)

Quoted 6 lines
Quoted 1 line

Sorry to waste your time and thanks for you help

For future reference, you should have done:

    git clone https://github.com/syzzer/openvpn --branch nego-squashed

-Steffan

thank you :-) ( RTGM )