Client-specific tls-crypt keys (--tls-crypt-v2)

Needed for tls-crypt-v2, but isolated enough to be reviewed as a separate patch.

The encode API allocates memory, because it fits our typical gc-oriented code pattern and the caller does not have to do multiple calls or calculations to determine the required destination buffer size.

The decode API does not allocate memory, because the required destination buffer is always smaller than the input buffer (so is easy to manage by the caller) and does not force the caller to use the heap.

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

---
 src/openvpn/crypto_backend.h           | 30 +++++++++++
 src/openvpn/crypto_mbedtls.c           | 74 +++++++++++++++++++++++++++
 src/openvpn/crypto_openssl.c           | 82 ++++++++++++++++++++++++++++++
 tests/unit_tests/openvpn/Makefile.am   | 16 +++++-
 tests/unit_tests/openvpn/test_crypto.c | 92 ++++++++++++++++++++++++++++++++++
 5 files changed, 293 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit_tests/openvpn/test_crypto.c

src/openvpn/crypto_backend.h +1 -0

index 567fd9b..83e14c8 100644
@@ -36,6 +36,7 @@
 #include "crypto_mbedtls.h"
 #endif
 #include "basic.h"
+#include "buffer.h"
 
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16

@@ -105,6 +106,35 @@ void show_available_digests(void);
 

src/openvpn/crypto_backend.h +21 -0

+ * @param src       Source buffer
+ * @param gc        The garbage collector to use when allocating memory for
+ *                  dst.
+ *
+ * @return true iff PEM encode succeeded.
+ */
+bool crypto_pem_encode(const char *name, struct buffer *dst,
+                       const struct buffer *src, struct gc_arena *gc);
+
+/**
+ * Decode a PEM buffer to binary data
+ *
+ * @param name      The name expected in the PEM header/footer
+ * @param dst       Destination buffer for decoded data
+ * @param src       Source buffer (PEM data)
+ *
+ * @return true iff PEM decode succeeded.
+ */
+bool crypto_pem_decode(const char *name, struct buffer *dst,
+                       const struct buffer *src);
+
 /*
  *
  * Random number functions, used in cases where we want

src/openvpn/crypto_mbedtls.c +21 -0

index 8fa03da..1e86854 100644
@@ -44,11 +44,13 @@
 #include "otime.h"
 #include "misc.h"
 
+#include <mbedtls/base64.h>
 #include <mbedtls/des.h>
 #include <mbedtls/error.h>
 #include <mbedtls/md5.h>
 #include <mbedtls/cipher.h>
 #include <mbedtls/havege.h>
+#include <mbedtls/pem.h>
 
 #include <mbedtls/entropy.h>
 

@@ -229,6 +231,78 @@ show_available_engines(void)
            "available\n");
 }
 
+bool
+crypto_pem_encode(const char *name, struct buffer *dst,
+                  const struct buffer *src, struct gc_arena *gc)
+{
+    /* 1000 chars is the PEM line length limit (+1 for tailing NUL) */
+    char header[1000+1] = { 0 };
+    char footer[1000+1] = { 0 };
+
+    if (!openvpn_snprintf(header, sizeof(header), "-----BEGIN %s-----\n", name))
+    {
+        return false;
+    }
+    if (!openvpn_snprintf(footer, sizeof(footer), "-----END %s-----\n", name))
+    {
+        return false;
+    }
+
+    size_t out_len = 0;
+    if (MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL !=

src/openvpn/crypto_mbedtls.c +43 -0

+    {
+        return false;
+    }
+
+    return true;
+}
+
+bool
+crypto_pem_decode(const char *name, struct buffer *dst,
+                  const struct buffer *src)
+{
+    /* 1000 chars is the PEM line length limit (+1 for tailing NUL) */
+    char header[1000+1] = { 0 };
+    char footer[1000+1] = { 0 };
+
+    if (*(BLAST(src)) != '\0')
+    {
+        msg(M_WARN, "PEM decode error: source buffer not null-terminated");
+        return false;
+    }
+    if (!openvpn_snprintf(header, sizeof(header), "-----BEGIN %s-----", name))
+    {
+        return false;
+    }
+    if (!openvpn_snprintf(footer, sizeof(footer), "-----END %s-----", name))
+    {
+        return false;
+    }
+
+    size_t use_len = 0;
+    mbedtls_pem_context ctx = { 0 };
+    bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer, BPTR(src),
+                                               NULL, 0, &use_len));
+    if (ret && !buf_write(dst, ctx.buf, ctx.buflen))
+    {
+        ret = false;
+        msg(M_WARN, "PEM decode error: destination buffer too small");
+    }
+
+    mbedtls_pem_free(&ctx);
+    return ret;
+}
+
 /*
  *
  * Random number functions, used in cases where we want

src/openvpn/crypto_openssl.c +0 -0

index 20a519e..49d3aeb 100644

src/openvpn/crypto_openssl.c +64 -0

+
+    ret = true;
+cleanup:
+    if (!BIO_free(bio))
+    {
+        ret = false;;
+    }
+
+    return ret;
+}
+
+bool
+crypto_pem_decode(const char *name, struct buffer *dst,
+                  const struct buffer *src)
+{
+    bool ret = false;
+    BIO *bio;
+
+    if (!(bio = BIO_new_mem_buf((char *)BPTR(src), BLEN(src))))
+    {
+        crypto_msg(M_FATAL, "Cannot open memory BIO for PEM decode");
 
+    }
+
+    char *name_read = NULL;
+    char *header_read = NULL;
+    uint8_t *data_read = NULL;
+    long data_read_len = 0;
+    if (!PEM_read_bio(bio, &name_read, &header_read, &data_read,
+                      &data_read_len))
+    {
+        dmsg(D_CRYPT_ERRORS, "%s: PEM decode failed", __func__);
+        goto cleanup;
+    }
+
+    if (strcmp(name, name_read))
+    {
+        dmsg(D_CRYPT_ERRORS,
+             "%s: unexpected PEM name (got '%s', expected '%s')",
+             __func__, name_read, name);
+        goto cleanup;
+    }
+
+    uint8_t *dst_data = buf_write_alloc(dst, data_read_len);
+    if (!dst_data)
+    {
+        dmsg(D_CRYPT_ERRORS, "%s: dst too small (%i, needs %li)", __func__,
+             BCAP(dst), data_read_len);
+        goto cleanup;
+    }
+    memcpy(dst_data, data_read, data_read_len);
+
+    ret = true;
+cleanup:
+    OPENSSL_free(name_read);
+    OPENSSL_free(header_read);
+    OPENSSL_free(data_read);
+    if (!BIO_free(bio))
+    {
+        ret = false;;
+    }
+
+    return ret;
+}
+
 /*
  *

* Random number functions, used in cases where we want

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

index 23d758b..d100c21 100644
@@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT
 check_PROGRAMS += argv_testdriver buffer_testdriver
 endif
 
-check_PROGRAMS += packet_id_testdriver tls_crypt_testdriver
+check_PROGRAMS += crypto_testdriver packet_id_testdriver tls_crypt_testdriver
 
 TESTS = $(check_PROGRAMS)
 

@@ -29,6 +29,20 @@ buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/platform.c
 
+crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+crypto_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+crypto_testdriver_SOURCES = test_crypto.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/crypto.c \
+	$(openvpn_srcdir)/crypto_mbedtls.c \
+	$(openvpn_srcdir)/crypto_openssl.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
+
 packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)

tests/unit_tests/openvpn/test_crypto.c new file +0 -0

new file mode 100644
index 0000000..62d5b3f

tests/unit_tests/openvpn/test_crypto.c new file +25 -0

+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>

tests/unit_tests/openvpn/test_crypto.c new file +9 -0

+static void
+crypto_pem_encode_decode_loopback(void **state) {
+    struct gc_arena gc = gc_new();
+    struct buffer src_buf;
+    buf_set_read(&src_buf, (void *)testtext, sizeof(testtext));
+
+    uint8_t dec[sizeof(testtext)];
+    struct buffer dec_buf;
+    buf_set_write(&dec_buf, dec, sizeof(dec));

tests/unit_tests/openvpn/test_crypto.c new file +11 -0

+    gc_free(&gc);
+}
+
+int
+main(void) {
+    const struct CMUnitTest tests[] = {
+        cmocka_unit_test(crypto_pem_encode_decode_loopback),
+    };
+
+#if defined(ENABLE_CRYPTO_OPENSSL)
+    OpenSSL_add_all_algorithms();

Quoted 37 lines · Steffan Karger

Needed for tls-crypt-v2, but isolated enough to be reviewed as a separate patch.

The encode API allocates memory, because it fits our typical gc-oriented code pattern and the caller does not have to do multiple calls or calculations to determine the required destination buffer size.

The decode API does not allocate memory, because the required destination buffer is always smaller than the input buffer (so is easy to manage by the caller) and does not force the caller to use the heap.

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

---
 src/openvpn/crypto_backend.h           | 30 +++++++++++
 src/openvpn/crypto_mbedtls.c           | 74 +++++++++++++++++++++++++++
 src/openvpn/crypto_openssl.c           | 82 ++++++++++++++++++++++++++++++
 tests/unit_tests/openvpn/Makefile.am   | 16 +++++-
 tests/unit_tests/openvpn/test_crypto.c | 92 ++++++++++++++++++++++++++++++++++
 5 files changed, 293 insertions(+), 1 deletion(-)
 create mode 100644 tests/unit_tests/openvpn/test_crypto.c

src/openvpn/crypto_backend.h +1 -0

index 567fd9b..83e14c8 100644
@@ -36,6 +36,7 @@
 #include "crypto_mbedtls.h"
 #endif
 #include "basic.h"
+#include "buffer.h"
 
 /* TLS uses a tag of 128 bytes, let's do the same for OpenVPN */
 #define OPENVPN_AEAD_TAG_LENGTH 16

@@ -105,6 +106,35 @@ void show_available_digests(void);
 
 void show_available_engines(void);
 

src/openvpn/crypto_backend.h void show_available_digests(void); · line 116

Quoted 8 lines

src/openvpn/crypto_backend.h +8 -0

+/**
+ * Encode binary data as PEM
+ *
+ * @param name      The name to use in the PEM header/footer
+ * @param dst       Destination buffer for PEM-encoded data.  Must be a valid
+ *                  pointer to an uninitialized buffer structure.  Iff this
+ *                  function returns true, the buffer will contain memory
+ *                  allocated through the supplied gc.

minor: I see the current style is inconsistent wrt having a '.' at the end of each doxygen line. Maybe we should decide what to do and stick to that ;P But does not need to be changed in this patch of course.

Quoted 65 lines

src/openvpn/crypto_backend.h +21 -0

+ * @param src       Source buffer
+ * @param gc        The garbage collector to use when allocating memory for
+ *                  dst.
+ *
+ * @return true iff PEM encode succeeded.
+ */
+bool crypto_pem_encode(const char *name, struct buffer *dst,
+                       const struct buffer *src, struct gc_arena *gc);
+
+/**
+ * Decode a PEM buffer to binary data
+ *
+ * @param name      The name expected in the PEM header/footer
+ * @param dst       Destination buffer for decoded data
+ * @param src       Source buffer (PEM data)
+ *
+ * @return true iff PEM decode succeeded.
+ */
+bool crypto_pem_decode(const char *name, struct buffer *dst,
+                       const struct buffer *src);
+
 /*
  *
  * Random number functions, used in cases where we want

src/openvpn/crypto_mbedtls.c +21 -0

index 8fa03da..1e86854 100644
@@ -44,11 +44,13 @@
 #include "otime.h"
 #include "misc.h"
 
+#include <mbedtls/base64.h>
 #include <mbedtls/des.h>
 #include <mbedtls/error.h>
 #include <mbedtls/md5.h>
 #include <mbedtls/cipher.h>
 #include <mbedtls/havege.h>
+#include <mbedtls/pem.h>
 
 #include <mbedtls/entropy.h>
 

@@ -229,6 +231,78 @@ show_available_engines(void)
            "available\n");
 }
 
+bool
+crypto_pem_encode(const char *name, struct buffer *dst,
+                  const struct buffer *src, struct gc_arena *gc)
+{
+    /* 1000 chars is the PEM line length limit (+1 for tailing NUL) */
+    char header[1000+1] = { 0 };
+    char footer[1000+1] = { 0 };
+
+    if (!openvpn_snprintf(header, sizeof(header), "-----BEGIN %s-----\n", name))
+    {
+        return false;
+    }
+    if (!openvpn_snprintf(footer, sizeof(footer), "-----END %s-----\n", name))
+    {
+        return false;
+    }
+
+    size_t out_len = 0;
+    if (MBEDTLS_ERR_BASE64_BUFFER_TOO_SMALL !=

src/openvpn/crypto_mbedtls.c show_available_engines(void) · line 262

Quoted 10 lines

src/openvpn/crypto_mbedtls.c +10 -0

+            mbedtls_pem_write_buffer(header, footer, BPTR(src), BLEN(src),
+                                     NULL, 0, &out_len))
+    {
+        return false;
+    }
+
+    *dst = alloc_buf_gc(out_len, gc);
+    if (!mbed_ok(mbedtls_pem_write_buffer(header, footer, BPTR(src), BLEN(src),
+                                          BPTR(dst), BCAP(dst), &out_len))
+        || !buf_inc_len(dst, out_len))

Isn't in the spec of this function to keep the buffer uninitialized when returning false?
Not a big deal because the buffer area was allocated via gc so it can't be leaked.

Don't you think that if buf_inc_len() fails (can this really happen?) the buffer should better be reset?

Quoted 54 lines

src/openvpn/crypto_mbedtls.c +43 -0

+    {
+        return false;
+    }
+
+    return true;
+}
+
+bool
+crypto_pem_decode(const char *name, struct buffer *dst,
+                  const struct buffer *src)
+{
+    /* 1000 chars is the PEM line length limit (+1 for tailing NUL) */
+    char header[1000+1] = { 0 };
+    char footer[1000+1] = { 0 };
+
+    if (*(BLAST(src)) != '\0')
+    {
+        msg(M_WARN, "PEM decode error: source buffer not null-terminated");
+        return false;
+    }
+    if (!openvpn_snprintf(header, sizeof(header), "-----BEGIN %s-----", name))
+    {
+        return false;
+    }
+    if (!openvpn_snprintf(footer, sizeof(footer), "-----END %s-----", name))
+    {
+        return false;
+    }
+
+    size_t use_len = 0;
+    mbedtls_pem_context ctx = { 0 };
+    bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer, BPTR(src),
+                                               NULL, 0, &use_len));
+    if (ret && !buf_write(dst, ctx.buf, ctx.buflen))
+    {
+        ret = false;
+        msg(M_WARN, "PEM decode error: destination buffer too small");
+    }
+
+    mbedtls_pem_free(&ctx);
+    return ret;
+}
+
 /*
  *
  * Random number functions, used in cases where we want

src/openvpn/crypto_openssl.c +0 -0

index 20a519e..49d3aeb 100644
@@ -387,6 +387,88 @@ show_available_engines(void)
 #endif
 }
 

src/openvpn/crypto_openssl.c show_available_engines(void) · line 396

Quoted 7 lines

src/openvpn/crypto_openssl.c +7 -0

+
+bool
+crypto_pem_encode(const char *name, struct buffer *dst,
+                  const struct buffer *src, struct gc_arena *gc)
+{
+    bool ret = false;
+    BIO *bio = BIO_new(BIO_s_mem());

can we assume bio is always initialized with non-NULL here?

src/openvpn/crypto_openssl.c show_available_engines(void) · line 407

Quoted 11 lines

src/openvpn/crypto_openssl.c +11 -0

+    if (!PEM_write_bio(bio, name, "", BPTR(src), BLEN(src)))
+    {
+        ret = false;
+        goto cleanup;
+    }
+
+    BUF_MEM *bptr;
+    BIO_get_mem_ptr(bio, &bptr);
+
+    *dst = alloc_buf_gc(bptr->length, gc);
+    ASSERT(buf_write(dst, bptr->data, bptr->length));

why using an ASSERT() here and none in the mbedtls counterpart?

If there is no special reason I (personally) think that these helper functions should not use ASSERT() (unless something really bad is happening).
Or do you think that a failure here indicates a general memory problem?

Quoted 111 lines

src/openvpn/crypto_openssl.c +64 -0

+
+    ret = true;
+cleanup:
+    if (!BIO_free(bio))
+    {
+        ret = false;;
+    }
+
+    return ret;
+}
+
+bool
+crypto_pem_decode(const char *name, struct buffer *dst,
+                  const struct buffer *src)
+{
+    bool ret = false;
+    BIO *bio;
+
+    if (!(bio = BIO_new_mem_buf((char *)BPTR(src), BLEN(src))))
+    {
+        crypto_msg(M_FATAL, "Cannot open memory BIO for PEM decode");
 
+    }
+
+    char *name_read = NULL;
+    char *header_read = NULL;
+    uint8_t *data_read = NULL;
+    long data_read_len = 0;
+    if (!PEM_read_bio(bio, &name_read, &header_read, &data_read,
+                      &data_read_len))
+    {
+        dmsg(D_CRYPT_ERRORS, "%s: PEM decode failed", __func__);
+        goto cleanup;
+    }
+
+    if (strcmp(name, name_read))
+    {
+        dmsg(D_CRYPT_ERRORS,
+             "%s: unexpected PEM name (got '%s', expected '%s')",
+             __func__, name_read, name);
+        goto cleanup;
+    }
+
+    uint8_t *dst_data = buf_write_alloc(dst, data_read_len);
+    if (!dst_data)
+    {
+        dmsg(D_CRYPT_ERRORS, "%s: dst too small (%i, needs %li)", __func__,
+             BCAP(dst), data_read_len);
+        goto cleanup;
+    }
+    memcpy(dst_data, data_read, data_read_len);
+
+    ret = true;
+cleanup:
+    OPENSSL_free(name_read);
+    OPENSSL_free(header_read);
+    OPENSSL_free(data_read);
+    if (!BIO_free(bio))
+    {
+        ret = false;;
+    }
+
+    return ret;
+}
+
 /*
  *

* Random number functions, used in cases where we want

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

index 23d758b..d100c21 100644
@@ -6,7 +6,7 @@ if HAVE_LD_WRAP_SUPPORT
 check_PROGRAMS += argv_testdriver buffer_testdriver
 endif
 
-check_PROGRAMS += packet_id_testdriver tls_crypt_testdriver
+check_PROGRAMS += crypto_testdriver packet_id_testdriver tls_crypt_testdriver
 
 TESTS = $(check_PROGRAMS)
 

@@ -29,6 +29,20 @@ buffer_testdriver_SOURCES = test_buffer.c mock_msg.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/platform.c
 
+crypto_testdriver_CFLAGS  = @TEST_CFLAGS@ \
+	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
+	$(OPTIONAL_CRYPTO_CFLAGS)
+crypto_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
+	$(OPTIONAL_CRYPTO_LIBS)
+crypto_testdriver_SOURCES = test_crypto.c mock_msg.c \
+	$(openvpn_srcdir)/buffer.c \
+	$(openvpn_srcdir)/crypto.c \
+	$(openvpn_srcdir)/crypto_mbedtls.c \
+	$(openvpn_srcdir)/crypto_openssl.c \
+	$(openvpn_srcdir)/otime.c \
+	$(openvpn_srcdir)/packet_id.c \
+	$(openvpn_srcdir)/platform.c
+
 packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 	-I$(openvpn_includedir) -I$(compat_srcdir) -I$(openvpn_srcdir) \
 	$(OPTIONAL_CRYPTO_CFLAGS)

tests/unit_tests/openvpn/test_crypto.c new file +3 -0

new file mode 100644
index 0000000..62d5b3f
@@ -0,0 +1,92 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based

tests/unit_tests/openvpn/test_crypto.c line 9

Quoted 6 lines

tests/unit_tests/openvpn/test_crypto.c new file +6 -0

+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2016-2017 Fox Crypto B.V. <[email protected]...>
+ *

this should be updated, time flies :)

Quoted 30 lines

tests/unit_tests/openvpn/test_crypto.c new file +30 -0

+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#elif defined(_MSC_VER)
+#include "config-msvc.h"
+#endif
+
+#include "syshead.h"
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+#include <setjmp.h>
+#include <cmocka.h>
+

tests/unit_tests/openvpn/test_crypto.c line 44

Quoted 5 lines

tests/unit_tests/openvpn/test_crypto.c new file +5 -0

+#include "crypto.h"
+
+#include "mock_msg.h"
+
+int script_security = 0; /* Avoid including misc.c */

I tried commenting it and I don't see any warning. Which include chain should lead to its declaration?

tests/unit_tests/openvpn/test_crypto.c line 52

Quoted 8 lines

tests/unit_tests/openvpn/test_crypto.c new file +8 -0

+
+static const char testtext[] = "Dummy text to test PEM encoding";
+
+/**
+ * Check that packet replays are accepted when CO_IGNORE_PACKET_ID is set. This
+ * is used for the first control channel packet that arrives, because we don't
+ * know the packet ID yet.
+ */

Is the comment above a copy/paste incident? or there is something hidden in this unit test?

Quoted 15 lines

tests/unit_tests/openvpn/test_crypto.c new file +15 -0

+static void
+crypto_pem_encode_decode_loopback(void **state) {
+    struct gc_arena gc = gc_new();
+    struct buffer src_buf;
+    buf_set_read(&src_buf, (void *)testtext, sizeof(testtext));
+
+    uint8_t dec[sizeof(testtext)];
+    struct buffer dec_buf;
+    buf_set_write(&dec_buf, dec, sizeof(dec));
+
+    struct buffer pem_buf;
+
+    assert_true(crypto_pem_encode("TESTKEYNAME", &pem_buf, &src_buf, &gc));
+
+    /* Wrong key name */

tests/unit_tests/openvpn/test_crypto.c line 71

Quoted 4 lines

tests/unit_tests/openvpn/test_crypto.c new file +4 -0

+    assert_false(crypto_pem_decode("WRONGNAME", &dec_buf, &pem_buf));
+
+    assert_true(crypto_pem_decode("TESTKEYNAME", &dec_buf, &pem_buf));
+

As a final check, wouldn't it be meaningful to compare the content of dec_buf with src_buf to ensure that we properly obtained the original char array?

Another question: do we have any way to verify that pem_buf contains well-formatted PEM data after pem_encode()?
I am asking because if both encode and decode become no-op (because of some bug) we won't be able to realize that.

However, I am not sure we have an easy way to check that..maybe you could use some SSL function that would normally load PEM data?

Quoted 16 lines

tests/unit_tests/openvpn/test_crypto.c new file +16 -0

+    gc_free(&gc);
+}
+
+int
+main(void) {
+    const struct CMUnitTest tests[] = {
+        cmocka_unit_test(crypto_pem_encode_decode_loopback),
+    };
+
+#if defined(ENABLE_CRYPTO_OPENSSL)
+    OpenSSL_add_all_algorithms();
+#endif
+
+    int ret = cmocka_run_group_tests_name("crypto tests", tests, NULL, NULL);
+
+#if defined(ENABLE_CRYPTO_OPENSSL)

tests/unit_tests/openvpn/test_crypto.c line 92

Quoted 6 lines

tests/unit_tests/openvpn/test_crypto.c new file +5 -0

+    EVP_cleanup();
+#endif
+
+    return ret;
+}

Cheers,

Quoted 9 lines

Quoted 8 lines

+/**
+ * Encode binary data as PEM
+ *

+ * @param name      The name to use in the PEM header/footer
+ * @param dst       Destination buffer for PEM-encoded data.  Must be a valid
+ *                  pointer to an uninitialized buffer structure.  Iff this
+ *                  function returns true, the buffer will contain memory
+ *                  allocated through the supplied gc.

Quoted 3 lines

minor: I see the current style is inconsistent wrt having a '.' at the end of each doxygen line. Maybe we should decide what to do and stick to that ;P But does not need to be changed in this patch of course.

You sound just like my colleague who's pointing out the missing dots in my doxygen :') Any preference from your side? I think ending each line with a dot is best, because that looks best in the generated docs.

Quoted 14 lines

src/openvpn/crypto_openssl.c show_available_engines(void) · line 407

Quoted 11 lines

src/openvpn/crypto_openssl.c +11 -0

+    if (!PEM_write_bio(bio, name, "", BPTR(src), BLEN(src)))
+    {
+        ret = false;
+        goto cleanup;
+    }
+
+    BUF_MEM *bptr;
+    BIO_get_mem_ptr(bio, &bptr);
+
+    *dst = alloc_buf_gc(bptr->length, gc);
+    ASSERT(buf_write(dst, bptr->data, bptr->length));

why using an ASSERT() here and none in the mbedtls counterpart?

Quoted 4 lines

If there is no special reason I (personally) think that these helper functions should not use ASSERT() (unless something really bad is happening).
Or do you think that a failure here indicates a general memory problem?

Because in the mbedtls part, mbed my change the "outlen" value. While here, we create a buffer of bptr->length bytes and then write bptr->length bytes to it. If that fails, our entire logic (or executable integrity) is broken. It's our code only, and it can not fail.

Quoted 5 lines

Another question: do we have any way to verify that pem_buf contains well-formatted PEM data after pem_encode()?
I am asking because if both encode and decode become no-op (because of some bug) we won't be able to realize that.

Quoted 2 lines

However, I am not sure we have an easy way to check that..maybe you could use some SSL function that would normally load PEM data?

That is a bit annoying to test indeed. How about I just verify that this is encoding is not a no-op by adding a check that pem_buf is larger that src_buf?

As for correct PEM en/decoding, I expect that the crypto lib has tested that.

Quoted 2 lines

Cheers,

Thanks!

This is a preliminary description of tls-crypt-v2. It should give a good impression about the reasoning and design behind tls-crypt-v2, but might need some polishing and updating.

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

---
 doc/tls-crypt-v2.txt | 164 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 164 insertions(+)
 create mode 100644 doc/tls-crypt-v2.txt

doc/tls-crypt-v2.txt new file +3 -0

new file mode 100644
index 0000000..578b2f9
@@ -0,0 +1,164 @@
+Client-specific tls-crypt keys (--tls-crypt-v2)
+===============================================
+

doc/tls-crypt-v2.txt new file +67 -0

+keys, ``tls-crypt-v2`` adds the ability to supply each client with a unique
+tls-crypt key.  This allows large organisations and VPN providers to profit
+from the same DoS and TLS stack protection that small deployments can already
+achieve using ``tls-auth`` or ``tls-crypt``.
+
+Also, for ``tls-crypt``, even if all these peers succeed in keeping the key
+secret, the key lifetime is limited to roughly 8000 years, divided by the
+number of clients (see the ``--tls-crypt`` section of the man page).  Using
+client-specific keys, we lift this lifetime requirement to roughly 8000 years
+for each client key (which "Should Be Enough For Everybody (tm)").
+
+
+Introduction
+------------
+
+``tls-crypt-v2`` uses an encrypted cookie mechanism to introduce
+client-specific tls-crypt keys without introducing a lot of server-side state.
+The client-specific key is encrypted using a server key.  The server key is the
+same for all servers in a group.  When a client connects, it first sends the
+encrypted key to the server, such that the server can decrypt the key and all
+messages can thereafter be encrypted using the client-specific key.
+
+A wrapped (encrypted and authenticated) client-specific key can also contain
+metadata.  The metadata is wrapped together with the key, and can be used to
+allow servers to identify clients and/or key validity.  This allows the server
+to abort the connection immediately after receiving the first packet, rather
+than performing an entire TLS handshake.  Aborting the connection this early
+greatly improves the DoS resilience and reduces attack service against
+malicious clients that have the ``tls-crypt`` or ``tls-auth`` key.  This is
+particularly relevant for large deployments (think lost key or disgruntled
+employee) and VPN providers (clients are not trusted).
+
+To allow for a smooth transition, ``tls-crypt-v2`` is designed such that a
+server can enable both ``tls-crypt-v2`` and either ``tls-crypt`` or
+``tls-auth``.  This is achieved by introducing a P_CONTROL_HARD_RESET_CLIENT_V3
+opcode, that indicates that the client wants to use ``tls-crypt-v2`` for the
+current connection.
+
+For an exact specification and more details, read the Implementation section.
+
+
+Implementation
+--------------
+
+When setting up a tls-crypt-v2 group (similar to generating a tls-crypt or
+tls-auth key previously):
+
+1. Generate a tls-crypt-v2 server key using OpenVPN's ``--genkey``.  This key
+   contains 4 512-bit keys, of which we use:
+
+   * the first 256 bits of key 1 as AES-256-CTR encryption key ``Ke``
+   * the first 256 bits of key 2 as HMAC-SHA-256 authentication key ``Ka``
+
+2. Add the tls-crypt-v2 server key to all server configs
+   (``tls-crypt-v2 /path/to/server.key``)
+
+
+When provisioning a client, create a client-specific tls-crypt key:
+
+1. Generate 2048 bits client-specific key ``Kc``
+2. Optionally generate metadata
+3. Create a wrapped client key ``WKc``, using the same nonce-misuse-resistant
+   SIV conruction we use for tls-crypt:
+
+   ``T = HMAC-SHA256(Ka, Kc || metadata)``
+
+   ``IV = 128 most significant bits of T``

doc/tls-crypt-v2.txt new file +27 -0

+
+
+When setting up the openvpn connection:
+
+1. The client reads the tls-crypt-v2 key from its config, and:
+
+   1. loads ``Kc`` as its tls-crypt key,
+   2. stores ``WKc`` in memory for sending to the server.
+
+2. To start the connection, the client creates a P_CONTROL_HARD_RESET_CLIENT_V3
+   message without payload, wraps it with tls-crypt using ``Kc`` as the key,
+   and appends ``WKc``.  (``WKc`` must not be encrypted, to prevent a
+   chicken-and-egg problem.)
+
+3. The server receives the P_CONTROL_HARD_RESET_CLIENT_V3 message, and
+
+   a. unwraps ``WKc`` and strips ``WKc`` from the message.
+   b. uses unwrapped ``Kc`` to verify the remaining
+      P_CONTROL_HARD_RESET_CLIENT_V3 message's authentication.
+
+   The message is dropped and no error response is sent when either a or b
+   fails (DoS protection).
+
+4. Server optionally checks metadata using a --tls-crypt-v2-verify script
+
+   Metadata could for example contain the users certificate serial, such that
+   the incoming connection can be verified against a CRL, or a notAfter

doc/tls-crypt-v2.txt new file +9 -0

+
+   RFC: should the server send a 'key rejected' message if the key is e.g.
+   revoked or expired?  That allows for better client-side error reporting, but
+   also reduces the DoS resilience.
+
+6. Client and server use ``Kc`` for (un)wrapping any following control channel
+   messages.
+
+

doc/tls-crypt-v2.txt new file +8 -0

+
+``tls-crypt-v2`` uses fixed crypto algorithms, because:
+
+ * The crypto is used before we can do any negotiation, so the algorithms have
+   to be predefined.
+ * The crypto primitives are chosen conservatively, making problems with these
+   primitives unlikely.
+ * Making anything configurable adds complexity, both in implementation and

there was a comment in my previous review that remained unanswer. Please, see it below

On 09/03/18 04:23, Steffan Karger wrote:
[CUT]

doc/tls-crypt-v2.txt line 128

Quoted 4 lines

doc/tls-crypt-v2.txt new file +4 -0

+   The metadata is checked *after* the OpenVPN three-way handshake has
+   completed, to prevent DoS attacks.  (That is, once the client has proved to
+   the server that it possesses Kc, by authenticating a packet that contains the
+   session ID picked by the server.)

just a thought here: the metadata is actually created by the server/provider and it is authenticated/encrypted with the server key.

Isn't this enough to ensure that its content is not malicious and thus allow the server to parse it right after having received the HARD_RESET_V3 (instead of performing a "three-way handshake" first)?

To avoid a dependency on misc.c - which is a dependency mess - in the tls-crypt unit tests, split the env_set functionality out into it's own file.

Signed-off-by Antonio Quartulli <[email protected]...>

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

Acked-by Gert Doering <[email protected]...>

Does what it says on the lid: code is moved 1:1 with no code changes, and only #include lines are adjusted elsewhere (which is actually painful to review in this case, as the "moved out" stuff was scattered along misc.c and parts ended in env_set.h, so "plain diff" between old-misc.c and new-env_set.c didn't help much).

One small catch: this collided with 06ad53e067d9a8b and setenv_long_long() being changed from "%lld" to PRIi64 - so I changed the patch accordingly so that it still only moves lines from misc.c to env_set.c

Your patch has been applied to the master branch.

commit 0afe0ca5620d5ab7cec3380d40048dce2286f048

Author: Steffan Karger
Date:   Fri Dec 8 13:07:47 2017 +0100

     Move env helper functions into their own module/file

     Signed-off-by: Antonio Quartulli <[email protected]...>
     Signed-off-by: Steffan Karger <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg16066.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

To avoid having to include misc.c - which is a dependency mess - in the tls-crypt unit tests, move file-handing related functions to platform.c (which is where other file-related functions already reside).

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

---
 src/openvpn/init.c                         |   2 +-
 src/openvpn/misc.c                         | 148 -----------------------------
 src/openvpn/misc.h                         |  12 ---
 src/openvpn/multi.c                        |  25 ++---
 src/openvpn/pf.c                           |   4 +-
 src/openvpn/platform.c                     | 148 +++++++++++++++++++++++++++++
 src/openvpn/platform.h                     |  18 ++++
 src/openvpn/plugin.c                       |   6 +-
 src/openvpn/ssl_verify.c                   |  12 ++-
 tests/unit_tests/openvpn/Makefile.am       |   3 +
 tests/unit_tests/openvpn/mock_get_random.c |  36 +++++++
 11 files changed, 232 insertions(+), 182 deletions(-)
 create mode 100644 tests/unit_tests/openvpn/mock_get_random.c

src/openvpn/init.c +1 -1

index a4603b3..4055f28 100644
@@ -807,7 +807,7 @@ init_static(void)
 #ifdef STATUS_PRINTF_TEST
     {
         struct gc_arena gc = gc_new();
-        const char *tmp_file = create_temp_file("/tmp", "foo", &gc);
+        const char *tmp_file = platform_create_temp_file("/tmp", "foo", &gc);
         struct status_output *so = status_open(tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);
         status_printf(so, "%s", "foo");
         status_printf(so, "%s", "bar");

src/openvpn/misc.c +0 -148

index ad26f3d..b04a9d7 100644
@@ -313,87 +313,6 @@ openvpn_popen(const struct argv *a,  const struct env_set *es)
     return ret;
 }
 
-
-/* return true if filename can be opened for read */
-bool
-test_file(const char *filename)
-{
-    bool ret = false;
-    if (filename)
-    {
-        FILE *fp = platform_fopen(filename, "r");
-        if (fp)
-        {
-            fclose(fp);
-            ret = true;
-        }
-        else
-        {
-            if (openvpn_errno() == EACCES)
-            {
-                msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename);
-            }
-        }
-    }
-
-    dmsg(D_TEST_FILE, "TEST FILE '%s' [%d]",
-         filename ? filename : "UNDEF",
-         ret);
-
-    return ret;
-}
-
-/* create a temporary filename in directory */
-const char *
-create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
-{
-    int fd;
-    const char *retfname = NULL;
-    unsigned int attempts = 0;
-    char fname[256] = { 0 };
-    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
-    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
-
-    while (attempts < 6)
-    {
-        ++attempts;
-
-        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
-                              prefix, (unsigned long) get_random(),
-                              (unsigned long) get_random()))
-        {
-            msg(M_WARN, "ERROR: temporary filename too long");
-            return NULL;
-        }
-
-        retfname = gen_path(directory, fname, gc);
-        if (!retfname)
-        {
-            msg(M_WARN, "Failed to create temporary filename and path");
-            return NULL;
-        }
-
-        /* Atomically create the file.  Errors out if the file already
-         * exists.  */
-        fd = platform_open(retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
-        if (fd != -1)
-        {
-            close(fd);
-            return retfname;
-        }
-        else if (fd == -1 && errno != EEXIST)
-        {
-            /* Something else went wrong, no need to retry.  */
-            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
-                retfname);
-            return NULL;
-        }
-    }
-
-    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
-    return NULL;
-}
-
 /*
  * Prepend a random string to hostname to prevent DNS caching.
  * For example, foo.bar.gov would be modified to <random-chars>.foo.bar.gov.

@@ -416,73 +335,6 @@ hostname_randomize(const char *hostname, struct gc_arena *gc)
 }
 
 /*
- * Put a directory and filename together.
- */
-const char *
-gen_path(const char *directory, const char *filename, struct gc_arena *gc)
-{
-#ifdef _WIN32
-    const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON
-                                 |CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
-#else
-    const int CC_PATH_RESERVED = CC_SLASH;
-#endif
-
-    if (!gc)
-    {
-        return NULL; /* Would leak memory otherwise */
-    }
-
-    const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
-
-    if (safe_filename
-        && strcmp(safe_filename, ".")
-        && strcmp(safe_filename, "..")
-#ifdef _WIN32
-        && win_safe_filename(safe_filename)
-#endif
-        )
-    {
-        const size_t outsize = strlen(safe_filename) + (directory ? strlen(directory) : 0) + 16;
-        struct buffer out = alloc_buf_gc(outsize, gc);
-        char dirsep[2];
-
-        dirsep[0] = OS_SPECIFIC_DIRSEP;
-        dirsep[1] = '\0';
-
-        if (directory)
-        {
-            buf_printf(&out, "%s%s", directory, dirsep);
-        }
-        buf_printf(&out, "%s", safe_filename);
-
-        return BSTR(&out);
-    }
-    else
-    {
-        return NULL;
-    }
-}
-
-bool
-absolute_pathname(const char *pathname)
-{
-    if (pathname)
-    {
-        const int c = pathname[0];
-#ifdef _WIN32
-        return c == '\\' || (isalpha(c) && pathname[1] == ':' && pathname[2] == '\\');
-#else
-        return c == '/';
-#endif
-    }
-    else
-    {
-        return false;
-    }
-}
-
-/*
  * Get and store a username/password
  */
 

src/openvpn/misc.h +0 -12

index 3725fcf..82bff9a 100644
@@ -79,18 +79,6 @@ const char **make_extended_arg_array(char **p, struct gc_arena *gc);
 /* an analogue to the random() function, but use OpenSSL functions if available */
 long int get_random(void);
 
-/* return true if filename can be opened for read */
-bool test_file(const char *filename);
-
-/* create a temporary file in directory, returns the filename of the created file */
-const char *create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc);
-
-/* put a directory and filename together */
-const char *gen_path(const char *directory, const char *filename, struct gc_arena *gc);
-
-/* return true if pathname is absolute */
-bool absolute_pathname(const char *pathname);
-
 /* prepend a random prefix to hostname */
 const char *hostname_randomize(const char *hostname, struct gc_arena *gc);
 

src/openvpn/multi.c +14 -11

index 25b2d09..acc6912 100644
@@ -1639,7 +1639,7 @@ multi_client_connect_post(struct multi_context *m,
                           unsigned int *option_types_found)
 {
     /* Did script generate a dynamic config file? */
-    if (test_file(dc_file))
+    if (platform_test_file(dc_file))
     {
         options_server_import(&mi->context.options,
                               dc_file,

@@ -1826,12 +1826,13 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         {
             const char *ccd_file;
 
-            ccd_file = gen_path(mi->context.options.client_config_dir,
-                                tls_common_name(mi->context.c2.tls_multi, false),
-                                &gc);
+            ccd_file = platform_gen_path(mi->context.options.client_config_dir,
+                                         tls_common_name(mi->context.c2.tls_multi,
+                                                         false),
+                                         &gc);
 
             /* try common-name file */
-            if (test_file(ccd_file))
+            if (platform_test_file(ccd_file))
             {
                 options_server_import(&mi->context.options,
                                       ccd_file,

@@ -1842,11 +1843,11 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
             }
             else /* try default file */
             {
-                ccd_file = gen_path(mi->context.options.client_config_dir,
-                                    CCD_DEFAULT,
-                                    &gc);
+                ccd_file = platform_gen_path(mi->context.options.client_config_dir,
+                                             CCD_DEFAULT,
+                                             &gc);
 
-                if (test_file(ccd_file))
+                if (platform_test_file(ccd_file))
                 {
                     options_server_import(&mi->context.options,
                                           ccd_file,

@@ -1876,7 +1877,8 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
         if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
         {
             struct argv argv = argv_new();
-            const char *dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+            const char *dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
+                                                            "cc", &gc);
 
             if (!dc_file)
             {

@@ -1938,7 +1940,8 @@ script_depr_failed:
 
             setenv_str(mi->context.c2.es, "script_type", "client-connect");
 
-            dc_file = create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+            dc_file = platform_create_temp_file(mi->context.options.tmp_dir,
+                                                "cc", &gc);
             if (!dc_file)
             {
                 cc_succeeded = false;

src/openvpn/pf.c +2 -2

index 6e4107c..fd9f215 100644
@@ -621,8 +621,8 @@ pf_init_context(struct context *c)
 #ifdef PLUGIN_PF
     if (plugin_defined(c->plugins, OPENVPN_PLUGIN_ENABLE_PF))
     {
-        c->c2.pf.filename = create_temp_file(c->options.tmp_dir, "pf",
-                                             &c->c2.gc);
+        c->c2.pf.filename = platform_create_temp_file(c->options.tmp_dir, "pf",
+                                                      &c->c2.gc);
         if (c->c2.pf.filename)
         {
             setenv_str(c->c2.es, "pf_file", c->c2.pf.filename);

src/openvpn/platform.c +148 -0

index e942ba9..5281267 100644
@@ -31,6 +31,7 @@
 
 #include "buffer.h"
 #include "error.h"
+#include "misc.h"
 #include "win32.h"
 
 #include "memdbg.h"

@@ -335,3 +336,150 @@ platform_stat(const char *path, platform_stat_t *buf)
 #endif
 }
 
+/* create a temporary filename in directory */
+const char *
+platform_create_temp_file(const char *directory, const char *prefix, struct gc_arena *gc)
+{
+    int fd;
+    const char *retfname = NULL;
+    unsigned int attempts = 0;
+    char fname[256] = { 0 };
+    const char *fname_fmt = PACKAGE "_%.*s_%08lx%08lx.tmp";
+    const int max_prefix_len = sizeof(fname) - (sizeof(PACKAGE) + 7 + (2 * 8));
+
+    while (attempts < 6)
+    {
+        ++attempts;
+
+        if (!openvpn_snprintf(fname, sizeof(fname), fname_fmt, max_prefix_len,
+                              prefix, (unsigned long) get_random(),
+                              (unsigned long) get_random()))
+        {
+            msg(M_WARN, "ERROR: temporary filename too long");
+            return NULL;
+        }
+
+        retfname = platform_gen_path(directory, fname, gc);
+        if (!retfname)
+        {
+            msg(M_WARN, "Failed to create temporary filename and path");
+            return NULL;
+        }
+
+        /* Atomically create the file.  Errors out if the file already
+         * exists.  */
+        fd = platform_open(retfname, O_CREAT | O_EXCL | O_WRONLY, S_IRUSR | S_IWUSR);
+        if (fd != -1)
+        {
+            close(fd);
+            return retfname;
+        }
+        else if (fd == -1 && errno != EEXIST)
+        {
+            /* Something else went wrong, no need to retry.  */
+            msg(M_WARN | M_ERRNO, "Could not create temporary file '%s'",
+                retfname);
+            return NULL;
+        }
+    }
+
+    msg(M_WARN, "Failed to create temporary file after %i attempts", attempts);
+    return NULL;
+}
+
+/*
+ * Put a directory and filename together.
+ */
+const char *
+platform_gen_path(const char *directory, const char *filename,
+                  struct gc_arena *gc)
+{
+#ifdef _WIN32
+    const int CC_PATH_RESERVED = CC_LESS_THAN|CC_GREATER_THAN|CC_COLON
+                                 |CC_DOUBLE_QUOTE|CC_SLASH|CC_BACKSLASH|CC_PIPE|CC_QUESTION_MARK|CC_ASTERISK;
+#else
+    const int CC_PATH_RESERVED = CC_SLASH;
+#endif
+
+    if (!gc)
+    {
+        return NULL; /* Would leak memory otherwise */
+    }
+
+    const char *safe_filename = string_mod_const(filename, CC_PRINT, CC_PATH_RESERVED, '_', gc);
+
+    if (safe_filename
+        && strcmp(safe_filename, ".")
+        && strcmp(safe_filename, "..")
+#ifdef _WIN32
+        && win_safe_filename(safe_filename)
+#endif
+        )
+    {
+        const size_t outsize = strlen(safe_filename) + (directory ? strlen(directory) : 0) + 16;
+        struct buffer out = alloc_buf_gc(outsize, gc);
+        char dirsep[2];
+
+        dirsep[0] = OS_SPECIFIC_DIRSEP;
+        dirsep[1] = '\0';
+
+        if (directory)
+        {
+            buf_printf(&out, "%s%s", directory, dirsep);
+        }
+        buf_printf(&out, "%s", safe_filename);
+
+        return BSTR(&out);
+    }
+    else
+    {
+        return NULL;
+    }
+}
+
+bool
+platform_absolute_pathname(const char *pathname)
+{
+    if (pathname)
+    {
+        const int c = pathname[0];
+#ifdef _WIN32
+        return c == '\\' || (isalpha(c) && pathname[1] == ':' && pathname[2] == '\\');
+#else
+        return c == '/';
+#endif
+    }
+    else
+    {
+        return false;
+    }
+}
+
+/* return true if filename can be opened for read */
+bool
+platform_test_file(const char *filename)
+{
+    bool ret = false;
+    if (filename)
+    {
+        FILE *fp = platform_fopen(filename, "r");
+        if (fp)
+        {
+            fclose(fp);
+            ret = true;
+        }
+        else
+        {
+            if (openvpn_errno() == EACCES)
+            {
+                msg( M_WARN | M_ERRNO, "Could not access file '%s'", filename);
+            }
+        }
+    }
+
+    dmsg(D_TEST_FILE, "TEST FILE '%s' [%d]",
+         filename ? filename : "UNDEF",
+         ret);
+
+    return ret;
+}

src/openvpn/platform.h +18 -0

index cd2bbc9..e116fd5 100644
@@ -49,6 +49,7 @@
 #endif
 
 #include "basic.h"
+#include "buffer.h"
 
 /* Get/Set UID of process */
 

@@ -143,4 +144,21 @@ typedef struct stat platform_stat_t;
 #endif
 int platform_stat(const char *path, platform_stat_t *buf);
 
+/**
+ * Create a temporary file in directory, returns the filename of the created
+ * file.
+ */
+const char *platform_create_temp_file(const char *directory, const char *prefix,
+                                      struct gc_arena *gc);
+
+/** Put a directory and filename together. */
+const char *platform_gen_path(const char *directory, const char *filename,
+                              struct gc_arena *gc);
+
+/** Return true if pathname is absolute. */
+bool platform_absolute_pathname(const char *pathname);
+
+/** Return true if filename can be opened for read. */
+bool platform_test_file(const char *filename);
+
 #endif /* ifndef PLATFORM_H */

src/openvpn/plugin.c +3 -3

index 7387f8b..9dfa744 100644
@@ -249,7 +249,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
      * was parsed.
      *
      */
-    if (!absolute_pathname(p->so_pathname)
+    if (!platform_absolute_pathname(p->so_pathname)
         && p->so_pathname[0] != '.')
     {
         char full[PATH_MAX];

@@ -259,7 +259,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
     }
     else
     {
-        rel = !absolute_pathname(p->so_pathname);
+        rel = !platform_absolute_pathname(p->so_pathname);
         p->handle = dlopen(p->so_pathname, RTLD_NOW);
     }
     if (!p->handle)

@@ -271,7 +271,7 @@ plugin_init_item(struct plugin *p, const struct plugin_option *o)
 
 #else  /* ifndef _WIN32 */
 
-    rel = !absolute_pathname(p->so_pathname);
+    rel = !platform_absolute_pathname(p->so_pathname);
     p->module = LoadLibraryW(wide_string(p->so_pathname, &gc));
     if (!p->module)
     {

src/openvpn/ssl_verify.c +7 -5

index ebb1da2..3568bad 100644
@@ -547,7 +547,7 @@ verify_cert_export_cert(openvpn_x509_cert_t *peercert, const char *tmp_dir, stru
 
     /* create tmp file to store peer cert */
     if (!tmp_dir
-        || !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
+        || !(peercert_filename = platform_create_temp_file(tmp_dir, "pcf", gc)))
     {
         msg (M_WARN, "Failed to create peer cert file");
         return NULL;

@@ -887,7 +887,7 @@ key_state_gen_auth_control_file(struct key_state *ks, const struct tls_options *
     struct gc_arena gc = gc_new();
 
     key_state_rm_auth_control_file(ks);
-    const char *acf = create_temp_file(opt->tmp_dir, "acf", &gc);
+    const char *acf = platform_create_temp_file(opt->tmp_dir, "acf", &gc);
     if (acf)
     {
         ks->auth_control_file = string_alloc(acf, NULL);

@@ -1100,7 +1100,8 @@ verify_user_pass_script(struct tls_session *session, const struct user_pass *up)
         {
             struct status_output *so;
 
-            tmp_file = create_temp_file(session->opt->tmp_dir, "up", &gc);
+            tmp_file = platform_create_temp_file(session->opt->tmp_dir, "up",
+                                                 &gc);
             if (tmp_file)
             {
                 so = status_open(tmp_file, 0, -1, NULL, STATUS_OUTPUT_WRITE);

@@ -1510,8 +1511,9 @@ verify_final_auth_checks(struct tls_multi *multi, struct tls_session *session)
         struct gc_arena gc = gc_new();
 
         const char *cn = session->common_name;
-        const char *path = gen_path(session->opt->client_config_dir_exclusive, cn, &gc);
-        if (!cn || !strcmp(cn, CCD_DEFAULT) || !test_file(path))
+        const char *path = platform_gen_path(session->opt->client_config_dir_exclusive,
+                                             cn, &gc);
+        if (!cn || !strcmp(cn, CCD_DEFAULT) || !platform_test_file(path))
         {
             ks->authenticated = false;
             wipe_auth_token(multi);

tests/unit_tests/openvpn/Makefile.am +3 -0

index 7b41363..b69da84 100644
@@ -19,6 +19,7 @@ argv_testdriver_CFLAGS  = @TEST_CFLAGS@ -I$(openvpn_srcdir) -I$(compat_srcdir) \
 argv_testdriver_LDFLAGS = @TEST_LDFLAGS@ -L$(openvpn_srcdir) -Wl,--wrap=parse_line \
 	$(OPTIONAL_CRYPTO_LIBS)
 argv_testdriver_SOURCES = test_argv.c mock_msg.c \
+	mock_get_random.c \
 	$(openvpn_srcdir)/platform.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/argv.c

@@ -26,6 +27,7 @@ argv_testdriver_SOURCES = test_argv.c mock_msg.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 \
+	mock_get_random.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/platform.c
 

@@ -50,6 +52,7 @@ packet_id_testdriver_CFLAGS  = @TEST_CFLAGS@ \
 packet_id_testdriver_LDFLAGS = @TEST_LDFLAGS@ \
 	$(OPTIONAL_CRYPTO_LIBS)
 packet_id_testdriver_SOURCES = test_packet_id.c mock_msg.c \
+	mock_get_random.c \
 	$(openvpn_srcdir)/buffer.c \
 	$(openvpn_srcdir)/otime.c \
 	$(openvpn_srcdir)/packet_id.c \

tests/unit_tests/openvpn/mock_get_random.c new file +36 -0

new file mode 100644
index 0000000..da92a9b
@@ -0,0 +1,36 @@
+/*
+ *  OpenVPN -- An application to securely tunnel IP networks
+ *             over a single UDP port, with support for SSL/TLS-based
+ *             session authentication and key exchange,
+ *             packet encryption, packet authentication, and
+ *             packet compression.
+ *
+ *  Copyright (C) 2017 Fox Crypto B.V. <[email protected]...>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2
+ *  as published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <stdarg.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <setjmp.h>
+#include <cmocka.h>
+
+unsigned long
+get_random(void)
+{
+    /* rand() is not very random, but it's C99 and this is just for testing */
+    return rand();
+}

Quoted 5 lines · Steffan Karger

To avoid having to include misc.c - which is a dependency mess - in the tls-crypt unit tests, move file-handing related functions to platform.c (which is where other file-related functions already reside).

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

I have applied this patch right after the one about PEM and it triggered the following:

Falling back to patching base and 3-way merge...

How about rebasing it before the actual tls-crypt-v2 patches so that it could be applied earlier and shrink down this patchset?

On top of that, would you please extend the commit message a little bit to include the fact that you are adding a new mock'd component (get_random) needed by the tls-crypt unit-test?

Actually....If I understand correctly, this new mock'd component will only be used when the other patches will be merged too. Therefore, I guess this patch could be divided in 2: one part being the code move and the second part being the introduction of the mock'd component. The latter may directly be included in the patch adding the tls-crypt unit-test.

What do you think?

Other than this, the patch looks good.
Checked with "git show --color-moved=zebra" to ensure that the code being moved wasn't modified.

And +1 for this patch for cleaning up the misc.c mess some more.

Quoted 8 lines · Antonio Quartulli

Actually....If I understand correctly, this new mock'd component will only be used when the other patches will be merged too. Therefore, I guess this patch could be divided in 2: one part being the code move and the second part being the introduction of the mock'd component. The latter may directly be included in the patch adding the tls-crypt unit-test.

What do you think?

I think you have a good point. I'll reorder, split and merge.

From: Steffan Karger <[email protected]...>

To avoid having to include misc.c - which is a dependency mess - in the tls-crypt unit tests, move the command execution helper functions to a new run_command.c module.

While at it, abstract away the script_security global variable.

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