[Openvpn-devel,v3] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2

Message ID 20200427012627.27568-1-arne@rfc2549.org
State Superseded
Headers show
Series [Openvpn-devel,v3] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2 | expand

Commit Message

Arne Schwabe April 26, 2020, 3:26 p.m. UTC
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.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto.c                      |  4 ++--
 src/openvpn/crypto_mbedtls.c              | 28 +++++++++++++++++------
 src/openvpn/crypto_openssl.c              |  3 +--
 src/openvpn/tls_crypt.c                   |  2 +-
 tests/unit_tests/openvpn/test_tls_crypt.c |  6 +++--
 5 files changed, 29 insertions(+), 14 deletions(-)

Comments

Steffan Karger May 3, 2020, 7:18 a.m. UTC | #1
Hi,

On 27-04-2020 03:26, Arne Schwabe wrote:
> 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.

Thanks, this looks better. Two more things:

> @@ -273,9 +270,25 @@ crypto_pem_decode(const char *name, struct buffer *dst,
>          return false;
>      }
>  
> +    /* mbed TLS requires the src to be null-terminated */
> +    struct gc_arena gc = gc_new();
> +    struct buffer input;
> +
> +    if (*(BLAST(src)) == '\0')
> +    {
> +        input = *src;
> +    }
> +    else
> +    {
> +        /* allocate a new buffer to avoid modifying the src 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))
>      {

Maybe a matter of taste, but why not always alloc the buffer? That
simplifies the code, and performance nor memory footprint is relevant
here. Just a suggestion. I'm also fine with keeping it like this.

> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c
> index 366b48d5..f4b1f860 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -530,7 +530,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);
> @@ -542,7 +543,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 */
> 

ASAN tells me you're missing another one:

==26863==ERROR: AddressSanitizer: heap-buffer-overflow on address
0x6180000023d6 at pc 0x0000004310c8 bp 0x7fffbae53b90 sp 0x7fffbae53338
READ of size 847 at 0x6180000023d6 thread T0
    #0 0x4310c7 in strcmp
(tests/unit_tests/openvpn/tls_crypt_testdriver+0x4310c7)
    #1 0x7f9234541c6e  (/lib/x86_64-linux-gnu/libcmocka.so.0+0x2c6e)
    #2 0x7f92345446f9 in _check_expected
(/lib/x86_64-linux-gnu/libcmocka.so.0+0x56f9)
    #3 0x4cd414 in __wrap_buffer_write_file
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:108:5
    #4 0x4cc23b in tls_crypt_v2_write_client_key_file
tests/unit_tests/openvpn/../../.././../openvpn/src/openvpn/tls_crypt.c:709:15
    #5 0x4d0f15 in test_tls_crypt_v2_write_client_key_file_metadata
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:592:5
    #6 0x7f92345444e0  (/lib/x86_64-linux-gnu/libcmocka.so.0+0x54e0)
    #7 0x7f9234544ec4 in _cmocka_run_group_tests
(/lib/x86_64-linux-gnu/libcmocka.so.0+0x5ec4)
    #8 0x4cd75a in main
tests/unit_tests/openvpn/../../.././../openvpn/tests/unit_tests/openvpn/test_tls_crypt.c:645:15
    #9 0x7f92341fd0b2 in __libc_start_main
/build/glibc-YYA7BZ/glibc-2.31/csu/../csu/libc-start.c:308:16
    #10 0x41d92d in _start
(tests/unit_tests/openvpn/tls_crypt_testdriver+0x41d92d)

-Steffan

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 1678cba8..b05262e1 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1478,7 +1478,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))
@@ -1887,7 +1887,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 3e77fa9e..9529b319 100644
--- 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,25 @@  crypto_pem_decode(const char *name, struct buffer *dst,
         return false;
     }
 
+    /* mbed TLS requires the src to be null-terminated */
+    struct gc_arena gc = gc_new();
+    struct buffer input;
+
+    if (*(BLAST(src)) == '\0')
+    {
+        input = *src;
+    }
+    else
+    {
+        /* allocate a new buffer to avoid modifying the src 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 +297,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 a81dcfd8..4fa65ba8 100644
--- 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 e9f9cc2a..3018c18e 100644
--- 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_filename = INLINE_FILE_TAG;
         client_inline = (const char *)BPTR(&client_key_pem);
     }
diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c b/tests/unit_tests/openvpn/test_tls_crypt.c
index 366b48d5..f4b1f860 100644
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -530,7 +530,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);
@@ -542,7 +543,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 */