Browse code

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

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 <steffan@karger.me>
Acked-by: David Sommerseth <davids@openvpn.net>
Message-Id: <1481838366-32335-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13579.html
Signed-off-by: David Sommerseth <davids@openvpn.net>

Steffan Karger authored on 2016/12/16 06:46:06
Showing 4 changed files
... ...
@@ -923,6 +923,14 @@ const char *string_mod_const(const char *str,
923 923
 
924 924
 void string_replace_leading(char *str, const char match, const char replace);
925 925
 
926
+/** Return true iff str starts with prefix */
927
+static inline bool
928
+strprefix(const char *str, const char *prefix)
929
+{
930
+    return 0 == strncmp(str, prefix, strlen(prefix));
931
+}
932
+
933
+
926 934
 #ifdef CHARACTER_CLASS_DEBUG
927 935
 void character_class_debug(void);
928 936
 
... ...
@@ -677,17 +677,23 @@ process_incoming_push_request(struct context *c)
677 677
 #endif /* if P2MP_SERVER */
678 678
 
679 679
 static void
680
-push_update_digest(md_ctx_t *ctx, struct buffer *buf)
680
+push_update_digest(md_ctx_t *ctx, struct buffer *buf, const struct options *opt)
681 681
 {
682 682
     char line[OPTION_PARM_SIZE];
683 683
     while (buf_parse(buf, ',', line, sizeof(line)))
684 684
     {
685 685
         /* peer-id might change on restart and this should not trigger reopening tun */
686
-        if (strstr(line, "peer-id ") != line)
686
+        if (strprefix(line, "peer-id "))
687 687
         {
688
-            md_ctx_update(ctx, (const uint8_t *) line, strlen(line));
688
+            continue;
689
+        }
690
+        /* tun reopen only needed if cipher change can change tun MTU */
691
+        if (strprefix(line, "cipher ") && !opt->ce.tun_mtu_defined)
692
+        {
693
+            continue;
689 694
         }
690 695
     }
696
+    md_ctx_update(ctx, (const uint8_t *) line, strlen(line)+1);
691 697
 }
692 698
 
693 699
 int
... ...
@@ -730,7 +736,8 @@ process_incoming_push_msg(struct context *c,
730 730
                                    option_types_found,
731 731
                                    c->c2.es))
732 732
             {
733
-                push_update_digest(&c->c2.pulled_options_state, &buf_orig);
733
+                push_update_digest(&c->c2.pulled_options_state, &buf_orig,
734
+                                   &c->options);
734 735
                 switch (c->options.push_continuation)
735 736
                 {
736 737
                     case 0:
... ...
@@ -1,6 +1,6 @@
1 1
 AUTOMAKE_OPTIONS = foreign
2 2
 
3
-check_PROGRAMS = argv_testdriver
3
+check_PROGRAMS = argv_testdriver buffer_testdriver
4 4
 
5 5
 if ENABLE_CRYPTO
6 6
 check_PROGRAMS += tls_crypt_testdriver
... ...
@@ -21,6 +21,12 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.c \
21 21
 	$(openvpn_srcdir)/buffer.c \
22 22
 	$(openvpn_srcdir)/argv.c
23 23
 
24
+buffer_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir)
25
+buffer_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line
26
+buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
27
+	$(openvpn_srcdir)/buffer.c \
28
+	$(openvpn_srcdir)/platform.c
29
+
24 30
 tls_crypt_testdriver_CFLAGS  = @TEST_CFLAGS@ \
25 31
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
26 32
 	$(OPTIONAL_CRYPTO_CFLAGS)
27 33
new file mode 100644
... ...
@@ -0,0 +1,56 @@
0
+/*
1
+ *  OpenVPN -- An application to securely tunnel IP networks
2
+ *             over a single UDP port, with support for SSL/TLS-based
3
+ *             session authentication and key exchange,
4
+ *             packet encryption, packet authentication, and
5
+ *             packet compression.
6
+ *
7
+ *  Copyright (C) 2016 Fox Crypto B.V. <openvpn@fox-it.com>
8
+ *
9
+ *  This program is free software; you can redistribute it and/or modify
10
+ *  it under the terms of the GNU General Public License version 2
11
+ *  as published by the Free Software Foundation.
12
+ *
13
+ *  This program is distributed in the hope that it will be useful,
14
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
15
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
16
+ *  GNU General Public License for more details.
17
+ *
18
+ *  You should have received a copy of the GNU General Public License
19
+ *  along with this program (see the file COPYING included with this
20
+ *  distribution); if not, write to the Free Software Foundation, Inc.,
21
+ *  59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
22
+ */
23
+
24
+#ifdef HAVE_CONFIG_H
25
+#include "config.h"
26
+#elif defined(_MSC_VER)
27
+#include "config-msvc.h"
28
+#endif
29
+
30
+#include "syshead.h"
31
+
32
+#include <setjmp.h>
33
+#include <cmocka.h>
34
+
35
+#include "buffer.h"
36
+
37
+static void
38
+buffer_strprefix(void **state)
39
+{
40
+    assert_true(strprefix("123456", "123456"));
41
+    assert_true(strprefix("123456", "123"));
42
+    assert_true(strprefix("123456", ""));
43
+    assert_false(strprefix("123456", "456"));
44
+    assert_false(strprefix("12", "123"));
45
+}
46
+
47
+int
48
+main(void)
49
+{
50
+    const struct CMUnitTest tests[] = {
51
+        cmocka_unit_test(buffer_strprefix),
52
+    };
53
+
54
+    return cmocka_run_group_tests_name("buffer", tests, NULL, NULL);
55
+}