Don't reopen tun if cipher changes

PATCH Superseded Steffan Karger 2 participants 4 messages Dec 15, 2016

Share & tools

Steffan Karger

author

2016-12-15 18:52 UTC #0 lore raw

PATCH 1/1 Don't reopen tun if cipher changes

When the pulled options change, OpenVPN will attempt to reopen the tun device. That might fail if the process has already dropper privileges, and is not needed unless the tun MTU is changed. This patch therefore ignores the cipher value for the digest if a fixed tun-mtu is used.

Additionally, this patch changes the md_ctx_update() call to include the trailing zero byte of each option, to make sure that parsing "foo,bar" results in a different hash than "foobar". (Sorry for not catching that during the review...)

Trac: #761

Signed-off-by Steffan Karger <[email protected]...>

---
 src/openvpn/push.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

src/openvpn/push.c +11 -5

index 34c65c4..674efeb 100644
@@ -677,17 +677,22 @@ process_incoming_push_request(struct context *c)
 #endif /* if P2MP_SERVER */
 
 static void
-push_update_digest(md_ctx_t *ctx, struct buffer *buf)
+push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
 {
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
         /* peer-id might change on restart and this should not trigger reopening tun */
-        if (strstr(line, "peer-id ") != line)
+        if (strcmp(line, "peer-id ") == 0)
         {
-            md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
+            continue;
         }
-    }
+        if (strcmp(line, "cipher ") == 0 && !opt->ce.tun_mtu_defined)
+        {
+            continue;
+        }
+	}
+    md_ctx_update(ctx, (const uint8_t *) line, strlen(line)+1);
 }
 
 int

@@ -730,7 +735,8 @@ process_incoming_push_msg(struct context *c,
                                    option_types_found,
                                    c->c2.es))
             {
-                push_update_digest(&c->c2.pulled_options_state, &buf_orig);
+                push_update_digest(&c->c2.pulled_options_state, &buf_orig,
+                                   &c->options);
                 switch (c->options.push_continuation)
                 {
                     case 0:

Signature · 1 line

2.7.4

David Sommerseth

2016-12-15 19:49 UTC #1 lore raw

Quoted 24 lines · Steffan Karger

When the pulled options change, OpenVPN will attempt to reopen the tun device. That might fail if the process has already dropper privileges, and is not needed unless the tun MTU is changed. This patch therefore ignores the cipher value for the digest if a fixed tun-mtu is used.
Additionally, this patch changes the md_ctx_update() call to include the trailing zero byte of each option, to make sure that parsing "foo,bar" results in a different hash than "foobar". (Sorry for not catching that during the review...)
Trac: #761
Signed-off-by Steffan Karger <[email protected]...>
---
 src/openvpn/push.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)
src/openvpn/push.c +0 -0
index 34c65c4..674efeb 100644
@@ -677,17 +677,22 @@ process_incoming_push_request(struct context *c)
 #endif /* if P2MP_SERVER */
 

src/openvpn/push.c process_incoming_push_request(struct context *c) · line 686

Quoted 10 lines

src/openvpn/push.c +2 -2

 static void
-push_update_digest(md_ctx_t *ctx, struct buffer *buf)
+push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
 {
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
         /* peer-id might change on restart and this should not trigger reopening tun */
-        if (strstr(line, "peer-id ") != line)
+        if (strcmp(line, "peer-id ") == 0)

Are you sure about this strcmp()? I'd expect line to contain "peer-id 1" or something like that ... so the result would be 49 (or anything not 0) .... strncmp(line, "peer-id ", 8) would provide a match though.

src/openvpn/push.c process_incoming_push_request(struct context *c) · line 690

Quoted 6 lines

src/openvpn/push.c +2 -2

         {
-            md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
+            continue;
         }
-    }
+        if (strcmp(line, "cipher ") == 0 && !opt->ce.tun_mtu_defined)

Same as above.

Signature · 4 lines

kind regards,

David Sommerseth
OpenVPN Technologies, Inc

Steffan Karger

author

2016-12-15 21:43 UTC #2 lore raw

Hi,

Quoted 8 lines · David Sommerseth

src/openvpn/push.c process_incoming_push_request(struct context *c) · line 686
Quoted 2 lines · Steffan Karger
src/openvpn/push.c +1 -1
-        if (strstr(line, "peer-id ") != line)
+        if (strcmp(line, "peer-id ") == 0)
Are you sure about this strcmp()? I'd expect line to contain "peer-id 1" or something like that ... so the result would be 49 (or anything not 0) .... strncmp(line, "peer-id ", 8) would provide a match though.

Hrmpf, you're absolutely right. That's what I get for rushing out a patch. I'll send a v2 shortly.

-Steffan

Steffan Karger

author

2016-12-15 21:46 UTC #3 lore raw

PATCH v2 1/1 Don't reopen tun if cipher changes

In reply to Steffan Karger

The unit tests are a bit lame, but it secretly serves as a way to lower the bar for adding more buffer.c unit tests.

Trac: #761

Signed-off-by Steffan Karger <[email protected]...>

---
v2: check for prefix properly, add unit tests for prefix check.

 src/openvpn/buffer.h                   |  8 +++++
 src/openvpn/push.c                     | 15 ++++++---
 tests/unit_tests/openvpn/Makefile.am   |  8 ++++-
 tests/unit_tests/openvpn/test_buffer.c | 56 ++++++++++++++++++++++++++++++++++
 4 files changed, 82 insertions(+), 5 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/test_buffer.c

src/openvpn/buffer.h +8 -0

index 6bd5e20..a1a130c 100644
@@ -923,6 +923,14 @@ const char *string_mod_const(const char *str,
 
 void string_replace_leading(char *str, const char match, const char replace);
 
+/** Return true iff str starts with prefix */
+static inline bool
+strprefix(const char *str, const char *prefix)
+{
+    return 0 == strncmp(str, prefix, strlen(prefix));
+}
+
+
 #ifdef CHARACTER_CLASS_DEBUG
 void character_class_debug(void);

src/openvpn/push.c +11 -4

index 34c65c4..77adf84 100644
@@ -677,17 +677,23 @@ process_incoming_push_request(struct context *c)
 #endif /* if P2MP_SERVER */
 
 static void
-push_update_digest(md_ctx_t *ctx, struct buffer *buf)
+push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
 {
     char line[OPTION_PARM_SIZE];
     while (buf_parse(buf, ',', line, sizeof(line)))
     {
         /* peer-id might change on restart and this should not trigger reopening tun */
-        if (strstr(line, "peer-id ") != line)
+        if (strprefix(line, "peer-id "))
         {
-            md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
+            continue;
+        }
+        /* tun reopen only needed if cipher change can change tun MTU */
+        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
+        {
+            continue;
         }
     }
+    md_ctx_update(ctx, (const uint8_t *) line, strlen(line)+1);
 }
 
 int

@@ -730,7 +736,8 @@ process_incoming_push_msg(struct context *c,
                                    option_types_found,
                                    c->c2.es))
             {
-                push_update_digest(&c->c2.pulled_options_state, &buf_orig);
+                push_update_digest(&c->c2.pulled_options_state, &buf_orig,
+                                   &c->options);
                 switch (c->options.push_continuation)
                 {
                     case 0:

tests/unit_tests/openvpn/Makefile.am +7 -1

index 632ff58..fafe6b2 100644
@@ -1,6 +1,6 @@
 AUTOMAKE_OPTIONS = foreign
 
-check_PROGRAMS = argv_testdriver
+check_PROGRAMS = argv_testdriver buffer_testdriver
 
 if ENABLE_CRYPTO
 check_PROGRAMS += tls_crypt_testdriver

@@ -21,6 +21,12 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/argv.c
 
+buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir)
+buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line
+buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/platform.c
+
 tls_crypt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)

tests/unit_tests/openvpn/test_buffer.c new file +56 -0

56 lines not on this page · raw message

Signature · 1 line

2.7.4