diff mbox series

[Openvpn-devel,1/5] Fix unaligned access in auth-token

Message ID 20230130172936.3444840-1-arne@rfc2549.org
State Accepted
Headers
Series [Openvpn-devel,1/5] Fix unaligned access in auth-token |

Commit Message

Arne Schwabe Jan. 30, 2023, 5:29 p.m. UTC 
  The undefined behaviour USAN clang checker found this. The optimiser
of clang/gcc will optimise the memcpy away in the auth_token case and output
excactly the same assembly on amd64/arm64 but it is still better to not rely
on undefined behaviour.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/auth_token.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Frank Lichtenheld Jan. 31, 2023, 10:52 a.m. UTC | #1 
On Mon, Jan 30, 2023 at 06:29:32PM +0100, Arne Schwabe wrote:
> The undefined behaviour USAN clang checker found this. The optimiser
> of clang/gcc will optimise the memcpy away in the auth_token case and output
> excactly the same assembly on amd64/arm64 but it is still better to not rely
> on undefined behaviour.
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/auth_token.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
> index 7b963a9c5..e4486eb08 100644
> --- a/src/openvpn/auth_token.c
> +++ b/src/openvpn/auth_token.c
> @@ -324,8 +324,14 @@ verify_auth_token(struct user_pass *up, struct tls_multi *multi,
>      const uint8_t *tstamp_initial = sessid + AUTH_TOKEN_SESSION_ID_LEN;
>      const uint8_t *tstamp = tstamp_initial + sizeof(int64_t);
>  
> -    uint64_t timestamp = ntohll(*((uint64_t *) (tstamp)));
> -    uint64_t timestamp_initial = ntohll(*((uint64_t *) (tstamp_initial)));
> +    /* tstamp, tstamp_initial might not be aligned to an uint64, use memcpy
> +     * to avoid unaligned access */
> +    uint64_t timestamp = 0, timestamp_initial = 0;
> +    memcpy(&timestamp, tstamp, sizeof(uint64_t));
> +    timestamp = ntohll(timestamp);
> +
> +    memcpy(&timestamp_initial, tstamp_initial, sizeof(uint64_t));
> +    timestamp_initial = ntohll(timestamp_initial);
>  
>      hmac_ctx_t *ctx = multi->opt.auth_token_key.hmac;
>      if (check_hmac_token(ctx, b64decoded, up->username))

Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

Trivial enough.

Regards,
Gert Doering Feb. 1, 2023, 4:24 p.m. UTC | #2 
This one is fairly straightforward.  We had discussions on IRC if
we really need to make this network byte order ("the data in the token
is only for the server, nobody else needs to care") but then decided
that if someone wants to run a cluster of an intel + a s390 host,
they need to be compatible wrt timestamp representation...

Tested on the server testbed, just to be sure (it has an auth-token test) -
things still work, including token expiry after the configured time.

Your patch has been applied to the master and release/2.6 branch (bugfix).

commit f6ccff6d7ea806711f9af59c9de52b7cf80d9c81 (master)
commit 6241b2f8dbe39062a3273499a0259750d2f02cf8 (release/2.6)
Author: Arne Schwabe
Date:   Mon Jan 30 18:29:32 2023 +0100

     Fix unaligned access in auth-token

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Frank Lichtenheld <frank@lichtenheld.com>
     Message-Id: <20230130172936.3444840-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26103.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c
index 7b963a9c5..e4486eb08 100644
--- a/src/openvpn/auth_token.c
+++ b/src/openvpn/auth_token.c
@@ -324,8 +324,14 @@  verify_auth_token(struct user_pass *up, struct tls_multi *multi,
     const uint8_t *tstamp_initial = sessid + AUTH_TOKEN_SESSION_ID_LEN;
     const uint8_t *tstamp = tstamp_initial + sizeof(int64_t);
 
-    uint64_t timestamp = ntohll(*((uint64_t *) (tstamp)));
-    uint64_t timestamp_initial = ntohll(*((uint64_t *) (tstamp_initial)));
+    /* tstamp, tstamp_initial might not be aligned to an uint64, use memcpy
+     * to avoid unaligned access */
+    uint64_t timestamp = 0, timestamp_initial = 0;
+    memcpy(&timestamp, tstamp, sizeof(uint64_t));
+    timestamp = ntohll(timestamp);
+
+    memcpy(&timestamp_initial, tstamp_initial, sizeof(uint64_t));
+    timestamp_initial = ntohll(timestamp_initial);
 
     hmac_ctx_t *ctx = multi->opt.auth_token_key.hmac;
     if (check_hmac_token(ctx, b64decoded, up->username))