Browse code

Refuse clients if username or password is longer than USER_PASS_LEN

When OpenVPN is compiled without PKCS11 support USER_PASS_LEN is 128
bytes. If we encounter a username larger than this length, we would
only read the 2 bytes length header of the username/password. We did
then also NOT skip the username or password field meaning that we would
continue reading the rest of the packet at the wrong offset and get
garbage results like not having peerinfo and then rejecting a client
because of no common cipher or missing data v2 support.

This will tell the client that username/password is too regardless
of whether password/username authentication is used. This way we
do not leak if username/password authentication is active.

To reproduce this issue have the server compiled with a USER_PASS_LEN
set to 128 (e.g. without pkcs11 or manually adjusting the define) and
have the client with a larger USER_PASS_LEN to actually be able to
send the larger password. The server must also be set to use only
certificate authentication while the client must use certificates
and auth-user-pass because otherwise the user/pass verification will
reject the empty credentials.

Using the openvpn3 test client with overlong username/password also
works.

Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <20241028135505.28651-1-gert@greenie.muc.de>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg29675.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>
(cherry picked from commit a7f80d402fb95df3c58a8fc5d12cdb8f39c37d3e)

Arne Schwabe authored on 2024/10/28 22:55:04
Showing 1 changed files
... ...
@@ -1974,20 +1974,33 @@ write_string(struct buffer *buf, const char *str, const int maxlen)
1974 1974
     return true;
1975 1975
 }
1976 1976
 
1977
-static bool
1977
+/**
1978
+ * Read a string that is encoded as a 2 byte header with the length from the
1979
+ * buffer \c buf. Will return the non-negative value if reading was successful.
1980
+ * The returned value will include the trailing 0 byte.
1981
+ *
1982
+ * If the message is over the capacity or could not be read
1983
+ * it will return the negative length that was in the
1984
+ * header and try to skip the string. If the string cannot be skipped, the
1985
+ * buf will stay at the current position or position + 2
1986
+ */
1987
+static int
1978 1988
 read_string(struct buffer *buf, char *str, const unsigned int capacity)
1979 1989
 {
1980 1990
     const int len = buf_read_u16(buf);
1981 1991
     if (len < 1 || len > (int)capacity)
1982 1992
     {
1983
-        return false;
1993
+        buf_advance(buf, len);
1994
+
1995
+        /* will also return 0 for a no string being present */
1996
+        return -len;
1984 1997
     }
1985 1998
     if (!buf_read(buf, str, len))
1986 1999
     {
1987
-        return false;
2000
+        return -len;
1988 2001
     }
1989 2002
     str[len-1] = '\0';
1990
-    return true;
2003
+    return len;
1991 2004
 }
1992 2005
 
1993 2006
 static char *
... ...
@@ -2357,8 +2370,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
2357 2357
 {
2358 2358
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
2359 2359
 
2360
-    bool username_status, password_status;
2361
-
2362 2360
     struct gc_arena gc = gc_new();
2363 2361
     char *options;
2364 2362
     struct user_pass *up = NULL;
... ...
@@ -2392,7 +2403,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
2392 2392
     }
2393 2393
 
2394 2394
     /* get options */
2395
-    if (!read_string(buf, options, TLS_OPTIONS_LEN))
2395
+    if (read_string(buf, options, TLS_OPTIONS_LEN) < 0)
2396 2396
     {
2397 2397
         msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string");
2398 2398
         goto error;
... ...
@@ -2405,8 +2416,8 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
2405 2405
      * peer_info data which follows behind
2406 2406
      */
2407 2407
     ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc);
2408
-    username_status = read_string(buf, up->username, USER_PASS_LEN);
2409
-    password_status = read_string(buf, up->password, USER_PASS_LEN);
2408
+    int username_len = read_string(buf, up->username, USER_PASS_LEN);
2409
+    int password_len = read_string(buf, up->password, USER_PASS_LEN);
2410 2410
 
2411 2411
     /* get peer info from control channel */
2412 2412
     free(multi->peer_info);
... ...
@@ -2429,10 +2440,21 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
2429 2429
         multi->remote_ciphername = string_alloc("none", NULL);
2430 2430
     }
2431 2431
 
2432
-    if (tls_session_user_pass_enabled(session))
2432
+    if (username_len < 0 || password_len < 0)
2433
+    {
2434
+        msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long",
2435
+            abs(username_len), abs(password_len));
2436
+        auth_set_client_reason(multi, "Username or password is too long. "
2437
+                               "Maximum length is 128 bytes");
2438
+
2439
+        /* treat the same as failed username/password and do not error
2440
+         * out (goto error) to sent an AUTH_FAILED back to the client */
2441
+        ks->authenticated = KS_AUTH_FALSE;
2442
+    }
2443
+    else if (tls_session_user_pass_enabled(session))
2433 2444
     {
2434 2445
         /* Perform username/password authentication */
2435
-        if (!username_status || !password_status)
2446
+        if (!username_len || !password_len)
2436 2447
         {
2437 2448
             CLEAR(*up);
2438 2449
             if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL))