Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2

Change crypto_pem_encode to not put a nul-terminated terminated
string into the buffer. This was  useful for printf but should
not be written into the file.

Instead do not assume that the buffer is null terminated and
print only the number of bytes in the buffer. Also fix a
similar case in printing static key where the 0 byte was
never added to the buffer

Patch V2: make pem_encode behave more like other similar functions in
OpenVPN
          and do not null terminate.

Patch V3: also make the mbed TLS variant of pem_decode behave like other
          similar functions in OpeNVPN and accept a not null-terminated
          buffer.

Patch V4: The newly introduced unit test
          test_tls_crypt_v2_write_client_key_file_metadata
          was added after the V3 version of the patch and now misses the
          strlen with memcmp replacment that were added to
          test_tls_crypt_v2_write_client_key_file. Also add the
          modifictions to this function.

          Unconditionally allocate buffer in mbed TLS path as
          requested by Steffan.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>
Message-Id: <20200507132534.6380-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19852.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index f1a52d8c771464a14512e938ee07e4c1ea66849a..2388027c5e3cf7a8f06e04f550dab68173bc0c0f 100644 (file)
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1491,7 +1491,7 @@ write_key_file(const int nkeys, const char *filename)
     /* write key file to stdout if no filename given */
     if (!filename || strcmp(filename, "")==0)
     {
-        printf("%s\n", BPTR(&out));
+        printf("%.*s\n", BLEN(&out), BPTR(&out));
     }
     /* write key file, now formatted in out, to file */
     else if (!buffer_write_file(filename, &out))
@@ -1900,7 +1900,7 @@ write_pem_key_file(const char *filename, const char *pem_name)
 
     if (!filename || strcmp(filename, "")==0)
     {
-        printf("%s\n", BPTR(&server_key_pem));
+        printf("%.*s", BLEN(&server_key_pem), BPTR(&server_key_pem));
     }
     else if (!buffer_write_file(filename, &server_key_pem))
     {

diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 35072b736c073ae5517fb540d032bc4e1f80a4f2..c8dcddc88aca078d86799ebe2031dd70b163433c 100644 (file)
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -239,10 +239,12 @@ crypto_pem_encode(const char *name, struct buffer *dst,
         return false;
     }
 
+    /* We set the size buf to out_len-1 to NOT include the 0 byte that
+     * mbedtls_pem_write_buffer in its length calculation */
     *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))
+        || !buf_inc_len(dst, out_len-1))
     {
         CLEAR(*dst);
         return false;
@@ -259,11 +261,6 @@ crypto_pem_decode(const char *name, struct buffer *dst,
     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;
@@ -273,9 +270,16 @@ crypto_pem_decode(const char *name, struct buffer *dst,
         return false;
     }
 
+    /* mbed TLS requires the src to be null-terminated */
+    /* allocate a new buffer to avoid modifying the src buffer */
+    struct gc_arena gc = gc_new();
+    struct buffer input = alloc_buf_gc(BLEN(src) + 1, &gc);
+    buf_copy(&input, src);
+    buf_null_terminate(&input);
+
     size_t use_len = 0;
     mbedtls_pem_context ctx = { 0 };
-    bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer, BPTR(src),
+    bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer, BPTR(&input),
                                                NULL, 0, &use_len));
     if (ret && !buf_write(dst, ctx.buf, ctx.buflen))
     {
@@ -284,6 +288,7 @@ crypto_pem_decode(const char *name, struct buffer *dst,
     }
 
     mbedtls_pem_free(&ctx);
+    gc_free(&gc);
     return ret;
 }
 

diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 9e7ea0fff3ac3820fc1ff327414ce2243888ed65..a5b2c45a43483bc675d2a1fa44724b24d2934631 100644 (file)
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -400,9 +400,8 @@ crypto_pem_encode(const char *name, struct buffer *dst,
     BUF_MEM *bptr;
     BIO_get_mem_ptr(bio, &bptr);
 
-    *dst = alloc_buf_gc(bptr->length + 1, gc);
+    *dst = alloc_buf_gc(bptr->length, gc);
     ASSERT(buf_write(dst, bptr->data, bptr->length));
-    buf_null_terminate(dst);
 
     ret = true;
 cleanup:

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index a3894d66e758e35b93b6d3138543945e62b70a14..7b5016d38eddf518f9156ca006458aa3d3dd6894 100644 (file)
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -702,7 +702,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
 
     if (!filename || streq(filename, ""))
     {
-        printf("%s\n", BPTR(&client_key_pem));
+        printf("%.*s\n", BLEN(&client_key_pem), BPTR(&client_key_pem));
         client_file = (const char *)BPTR(&client_key_pem);
         client_inline = true;
     }

diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c
index 2a146f840dc3e425ba99003a9f1f12d704e78406..218772e8b3612b07aae60c42351c0c0f99836c7a 100644 (file)
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -548,7 +548,8 @@ test_tls_crypt_v2_write_server_key_file(void **state)
     const char *filename = "testfilename.key";
 
     expect_string(__wrap_buffer_write_file, filename, filename);
-    expect_string(__wrap_buffer_write_file, pem, test_server_key);
+    expect_memory(__wrap_buffer_write_file, pem, test_server_key,
+                  strlen(test_server_key));
     will_return(__wrap_buffer_write_file, true);
 
     tls_crypt_v2_write_server_key_file(filename);
@@ -561,7 +562,8 @@ test_tls_crypt_v2_write_client_key_file(void **state)
 
     /* Test writing the client key */
     expect_string(__wrap_buffer_write_file, filename, filename);
-    expect_string(__wrap_buffer_write_file, pem, test_client_key);
+    expect_memory(__wrap_buffer_write_file, pem, test_client_key,
+                  strlen(test_client_key));
     will_return(__wrap_buffer_write_file, true);
 
     /* Key generation re-reads the created file as a sanity check */
@@ -579,7 +581,8 @@ test_tls_crypt_v2_write_client_key_file_metadata(void **state)
 
     /* Test writing the client key */
     expect_string(__wrap_buffer_write_file, filename, filename);
-    expect_string(__wrap_buffer_write_file, pem, test_client_key_metadata);
+    expect_memory(__wrap_buffer_write_file, pem, test_client_key_metadata,
+                strlen(test_client_key_metadata));
     will_return(__wrap_buffer_write_file, true);
 
     /* Key generation re-reads the created file as a sanity check */