From patchwork Fri Oct 25 14:45:34 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "ordex (Code Review)" X-Patchwork-Id: 3914 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:3a4c:b0:5b9:581e:f939 with SMTP id n12csp1064560mao; Fri, 25 Oct 2024 07:46:03 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCXtg/wR+jCpYfsGIBIUavEj3XspZ1rVEriWlj+c8nMCjo2nnjwDs1sIh2MCq7WwOn3gPUlEDAFsN6M=@openvpn.net X-Google-Smtp-Source: AGHT+IF/hwrI8MzO+gEXifNyKjIFWjJzWS1z/QPXgf7U4BURGiMVcE9tgZSbo/jb7C+wTNB+Kks3 X-Received: by 2002:a05:6871:708:b0:26f:ddfa:3571 with SMTP id 586e51a60fabf-28ced115c64mr5685384fac.2.1729867562739; Fri, 25 Oct 2024 07:46:02 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1729867562; cv=none; d=google.com; s=arc-20240605; b=aSRYYuumSoZdUYvejvnZeFEowD/jpqNsT/bE7qETeOo2Z6VGNSrPUXcUOkn7nmG6M2 wt0QbvKoPrfEjMLZYNLtJ2WV+BcdBXq18M0qTf2/P4mR0lG0Ojq67yxMIalEYLFJ00Bt CMpyHX3vOxftBYtYMmMemkjmbeJB/8NZiafC7lNbRKzmIaMZ2z/YldLc+BmN36HdmKXv 1olHxSIdjfd7qALnxOG1cbOzSgx5XHO0jd/GWuVPFZVWs6eywhwR5G3T7P/LnJZ0hcRn Y9g6S1UmO9+psYswkJ7dkm228Vm7zRwaV8RvS9CeMeygFcegS/pF/Uj1vJ09FVW0AUkg iyMA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:cc:reply-to:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence:subject:user-agent :mime-version:message-id:references:auto-submitted:to:date:from :dkim-signature:dkim-signature:dkim-signature; bh=chdZU6zqsxMuQqiQRL1vq1KlCysRw9p6YFPo1OkIswI=; fh=lm0MLPW7DntlrDqRECIiC9JlE1uPxhepE0URYHIf+eE=; b=ckGwYnKXdrOBnU73dWfOlO22HolwGZ5MNxmbnldMT28VcGhZVzNe3TbmSlSvRB7zYI XxwB3McYxiC0M8Iecx/xOOvb1kYPRPdLfeFLhRnbUGCqzNQvsHMeNTsWdcED8dzIfEc5 LWsm5ZS9kNg/iSoff7YFOCX6Wst25/bDFtpqxYPD+SpQxj2GhXZ1Bt3HNyFZQkRD34Sf 1P8lhb39w7qKsrJUqd46R75yoQPuuKEq5MySMJkfGYS7jBgMhAcW5v6yKw2fpaKaT/c/ NXJjn//W0gA4+SV9CmGop3JoRsQyqUjyKI5YsTdJ+xK46m1IEjLp09dMwLHt011WHeid YrYQ==; dara=google.com ARC-Authentication-Results: i=1; mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=mkdAiidO; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=YLCQBWtF; dkim=neutral (body hash did not verify) header.i=@openvpn.net header.s=google header.b=UiqkUXWy; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=openvpn.net; dara=fail header.i=@openvpn.net Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 586e51a60fabf-290357651c6si708862fac.32.2024.10.25.07.46.02 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 25 Oct 2024 07:46:02 -0700 (PDT) Received-SPF: pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) client-ip=216.105.38.7; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@sourceforge.net header.s=x header.b=mkdAiidO; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=YLCQBWtF; dkim=neutral (body hash did not verify) header.i=@openvpn.net header.s=google header.b=UiqkUXWy; spf=pass (google.com: domain of openvpn-devel-bounces@lists.sourceforge.net designates 216.105.38.7 as permitted sender) smtp.mailfrom=openvpn-devel-bounces@lists.sourceforge.net; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=openvpn.net; dara=fail header.i=@openvpn.net Received: from [127.0.0.1] (helo=sfs-ml-2.v29.lw.sourceforge.com) by sfs-ml-2.v29.lw.sourceforge.com with esmtp (Exim 4.95) (envelope-from ) id 1t4LZW-0000Cp-2G; Fri, 25 Oct 2024 14:45:50 +0000 Received: from [172.30.29.66] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1t4LZU-0000Cc-KT for openvpn-devel@lists.sourceforge.net; Fri, 25 Oct 2024 14:45:48 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Type:Content-Transfer-Encoding:MIME-Version :Message-ID:Reply-To:References:Subject:List-Unsubscribe:List-Id:Cc:To:Date: From:Sender:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-Reply-To:List-Help: List-Subscribe:List-Post:List-Owner:List-Archive; bh=iO3uNxKDmuU45p5d93HaWflYhxNkTTHGpm+rRqvDR18=; b=mkdAiidO9FxHWL909dkqU7ZtaM /h4QuZev4INR2Ca7DksuK0ZycVSQwyTqx2Cenz85MiRCSBnFnGZeOaj/xRvZAL0UvTpY4brdtD+aL z/uxPRpBJ/CXeKFJ+itw8VFMszHlrIgYLgOC1ZsKJTozdInVlmzR60NjK35ieCW/k4FU=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Type:Content-Transfer-Encoding:MIME-Version:Message-ID:Reply-To: References:Subject:List-Unsubscribe:List-Id:Cc:To:Date:From:Sender:Content-ID :Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To: Resent-Cc:Resent-Message-ID:In-Reply-To:List-Help:List-Subscribe:List-Post: List-Owner:List-Archive; bh=iO3uNxKDmuU45p5d93HaWflYhxNkTTHGpm+rRqvDR18=; b=Y LCQBWtF7o2sZ7eRH/seLXWzAE3aTKH9tvwTFWSF+Yb4TpD5Fcws7/Hj4JStmclLJurg8Yb/CZ5ZtT ATW18KjyTPc9PMXtj5auz5s8ovRXQKF79iy9E0/tKE1+Jir4AScD8yaYgbBNVPJLjF4BJuSuALTJm BnLE35zba5jYzheY=; Received: from mail-lf1-f50.google.com ([209.85.167.50]) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.95) id 1t4LZT-0006Vy-7N for openvpn-devel@lists.sourceforge.net; Fri, 25 Oct 2024 14:45:48 +0000 Received: by mail-lf1-f50.google.com with SMTP id 2adb3069b0e04-539e6c754bdso1987017e87.2 for ; Fri, 25 Oct 2024 07:45:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=openvpn.net; s=google; t=1729867535; x=1730472335; darn=lists.sourceforge.net; h=user-agent:content-disposition:content-transfer-encoding :mime-version:message-id:reply-to:references:subject :list-unsubscribe:list-id:auto-submitted:cc:to:date:from:from:to:cc :subject:date:message-id:reply-to; bh=iO3uNxKDmuU45p5d93HaWflYhxNkTTHGpm+rRqvDR18=; b=UiqkUXWy9ZH9Oaau9B41+hi3h6Pl+ontNrhbb189KB7mHmgdpCK6NiGpdOOTTPbqkb Ekq+l6+o4sRMYfnqaKwgF3VRzj1yKON0Ov24d8dGK71uZGXpDC27V7pC0exDI//joNYF O4ikiGNxdl/CEaIa1SPm4+imDBOW7f40umWl1ljaYxmHa4rCHMCwRJrYKUSwTkrqdN7Y qlh0EGAo2wyjxmClPJSy9up5uhhfJS715qU+Vr+4kr3MtiFgzl8gGki0gjkMCxkWAhYv KgWqf9wasg3CeQi5N3Ppts1pvHVmlTkgATK0DCHCizLaT2JVcasxCHmSGIHLd4y3PBl4 7Ajw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1729867535; x=1730472335; h=user-agent:content-disposition:content-transfer-encoding :mime-version:message-id:reply-to:references:subject :list-unsubscribe:list-id:auto-submitted:cc:to:date:from :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=iO3uNxKDmuU45p5d93HaWflYhxNkTTHGpm+rRqvDR18=; b=k2z7n1i+ZA2eklCRF6QXdxHMije11jPw2lTRkwnJ2KLw00PhT5eoF9m9qN3dID4Y4P N/LFDIzAP6EMbEAV5uI/TqloMwCq13N/DxFE+8FlUrxy8Ev9KUHBbJEmKPai0DYvMoGr M4Q0IuQdLn1Mvo0GIAJrCUnvvkDMvBzz9LYmApciVU1Fm2ajauHVTOni/o8hoMZHH2J6 0bCAnzMjZghkoSSTN6OGaFWb3+DD2K0TthpjVmRZm98rhb1nydQ6Af2EzWBMmJFmEF+1 0rB115BkJ3CBKl3ShYUY+5awC7g27LuVLDWZXjydivj9juSRq24sm23nsNYpBY7yyLby jNjQ== X-Gm-Message-State: AOJu0Yxg/aCC1nibPKnyKyAbUkyC0/X71R+ZTA2H90ffrl7xjdom+/AO scLtSCEcQLXQp3G++cFTZgp1i+aT+ZAnt6vzrw6PLye5QaDkV95+mTL5Bv83Jb+lT604oqBZ+X4 s X-Received: by 2002:a05:6512:ad5:b0:539:e2cc:d380 with SMTP id 2adb3069b0e04-53b1a341cb6mr5319293e87.27.1729867534918; Fri, 25 Oct 2024 07:45:34 -0700 (PDT) Received: from gerrit.openvpn.in (ec2-18-159-0-78.eu-central-1.compute.amazonaws.com. [18.159.0.78]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38058b13246sm1694335f8f.11.2024.10.25.07.45.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Oct 2024 07:45:34 -0700 (PDT) From: "plaisthos (Code Review)" X-Google-Original-From: "plaisthos (Code Review)" X-Gerrit-PatchSet: 1 Date: Fri, 25 Oct 2024 14:45:34 +0000 To: flichtenheld Auto-Submitted: auto-generated X-Gerrit-MessageType: newchange X-Gerrit-Change-Id: I60f02c919767eb8f1b95253689a8233f5f68621d X-Gerrit-Change-Number: 787 X-Gerrit-Project: openvpn X-Gerrit-ChangeURL: X-Gerrit-Commit: 7143f3d32c26b202c24b9e5eb5ec890043144ebf References: Message-ID: MIME-Version: 1.0 User-Agent: Gerrit/3.8.2 X-Spam-Score: -1.9 (-) X-Spam-Report: Spam detection software, running on the system "util-spamd-2.v13.lw.sourceforge.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see the administrator of that system for details. Content preview: Attention is currently required from: flichtenheld. Hello flichtenheld, I'd like you to do a code review. Please visit Content analysis details: (-1.9 points, 6.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [209.85.167.50 listed in list.dnswl.org] -1.7 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.167.50 listed in wl.mailspike.net] 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 SPF_PASS SPF: sender matches SPF record 0.0 WEIRD_PORT URI: Uses non-standard port number for HTTP 0.0 HTML_MESSAGE BODY: HTML included in message -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.0 T_KAM_HTML_FONT_INVALID Test for Invalidly Named or Formatted Colors in HTML X-Headers-End: 1t4LZT-0006Vy-7N Subject: [Openvpn-devel] [S] Change in openvpn[master]: Refuse clients if username or password is > USER_PASS_LEN X-BeenThere: openvpn-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: arne-openvpn@rfc2549.org, openvpn-devel@lists.sourceforge.net, frank@lichtenheld.com Cc: openvpn-devel Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1813897609517833585?= X-GMAIL-MSGID: =?utf-8?q?1813897609517833585?= X-getmail-filter-classifier: gerrit message type newchange 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 --- 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 diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index e2be614..e5f4f2b 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -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))