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

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

Commit Message

Arne Schwabe April 6, 2020, 1 p.m.
crypto_pem_encode put a nul-terminated terminated string into the
buffer. This is useful for printf but should not be written into
the file.

Also for static keys, we were missing the nul termination when priting
it to stadout but since the buffer was cleared before, there was always
a NULL byte in the right place. Make it explicit instead.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/crypto.c    | 11 +++++++++--
 src/openvpn/tls_crypt.c | 10 ++++++++--
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

tincanteksup April 6, 2020, 1:57 p.m. | #1
I have tested this fix all the way to ensuring that tls-cypt-v2.keys are 
created successfully and do allow successful VPN connection.

I have not tested auth-tokens.

Tested-by: Richard Bonhomme <tincanteksup@gmail.com>

On 06/04/2020 14:00, Arne Schwabe wrote:
> crypto_pem_encode put a nul-terminated terminated string into the
> buffer. This is useful for printf but should not be written into
> the file.
> 
> Also for static keys, we were missing the nul termination when priting
> it to stadout but since the buffer was cleared before, there was always
> a NULL byte in the right place. Make it explicit instead.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>   src/openvpn/crypto.c    | 11 +++++++++--
>   src/openvpn/tls_crypt.c | 10 ++++++++--
>   2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 453cb20a..7af48df0 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
>       /* write key file to stdout if no filename given */
>       if (!filename || strcmp(filename, "")==0)
>       {
> +        buf_null_terminate(&out);
>           printf("%s\n", BPTR(&out));
>       }
>       /* write key file, now formatted in out, to file */
> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char *pem_name)
>       {
>           printf("%s\n", BPTR(&server_key_pem));
>       }
> -    else if (!buffer_write_file(filename, &server_key_pem))
> +    else
>       {
> -        msg(M_ERR, "ERROR: could not write key file");
> +        /* crypto_pem_encode null terminates the buffer, do
> +         * not write this to the file */
> +        buf_inc_len(&server_key_pem, -1);
> +        if (!buffer_write_file(filename, &server_key_pem))
> +        {
> +            msg(M_ERR, "ERROR: could not write key file");
> +        }
>           goto cleanup;
>       }
>   
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index e9f9cc2a..85f2862b 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>           client_filename = INLINE_FILE_TAG;
>           client_inline = (const char *)BPTR(&client_key_pem);
>       }
> -    else if (!buffer_write_file(filename, &client_key_pem))
> +    else
>       {
> -        msg(M_FATAL, "ERROR: could not write client key file");
> +        /* crypto_pem_encode null terminates the buffer, do
> +         * not write this to the file */
> +        buf_inc_len(&client_key_pem, -1);
> +        if (!buffer_write_file(filename, &client_key_pem))
> +        {
> +            msg(M_FATAL, "ERROR: could not write client key file");
> +        }
>           goto cleanup;
>       }
>   
>
Steffan Karger April 6, 2020, 3:44 p.m. | #2
Hi,

On 06-04-2020 15:00, Arne Schwabe wrote:
> crypto_pem_encode put a nul-terminated terminated string into the
> buffer. This is useful for printf but should not be written into
> the file.
> 
> Also for static keys, we were missing the nul termination when priting
> it to stadout but since the buffer was cleared before, there was always
> a NULL byte in the right place. Make it explicit instead.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/crypto.c    | 11 +++++++++--
>  src/openvpn/tls_crypt.c | 10 ++++++++--
>  2 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
> index 453cb20a..7af48df0 100644
> --- a/src/openvpn/crypto.c
> +++ b/src/openvpn/crypto.c
> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
>      /* write key file to stdout if no filename given */
>      if (!filename || strcmp(filename, "")==0)
>      {
> +        buf_null_terminate(&out);
>          printf("%s\n", BPTR(&out));
>      }
>      /* write key file, now formatted in out, to file */
> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char *pem_name)
>      {
>          printf("%s\n", BPTR(&server_key_pem));
>      }
> -    else if (!buffer_write_file(filename, &server_key_pem))
> +    else
>      {
> -        msg(M_ERR, "ERROR: could not write key file");
> +        /* crypto_pem_encode null terminates the buffer, do
> +         * not write this to the file */
> +        buf_inc_len(&server_key_pem, -1);
> +        if (!buffer_write_file(filename, &server_key_pem))
> +        {
> +            msg(M_ERR, "ERROR: could not write key file");
> +        }
>          goto cleanup;
>      }
>  
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index e9f9cc2a..85f2862b 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>          client_filename = INLINE_FILE_TAG;
>          client_inline = (const char *)BPTR(&client_key_pem);
>      }
> -    else if (!buffer_write_file(filename, &client_key_pem))
> +    else
>      {
> -        msg(M_FATAL, "ERROR: could not write client key file");
> +        /* crypto_pem_encode null terminates the buffer, do
> +         * not write this to the file */
> +        buf_inc_len(&client_key_pem, -1);
> +        if (!buffer_write_file(filename, &client_key_pem))
> +        {
> +            msg(M_FATAL, "ERROR: could not write client key file");
> +        }
>          goto cleanup;
>      }
>  
> 

Fix is correct, but I'm not too happy with changing the buffer from
being null-terminated to not-null-terminated. This change itself is
correct, but it feels like a recipe for mistakes in the future.

What would you think about adding a length parameter to
buffer_write_file() instead, so that the API there changes to "write len
bytes of data to file"?

-Steffan
Arne Schwabe April 6, 2020, 4 p.m. | #3
Am 06.04.20 um 17:44 schrieb Steffan Karger:
> Hi,
> 
> On 06-04-2020 15:00, Arne Schwabe wrote:
>> crypto_pem_encode put a nul-terminated terminated string into the
>> buffer. This is useful for printf but should not be written into
>> the file.
>>
>> Also for static keys, we were missing the nul termination when priting
>> it to stadout but since the buffer was cleared before, there was always
>> a NULL byte in the right place. Make it explicit instead.
>>
>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>> ---
>>  src/openvpn/crypto.c    | 11 +++++++++--
>>  src/openvpn/tls_crypt.c | 10 ++++++++--
>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>> index 453cb20a..7af48df0 100644
>> --- a/src/openvpn/crypto.c
>> +++ b/src/openvpn/crypto.c
>> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
>>      /* write key file to stdout if no filename given */
>>      if (!filename || strcmp(filename, "")==0)
>>      {
>> +        buf_null_terminate(&out);
>>          printf("%s\n", BPTR(&out));
>>      }
>>      /* write key file, now formatted in out, to file */
>> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char *pem_name)
>>      {
>>          printf("%s\n", BPTR(&server_key_pem));
>>      }
>> -    else if (!buffer_write_file(filename, &server_key_pem))
>> +    else
>>      {
>> -        msg(M_ERR, "ERROR: could not write key file");
>> +        /* crypto_pem_encode null terminates the buffer, do
>> +         * not write this to the file */
>> +        buf_inc_len(&server_key_pem, -1);
>> +        if (!buffer_write_file(filename, &server_key_pem))
>> +        {
>> +            msg(M_ERR, "ERROR: could not write key file");
>> +        }
>>          goto cleanup;
>>      }
>>  
>> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
>> index e9f9cc2a..85f2862b 100644
>> --- a/src/openvpn/tls_crypt.c
>> +++ b/src/openvpn/tls_crypt.c
>> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>>          client_filename = INLINE_FILE_TAG;
>>          client_inline = (const char *)BPTR(&client_key_pem);
>>      }
>> -    else if (!buffer_write_file(filename, &client_key_pem))
>> +    else
>>      {
>> -        msg(M_FATAL, "ERROR: could not write client key file");
>> +        /* crypto_pem_encode null terminates the buffer, do
>> +         * not write this to the file */
>> +        buf_inc_len(&client_key_pem, -1);
>> +        if (!buffer_write_file(filename, &client_key_pem))
>> +        {
>> +            msg(M_FATAL, "ERROR: could not write client key file");
>> +        }
>>          goto cleanup;
>>      }
>>  
>>
> 
> Fix is correct, but I'm not too happy with changing the buffer from
> being null-terminated to not-null-terminated. This change itself is
> correct, but it feels like a recipe for mistakes in the future.
> 
> What would you think about adding a length parameter to
> buffer_write_file() instead, so that the API there changes to "write len
> bytes of data to file"?

So there are three alternatives how to things:

a) make pem_encode behave more like other similar functions in OepnVPN
and do not null terminate. Then we would need to call buf_null_terminate
like in the static key path

b) do what this patch does

c) have buffer_write_file take a length argument.

I considered c) but it felt wrong because the function currently just
does a very simple job.

I did not do a) because that would make printing the buffer etc more
complicated since either pem_encode needs to allocate a bigger buffer
than it needs to or we need to copy the buffer into one that allows an
extra null byte.

That being said I don't feel stronlgy about this and can send an updated
patch that is a) or c)

Arne
Steffan Karger April 6, 2020, 4:16 p.m. | #4
On 06-04-2020 18:00, Arne Schwabe wrote:
> Am 06.04.20 um 17:44 schrieb Steffan Karger:
>> Hi,
>>
>> On 06-04-2020 15:00, Arne Schwabe wrote:
>>> crypto_pem_encode put a nul-terminated terminated string into the
>>> buffer. This is useful for printf but should not be written into
>>> the file.
>>>
>>> Also for static keys, we were missing the nul termination when priting
>>> it to stadout but since the buffer was cleared before, there was always
>>> a NULL byte in the right place. Make it explicit instead.
>>>
>>> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
>>> ---
>>>  src/openvpn/crypto.c    | 11 +++++++++--
>>>  src/openvpn/tls_crypt.c | 10 ++++++++--
>>>  2 files changed, 17 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
>>> index 453cb20a..7af48df0 100644
>>> --- a/src/openvpn/crypto.c
>>> +++ b/src/openvpn/crypto.c
>>> @@ -1477,6 +1477,7 @@ write_key_file(const int nkeys, const char *filename)
>>>      /* write key file to stdout if no filename given */
>>>      if (!filename || strcmp(filename, "")==0)
>>>      {
>>> +        buf_null_terminate(&out);
>>>          printf("%s\n", BPTR(&out));
>>>      }
>>>      /* write key file, now formatted in out, to file */
>>> @@ -1888,9 +1889,15 @@ write_pem_key_file(const char *filename, const char *pem_name)
>>>      {
>>>          printf("%s\n", BPTR(&server_key_pem));
>>>      }
>>> -    else if (!buffer_write_file(filename, &server_key_pem))
>>> +    else
>>>      {
>>> -        msg(M_ERR, "ERROR: could not write key file");
>>> +        /* crypto_pem_encode null terminates the buffer, do
>>> +         * not write this to the file */
>>> +        buf_inc_len(&server_key_pem, -1);
>>> +        if (!buffer_write_file(filename, &server_key_pem))
>>> +        {
>>> +            msg(M_ERR, "ERROR: could not write key file");
>>> +        }
>>>          goto cleanup;
>>>      }
>>>  
>>> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
>>> index e9f9cc2a..85f2862b 100644
>>> --- a/src/openvpn/tls_crypt.c
>>> +++ b/src/openvpn/tls_crypt.c
>>> @@ -706,9 +706,15 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>>>          client_filename = INLINE_FILE_TAG;
>>>          client_inline = (const char *)BPTR(&client_key_pem);
>>>      }
>>> -    else if (!buffer_write_file(filename, &client_key_pem))
>>> +    else
>>>      {
>>> -        msg(M_FATAL, "ERROR: could not write client key file");
>>> +        /* crypto_pem_encode null terminates the buffer, do
>>> +         * not write this to the file */
>>> +        buf_inc_len(&client_key_pem, -1);
>>> +        if (!buffer_write_file(filename, &client_key_pem))
>>> +        {
>>> +            msg(M_FATAL, "ERROR: could not write client key file");
>>> +        }
>>>          goto cleanup;
>>>      }
>>>  
>>>
>>
>> Fix is correct, but I'm not too happy with changing the buffer from
>> being null-terminated to not-null-terminated. This change itself is
>> correct, but it feels like a recipe for mistakes in the future.
>>
>> What would you think about adding a length parameter to
>> buffer_write_file() instead, so that the API there changes to "write len
>> bytes of data to file"?
> 
> So there are three alternatives how to things:
> 
> a) make pem_encode behave more like other similar functions in OepnVPN
> and do not null terminate. Then we would need to call buf_null_terminate
> like in the static key path
> 
> b) do what this patch does
> 
> c) have buffer_write_file take a length argument.
> 
> I considered c) but it felt wrong because the function currently just
> does a very simple job.

I felt that too, but slightly less so than having buffers that are
sometimes null-terminated and sometimes not. Slight different preference
I guess.

> I did not do a) because that would make printing the buffer etc more
> complicated since either pem_encode needs to allocate a bigger buffer
> than it needs to or we need to copy the buffer into one that allows an
> extra null byte.

I originally discarded this idea, but thinking a bit more about it, I
think it makes sense. You don't have to allocate extra bytes, I think
the "%.*s" format specifier should work just fine:

   printf("%.*s", BLEN(buf), BPTR(buf));

> That being said I don't feel stronlgy about this and can send an updated
> patch that is a) or c)

Same here. Hence the fuzzy wording. Does my suggestion make you like c)
more? If you still like b) most, I'm okay with sticking to it. Neither
option excels in beauty :(

-Steffan

Patch

diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 453cb20a..7af48df0 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1477,6 +1477,7 @@  write_key_file(const int nkeys, const char *filename)
     /* write key file to stdout if no filename given */
     if (!filename || strcmp(filename, "")==0)
     {
+        buf_null_terminate(&out);
         printf("%s\n", BPTR(&out));
     }
     /* write key file, now formatted in out, to file */
@@ -1888,9 +1889,15 @@  write_pem_key_file(const char *filename, const char *pem_name)
     {
         printf("%s\n", BPTR(&server_key_pem));
     }
-    else if (!buffer_write_file(filename, &server_key_pem))
+    else
     {
-        msg(M_ERR, "ERROR: could not write key file");
+        /* crypto_pem_encode null terminates the buffer, do
+         * not write this to the file */
+        buf_inc_len(&server_key_pem, -1);
+        if (!buffer_write_file(filename, &server_key_pem))
+        {
+            msg(M_ERR, "ERROR: could not write key file");
+        }
         goto cleanup;
     }
 
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index e9f9cc2a..85f2862b 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -706,9 +706,15 @@  tls_crypt_v2_write_client_key_file(const char *filename,
         client_filename = INLINE_FILE_TAG;
         client_inline = (const char *)BPTR(&client_key_pem);
     }
-    else if (!buffer_write_file(filename, &client_key_pem))
+    else
     {
-        msg(M_FATAL, "ERROR: could not write client key file");
+        /* crypto_pem_encode null terminates the buffer, do
+         * not write this to the file */
+        buf_inc_len(&client_key_pem, -1);
+        if (!buffer_write_file(filename, &client_key_pem))
+        {
+            msg(M_FATAL, "ERROR: could not write client key file");
+        }
         goto cleanup;
     }