@@ -1835,20 +1835,33 @@
return true;
}
-static bool
+/**
+ * Read a string that is encoded as a 2 byte header with the length from the
+ * buffer \c buf. Will return the non-negative value if reading was successful.
+ * The returned value will include the trailing 0 byte.
+ *
+ * If the message is over the capacity or could not be read
+ * it will return the negative length that was in the
+ * header and try to skip the string. If the string cannot be skipped, the
+ * buf will stay at the current position or position + 2
+ */
+static int
read_string(struct buffer *buf, char *str, const unsigned int capacity)
{
const int len = buf_read_u16(buf);
if (len < 1 || len > (int)capacity)
{
- return false;
+ buf_advance(buf, len);
+
+ /* will also return 0 for a no string being present */
+ return -len;
}
if (!buf_read(buf, str, len))
{
- return false;
+ return -len;
}
str[len-1] = '\0';
- return true;
+ return len;
}
static char *
@@ -2218,8 +2231,6 @@
{
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
- bool username_status, password_status;
-
struct gc_arena gc = gc_new();
char *options;
struct user_pass *up = NULL;
@@ -2253,7 +2264,7 @@
}
/* get options */
- if (!read_string(buf, options, TLS_OPTIONS_LEN))
+ if (read_string(buf, options, TLS_OPTIONS_LEN) < 0)
{
msg(D_TLS_ERRORS, "TLS Error: Failed to read required OCC options string");
goto error;
@@ -2266,8 +2277,11 @@
* peer_info data which follows behind
*/
ALLOC_OBJ_CLEAR_GC(up, struct user_pass, &gc);
- username_status = read_string(buf, up->username, USER_PASS_LEN);
- password_status = read_string(buf, up->password, USER_PASS_LEN);
+ int username_len = read_string(buf, up->username, USER_PASS_LEN);
+ int password_len = read_string(buf, up->password, USER_PASS_LEN);
+
+ msg(D_TLS_ERRORS, "TLS INFO: Username (%d) or password (%d) long",
+ abs(username_len), abs(password_len));
/* get peer info from control channel */
free(multi->peer_info);
@@ -2290,10 +2304,21 @@
multi->remote_ciphername = string_alloc("none", NULL);
}
- if (tls_session_user_pass_enabled(session))
+ if (username_len < 0 || password_len < 0)
+ {
+ msg(D_TLS_ERRORS, "TLS Error: Username (%d) or password (%d) too long",
+ abs(username_len), abs(password_len));
+ auth_set_client_reason(multi, "Username or password is too long. "
+ "Maximum length is 128 bytes");
+
+ /* treat the same as failed username/password and do not error
+ * out (goto error) to sent an AUTH_FAILED back to the client */
+ ks->authenticated = KS_AUTH_FALSE;
+ }
+ else if (tls_session_user_pass_enabled(session))
{
/* Perform username/password authentication */
- if (!username_status || !password_status)
+ if (!username_len || !password_len)
{
CLEAR(*up);
if (!(session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL))
Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit http://gerrit.openvpn.net/c/openvpn/+/787?usp=email to review the following change. Change subject: Refuse clients if username or password is > USER_PASS_LEN ...................................................................... Refuse clients if username or password is > 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. Using the openvpn3 test client with overlong username/password also works. Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- M src/openvpn/ssl.c 1 file changed, 36 insertions(+), 11 deletions(-) git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/87/787/1