| 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 | expand |
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; > } > >
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
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
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
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; }
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(-)