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

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

Commit Message

Arne Schwabe April 20, 2020, 12:06 p.m.
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.

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

Comments

Steffan Karger April 25, 2020, 8:37 a.m. | #1
Hi,

On 20-04-2020 14:06, 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.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/crypto.c                      | 4 ++--
>  src/openvpn/crypto_mbedtls.c              | 4 +++-
>  src/openvpn/crypto_openssl.c              | 3 +--
>  src/openvpn/tls_crypt.c                   | 2 +-
>  tests/unit_tests/openvpn/test_tls_crypt.c | 6 ++++--
>  5 files changed, 11 insertions(+), 8 deletions(-)
> 
> 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..14a528af 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;
> 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 b9e3a7a6..54fc917d 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -512,7 +512,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);
> @@ -524,7 +525,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 */
> 

Didn't review in detail yet, but this patch now fails "make check" with
mbedtls:

[==========] Running 1 test(s).
[ RUN      ] crypto_pem_encode_decode_loopback
PEM decode error: source buffer not null-terminated
PEM decode error: source buffer not null-terminated
[  ERROR   ] --- crypto_pem_decode("TESTKEYNAME", &dec_buf, &pem_buf)
[   LINE   ] --- openvpn/tests/unit_tests/openvpn/test_crypto.c:64:
error: Failure!
[  FAILED  ] crypto_pem_encode_decode_loopback
[==========] 1 test(s) run.
[  PASSED  ] 0 test(s).
[  FAILED  ] 1 test(s), listed below:
[  FAILED  ] crypto_pem_encode_decode_loopback

 1 FAILED TEST(S)
FAIL: crypto_testdriver

With openssl, make check succeeds.

-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..14a528af 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;
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 b9e3a7a6..54fc917d 100644
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -512,7 +512,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);
@@ -524,7 +525,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 */