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