[Openvpn-devel] Fix off-by-one in tls-crypt-v2 client wrapping with custom metadata

Message ID 20200403090944.17726-1-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel] Fix off-by-one in tls-crypt-v2 client wrapping with custom metadata
Related show

Commit Message

Arne Schwabe April 3, 2020, 9:09 a.m.
Instead of writing at the end of the metadata buffer, the decoded
base64 data overwrites the opcode as BPTR points to the beginning
of the buffer and not the current position. Replace with BEND to
fix this off-by-one

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/tls_crypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Steffan Karger April 6, 2020, 3:20 p.m. | #1
Hi,

On 03-04-2020 11:09, Arne Schwabe wrote:
> Instead of writing at the end of the metadata buffer, the decoded
> base64 data overwrites the opcode as BPTR points to the beginning
> of the buffer and not the current position. Replace with BEND to
> fix this off-by-one
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/tls_crypt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
> index 37df2ce7..e9f9cc2a 100644
> --- a/src/openvpn/tls_crypt.c
> +++ b/src/openvpn/tls_crypt.c
> @@ -664,7 +664,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
>                  (int)strlen(b64_metadata), TLS_CRYPT_V2_MAX_B64_METADATA_LEN);
>          }
>          ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_USER, 1));
> -        int decoded_len = openvpn_base64_decode(b64_metadata, BPTR(&metadata),
> +        int decoded_len = openvpn_base64_decode(b64_metadata, BEND(&metadata),
>                                                  BCAP(&metadata));
>          if (decoded_len < 0)
>          {
> 

Good catch. And apologies for the silly bug. Patch looks good, but it
would have been nice to add a regression (unit) test. Are you willing to
write one? Otherwise I will.

Acked-by: Steffan Karger <steffan.karger@foxcrypto.com>

-Steffan
Gert Doering April 6, 2020, 3:45 p.m. | #2
Your patch has been applied to the master branch.

While a bugfix, the code in question is not in release/2.4, so no need
to backport.

No real testing done, just a quick test compile.

commit e23fb6b8c88a2aec160965769f6467d455c0d010 (master)
Author: Arne Schwabe
Date:   Fri Apr 3 11:09:44 2020 +0200

     Fix off-by-one in tls-crypt-v2 client wrapping with custom metadata

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Steffan Karger <steffan.karger@fox-it.com>
     Message-Id: <20200403090944.17726-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19695.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index 37df2ce7..e9f9cc2a 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -664,7 +664,7 @@  tls_crypt_v2_write_client_key_file(const char *filename,
                 (int)strlen(b64_metadata), TLS_CRYPT_V2_MAX_B64_METADATA_LEN);
         }
         ASSERT(buf_write(&metadata, &TLS_CRYPT_METADATA_TYPE_USER, 1));
-        int decoded_len = openvpn_base64_decode(b64_metadata, BPTR(&metadata),
+        int decoded_len = openvpn_base64_decode(b64_metadata, BEND(&metadata),
                                                 BCAP(&metadata));
         if (decoded_len < 0)
         {