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 <a...@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
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
