From patchwork Tue Feb 18 14:54:21 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Selva Nair X-Patchwork-Id: 1011 Return-Path: Delivered-To: patchwork@openvpn.net Delivered-To: patchwork@openvpn.net Received: from director7.mail.ord1d.rsapps.net ([172.30.191.6]) by backend30.mail.ord1d.rsapps.net with LMTP id UHOcBJuVTF6WbAAAIUCqbw for ; Tue, 18 Feb 2020 20:55:39 -0500 Received: from proxy15.mail.ord1d.rsapps.net ([172.30.191.6]) by director7.mail.ord1d.rsapps.net with LMTP id 8ENoBJuVTF6gcgAAovjBpQ ; Tue, 18 Feb 2020 20:55:39 -0500 Received: from smtp7.gate.ord1d ([172.30.191.6]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) by proxy15.mail.ord1d.rsapps.net with LMTP id 8C4tBJuVTF4GYgAAAY1PeQ ; Tue, 18 Feb 2020 20:55:39 -0500 X-Spam-Threshold: 95 X-Spam-Score: 0 X-Spam-Flag: NO X-Virus-Scanned: OK X-Orig-To: openvpnslackdevel@openvpn.net X-Originating-Ip: [216.105.38.7] Authentication-Results: smtp7.gate.ord1d.rsapps.net; iprev=pass policy.iprev="216.105.38.7"; spf=pass smtp.mailfrom="openvpn-devel-bounces@lists.sourceforge.net" smtp.helo="lists.sourceforge.net"; dkim=fail (signature verification failed) header.d=sourceforge.net; dkim=fail (signature verification failed) header.d=sf.net; dkim=fail (signature verification failed) header.d=gmail.com; dmarc=fail (p=none; dis=none) header.from=gmail.com X-Suspicious-Flag: YES X-Classification-ID: ecfb7526-52ba-11ea-afd2-525400d0c497-1-1 Received: from [216.105.38.7] ([216.105.38.7:35830] helo=lists.sourceforge.net) by smtp7.gate.ord1d.rsapps.net (envelope-from ) (ecelerity 4.2.38.62370 r(:)) with ESMTPS (cipher=DHE-RSA-AES256-GCM-SHA384) id AE/B3-27675-A959C4E5; Tue, 18 Feb 2020 20:55:38 -0500 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.90_1) (envelope-from ) id 1j4EZN-0000Hj-J0; Wed, 19 Feb 2020 01:54:33 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-2.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1j4EZM-0000Hc-Mx for openvpn-devel@lists.sourceforge.net; Wed, 19 Feb 2020 01:54:32 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To: MIME-Version:Content-Type:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=KbQZKTukHVV2U4/9WScFxj1WvTezceyxTAnwIDo2fvE=; b=mXPi7yg6lTeCMx/Idf85Iv9L3C L5pjpc8sUerAOTW1wHwrKFK2ga2hVaAeJ2KjwpNViiysvboHsyVY5lXvSPHdjIC5ywpxo7hSC5omy Gz6EiSZf69pQEIv/KugoDRT7LAnridgY1MJypU7ugIQuyPxpIk1dMFUr2mk9QaPx2btU=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Message-Id:Date:Subject:Cc:To:From:Sender:Reply-To:MIME-Version: Content-Type:Content-Transfer-Encoding:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: In-Reply-To:References:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=KbQZKTukHVV2U4/9WScFxj1WvTezceyxTAnwIDo2fvE=; b=KK57sysnhLxnVb8jiWkWMJnc3a XwNuZaotS72PDEBE2gYvAM3BX160VwST98/RSM0gHF0WivwMsMGBENkrOo77ZZXlGMWE98pZatbv+ yXSMkyZUWncct2dnqOGEHswVQkVQkMqlc9Rdt2dEd6FjOn4/YSbYvXLvpjdO3QEdHb+k=; Received: from mail-qk1-f180.google.com ([209.85.222.180]) by sfi-mx-4.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.92.2) id 1j4EZL-007UpV-75 for openvpn-devel@lists.sourceforge.net; Wed, 19 Feb 2020 01:54:32 +0000 Received: by mail-qk1-f180.google.com with SMTP id p7so21610774qkh.10 for ; Tue, 18 Feb 2020 17:54:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id; bh=KbQZKTukHVV2U4/9WScFxj1WvTezceyxTAnwIDo2fvE=; b=rMAp65aUTK+n/n6x4dQH2XqKR5mEQwhrK2Sojt0KKNouSZwpAXcy1Q3somI/hijgbN AFDqGHrzHAqnP5orijDVmf7BOu8VXEasAIHmRCwrt/S/Kt2WMC+1jpDe9f716HVp+nTQ dS5SsXXBnkogVpuP30gGRO7qyOJjxZoN8F9Vd8bxqJGlOLEpIkfOI5cdQHv/Ztmepx/Q qic0kpXftbOa2inKW6ezzaT3hvhsS6ZdlWxW0FZXn4cGf2SVDkKFI/0k20LdXGIGcfbI ga63O7NtXYurtViAlYclx+ZipRxHQV1pRbE7QJfFSLX7qnlkhPCNM4c0uOn6LQo6eq6d cGow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=KbQZKTukHVV2U4/9WScFxj1WvTezceyxTAnwIDo2fvE=; b=edY5FZNn2rOz1XI88234cEu0hjV7GSWfbcErtorylmu1mpDBYnpdJCM5lGv71pEESe qnzf9VfZGyg9Hgmo0rpq0cDckfvK3mMxvGRR/8Xtiq5tgilpTkd0ahXRxd06f/OWIBhg HmA8CAtv42Lx0X+6QVA27Imeu2AgJLcYrutxwjDAMVYLwIPUiV9eXdPxnlSRVER6QoTc Es10HGHA93opi54sHvSj8KbFTwfSZCU3KBpXc5KMPv08p9pFKGFrfqbghz6jfTF52hfI L6Qx3b2Y58zs5akprKdCk0NtBbbxesNnZpxvwo2sp+qVO4KdXGbrkYy51hX+MuZOG/fw g0kw== X-Gm-Message-State: APjAAAXLDCbY6SaKBXYZFyTMRx6fJXAVpjMf+RD92VLAkLtyXwLJILoZ 12SJyGwUje6EmRiIdbUxpVd6VbKQtgY= X-Google-Smtp-Source: APXvYqz9yfHyjtkkL2L3r9SUCXSjgbeWeRej9JSQGv/21eNbqBzSoW7nm8Go70/8yhb7ZZnft2w7Jg== X-Received: by 2002:ae9:dcc1:: with SMTP id q184mr21099270qkf.480.1582077264724; Tue, 18 Feb 2020 17:54:24 -0800 (PST) Received: from saturn.home.sansel.ca (CPE40167ea0e1c2-CM788df74daaa0.cpe.net.cable.rogers.com. [99.228.216.21]) by smtp.gmail.com with ESMTPSA id g18sm279434qki.13.2020.02.18.17.54.23 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 18 Feb 2020 17:54:24 -0800 (PST) From: selva.nair@gmail.com To: openvpn-devel@lists.sourceforge.net Date: Tue, 18 Feb 2020 20:54:21 -0500 Message-Id: <1582077261-9467-1-git-send-email-selva.nair@gmail.com> X-Mailer: git-send-email 2.1.4 X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (selva.nair[at]gmail.com) -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [209.85.222.180 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.0 RCVD_IN_MSPIKE_H2 RBL: Average reputation (+2) [209.85.222.180 listed in wl.mailspike.net] -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid X-Headers-End: 1j4EZL-007UpV-75 Subject: [Openvpn-devel] [PATCH 2.4 v3] Swap the order of checks for validating interactive service user 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: , MIME-Version: 1.0 Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox From: Selva Nair Check the config file location and command line options first and membership in OpenVPNAdministrators group after that as the latter could be a slow process for active directory users. When connection to domain controllers is poor or unavailable, checking the group membership is slow and causes timeouts in the GUI (Trac 1051). However, in cases where the config is in the global directory, no group membership check should be required. The re-ordering here avoids the redundant check in such cases. In addition to this, its also proposed to improve the timeout handling in the GUI, but this change is still useful as it should completely eliminate the timeout issue for many users. v3: Do not send error message to the client pipe from ValidateOptions(). Instead save the error and send it on only if user authorization also fails. The error buffer size is increased to 512 wide chars as these messages could get long in some cases and may get truncated otherwise. Also see: https://github.com/OpenVPN/openvpn-gui/issues/332 Signed-off-by: Selva Nair Acked-by: Lev Stipakov --- cherry-picked from commit c6cc66a13568dd1078bfbeb763998c1b9e2a2999 with one change: - openvpn_swprintf() -> swprintf() as the latter is not readily accessible here in 2.4 src/openvpnserv/interactive.c | 39 ++++++++++++++++++++++----------------- 1 file changed, 22 insertions(+), 17 deletions(-) diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index d7c9eea..a2b3b20 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -360,14 +360,13 @@ ReturnOpenvpnOutput(HANDLE pipe, HANDLE ovpn_output, DWORD count, LPHANDLE event /* * Validate options against a white list. Also check the config_file is * inside the config_dir. The white list is defined in validate.c - * Returns true on success + * Returns true on success, false on error with reason set in errmsg. */ static BOOL -ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options) +ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options, WCHAR *errmsg, DWORD capacity) { WCHAR **argv; int argc; - WCHAR buf[256]; BOOL ret = FALSE; int i; const WCHAR *msg1 = L"You have specified a config file location (%s relative to %s)" @@ -382,8 +381,10 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options) if (!argv) { - ReturnLastError(pipe, L"CommandLineToArgvW"); - ReturnError(pipe, ERROR_STARTUP_DATA, L"Cannot validate options", 1, &exit_event); + swprintf(errmsg, capacity, + L"Cannot validate options: CommandLineToArgvW failed with error = 0x%08x", + GetLastError()); + errmsg[capacity-1] = L'\0'; goto out; } @@ -403,10 +404,9 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options) if (!CheckOption(workdir, 2, argv_tmp, &settings)) { - swprintf(buf, _countof(buf), msg1, argv[0], workdir, + swprintf(errmsg, capacity, msg1, argv[0], workdir, settings.ovpn_admin_group); - buf[_countof(buf) - 1] = L'\0'; - ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event); + errmsg[capacity-1] = L'\0'; } goto out; } @@ -422,18 +422,15 @@ ValidateOptions(HANDLE pipe, const WCHAR *workdir, const WCHAR *options) { if (wcscmp(L"--config", argv[i]) == 0 && argc-i > 1) { - swprintf(buf, _countof(buf), msg1, argv[i+1], workdir, + swprintf(errmsg, capacity, msg1, argv[i+1], workdir, settings.ovpn_admin_group); - buf[_countof(buf) - 1] = L'\0'; - ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event); } else { - swprintf(buf, _countof(buf), msg2, argv[i], + swprintf(errmsg, capacity, msg2, argv[i], settings.ovpn_admin_group); - buf[_countof(buf) - 1] = L'\0'; - ReturnError(pipe, ERROR_STARTUP_DATA, buf, 1, &exit_event); } + errmsg[capacity-1] = L'\0'; goto out; } } @@ -1367,6 +1364,7 @@ RunOpenvpn(LPVOID p) WCHAR *cmdline = NULL; size_t cmdline_size; undo_lists_t undo_lists; + WCHAR errmsg[512] = L""; SECURITY_ATTRIBUTES inheritable = { .nLength = sizeof(inheritable), @@ -1459,10 +1457,17 @@ RunOpenvpn(LPVOID p) goto out; } - /* Check user is authorized or options are white-listed */ - if (!IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group) - && !ValidateOptions(pipe, sud.directory, sud.options)) + /* + * Only authorized users are allowed to use any command line options or + * have the config file in locations other than the global config directory. + * + * Check options are white-listed and config is in the global directory + * OR user is authorized to run any config. + */ + if (!ValidateOptions(pipe, sud.directory, sud.options, errmsg, _countof(errmsg)) + && !IsAuthorizedUser(ovpn_user->User.Sid, imp_token, settings.ovpn_admin_group)) { + ReturnError(pipe, ERROR_STARTUP_DATA, errmsg, 1, &exit_event); goto out; }