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 <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"?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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
