From patchwork Fri Sep 6 11:29:08 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gert Doering X-Patchwork-Id: 3801 Return-Path: Delivered-To: patchwork@openvpn.net Received: by 2002:a05:7000:6bd4:b0:5b9:581e:f939 with SMTP id c20csp79703max; Fri, 6 Sep 2024 04:29:50 -0700 (PDT) X-Forwarded-Encrypted: i=2; AJvYcCX6JAxlYhZBHWTCBsHUzx0jY1GwaIJYC+lOtQrjcmK0uDhgrdFTBlhgenNULOsa6qmQ/ydp9e2dSRg=@openvpn.net X-Google-Smtp-Source: AGHT+IFX9r/nbPZWjFwdNmWOB8HpnWmpjaulfwsydtWWbbVL9xuebaDgfI31QGok7k/9wf8bRf1T X-Received: by 2002:a05:6a20:d808:b0:1ce:cce2:f16d with SMTP id adf61e73a8af0-1cf1d1ec1f4mr1830334637.31.1725622190315; Fri, 06 Sep 2024 04:29:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1725622190; cv=none; d=google.com; s=arc-20240605; b=N/fAKQfwh1ijQjP8gJAEXkmpopiGZeXDPZRC//hzgjQmxx5LgYkRLHBX+DqAVtWZHh XPlBGKTTLorYlV9osFR/C8sghGzco/zIXxf0bvwQsD9w7pwfFw1HOccYIlkLS8FnfHlT o5LWckBaQeOBBAwnWGgwmCeNaj2hZSm7goaXJgRTNc7lz1qDb1tjgcnC9GS/BMWpKbyt OWNzjA4vvnmvToY5vDuHV9gxTw+Mk/UcerBmL2uPvR2Rcvai/ibz79kHL2ZS1wOcF+kN KihHluwixnds1QMwT5Yd+xAoQCJN4AJbxAXogztx7Fpc2K6+B2p8isBz/EkHZL2WX9vV zIxg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20240605; h=errors-to:content-transfer-encoding:list-subscribe:list-help :list-post:list-archive:list-unsubscribe:list-id:precedence:subject :mime-version:references:in-reply-to:message-id:date:to:from :dkim-signature:dkim-signature; bh=jHLOAsfqcNMDHrFnnKCLKXQBQuEtbNIEP1v/PJW3CSU=; fh=4NbAC/LsuMLI0S0hprUlLSLCiHwg6SCAifhH718Jh0Q=; b=HOJsx5t47QxgQILKNVgHatGi4njS8Zh8UkJ79yuwxQHpnvABAF/ejnK5Ig9fc38b9d zFtLjXfyi1c+68h7VR2PyBC2po48L8IsUKKSrCDSAsyea3DvHC8ytBpI7pIWNbUcCEgP 9sdsKyPy0abmYGGjbelQMmDcbJU/7+uMZUhWRoXjhXphfA489uobVAAGWKgl+wWpHAhs jQSd+HlMcgErjyCPVfUr6jjZ35MvsBwMdICULp0XsUXs0RzlTQ3PaEMb2E5DSjjBUcAf 4d7U530+CqTta+UfHONJsE/amMNvvaxRGFOjC+MwhH5yUb/Sb/F3g9uwVBp7+tFNy/Op jvbA==; 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=QuTKjE8R; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=mlBSdsvN; 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=muc.de Received: from lists.sourceforge.net (lists.sourceforge.net. [216.105.38.7]) by mx.google.com with ESMTPS id 98e67ed59e1d1-2dadc11c25bsi1291952a91.185.2024.09.06.04.29.50 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 06 Sep 2024 04:29:50 -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=QuTKjE8R; dkim=neutral (body hash did not verify) header.i=@sf.net header.s=x header.b=mlBSdsvN; 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=muc.de 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 1smX9W-0002tg-UL; Fri, 06 Sep 2024 11:29:22 +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 1smX9T-0002tG-7S for openvpn-devel@lists.sourceforge.net; Fri, 06 Sep 2024 11:29:19 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=Content-Transfer-Encoding:MIME-Version:References: In-Reply-To:Message-ID:Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=OqDgMYE9jCpkZodEB7DAel9iTEhqavaBfvyLceJ8c3g=; b=QuTKjE8R4IEvQ/ILHgp00jCSYz YKLzvIZHWi2c76mF0MBo+b2hnwLpZZPCcv0eH9MeH93kl5VphJcQ6x3Ap94foIwyudL6eaYbKI3FV 30uBv4zVgIFro2jMTySg+/81JjKbhrJBCqHVxhWYFeNkXH7ULY/+neNBHuz2J9kc5ONk=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=Content-Transfer-Encoding:MIME-Version:References:In-Reply-To:Message-ID: Date:Subject:To:From:Sender:Reply-To:Cc:Content-Type:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=OqDgMYE9jCpkZodEB7DAel9iTEhqavaBfvyLceJ8c3g=; b=mlBSdsvN9GmCDqD1hHdBgCDP00 S1Pu1hTlMZlbJ0DOqtz2KAj482yZqYp1HFGGknxP2xoqZIGlqgsOiMU4nq9GfUiaLFEvaxaFumyK4 /fXUxMRjQlJh1ZT+VxtivU0vM9XxnayFBb35P0JgNsRstxh1Jdnzm4BHJik/jAPKFBJM=; Received: from dhcp-174.greenie.muc.de ([193.149.48.174] helo=blue.greenie.muc.de) by sfi-mx-2.v28.lw.sourceforge.com with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.95) id 1smX9R-0006vq-3F for openvpn-devel@lists.sourceforge.net; Fri, 06 Sep 2024 11:29:19 +0000 Received: from blue.greenie.muc.de (localhost [127.0.0.1]) by blue.greenie.muc.de (8.17.1.9/8.17.1.9) with ESMTP id 486BT9xn001021 for ; Fri, 6 Sep 2024 13:29:09 +0200 Received: (from gert@localhost) by blue.greenie.muc.de (8.17.1.9/8.17.1.9/Submit) id 486BT9xs001020 for openvpn-devel@lists.sourceforge.net; Fri, 6 Sep 2024 13:29:09 +0200 From: Gert Doering To: openvpn-devel@lists.sourceforge.net Date: Fri, 6 Sep 2024 13:29:08 +0200 Message-ID: <20240906112908.1009-1-gert@greenie.muc.de> X-Mailer: git-send-email 2.44.2 In-Reply-To: References: MIME-Version: 1.0 X-Spam-Score: -0.0 (/) X-Spam-Report: Spam detection software, running on the system "util-spamd-1.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: From: Selva Nair Keep the memory segment containing username and password in "struct user_pass" encrypted. Works only on Windows. Username and auth-token cached by the server are not covered here. Content analysis details: (-0.0 points, 6.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_HELO_PASS SPF: HELO matches SPF record -0.0 SPF_PASS SPF: sender matches SPF record X-Headers-End: 1smX9R-0006vq-3F Subject: [Openvpn-devel] [PATCH v3] Protect cached username, password and token on client 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: , Errors-To: openvpn-devel-bounces@lists.sourceforge.net X-getmail-retrieved-from-mailbox: Inbox X-GMAIL-THRID: =?utf-8?q?1809446013792750220?= X-GMAIL-MSGID: =?utf-8?q?1809446013792750220?= From: Selva Nair Keep the memory segment containing username and password in "struct user_pass" encrypted. Works only on Windows. Username and auth-token cached by the server are not covered here. v2: Encrypt username and password separately as it looks more robust. We continue to depend on the username and password buffer sizes to be a multiple of CRYPTPROTECTMEMORY_BLOCK_SIZE = 16, which is the case now. An error is logged if this is not the case. v3: move up ASSERT in auth_token.c Change-Id: I42e17e09a02f01aedadc2b03f9527967f6e1e8ff Signed-off-by: Selva Nair Acked-by: Frank Lichtenheld --- This change was reviewed on Gerrit and approved by at least one developer. I request to merge it to master. Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/728 This mail reflects revision 3 of this Change. Acked-by according to Gerrit (reflected above): Frank Lichtenheld diff --git a/src/openvpn/auth_token.c b/src/openvpn/auth_token.c index 6787ea7..5de65cb 100644 --- a/src/openvpn/auth_token.c +++ b/src/openvpn/auth_token.c @@ -301,6 +301,7 @@ * Base64 is <= input and input is < USER_PASS_LEN, so using USER_PASS_LEN * is safe here but a bit overkill */ + ASSERT(up && !up->protected); uint8_t b64decoded[USER_PASS_LEN]; int decoded_len = openvpn_base64_decode(up->password + strlen(SESSION_ID_PREFIX), b64decoded, USER_PASS_LEN); diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c index 598fbae..ef4ab69 100644 --- a/src/openvpn/misc.c +++ b/src/openvpn/misc.c @@ -223,6 +223,7 @@ bool password_from_stdin = false; bool response_from_stdin = true; + unprotect_user_pass(up); if (flags & GET_USER_PASS_PREVIOUS_CREDS_FAILED) { msg(M_WARN, "Note: previous '%s' credentials failed", prefix); @@ -479,14 +480,18 @@ secure_memzero(up, sizeof(*up)); up->nocache = nocache; } - /* - * don't show warning if the pass has been replaced by a token: this is an - * artificial "auth-nocache" - */ - else if (!warn_shown) + else { - msg(M_WARN, "WARNING: this configuration may cache passwords in memory -- use the auth-nocache option to prevent this"); - warn_shown = true; + protect_user_pass(up); + /* + * don't show warning if the pass has been replaced by a token: this is an + * artificial "auth-nocache" + */ + if (!warn_shown) + { + msg(M_WARN, "WARNING: this configuration may cache passwords in memory -- use the auth-nocache option to prevent this"); + warn_shown = true; + } } } @@ -495,6 +500,7 @@ { if (strlen(token)) { + unprotect_user_pass(tk); strncpynt(tk->password, token, USER_PASS_LEN); tk->token_defined = true; @@ -505,6 +511,7 @@ { tk->defined = true; } + protect_user_pass(tk); } } @@ -513,6 +520,7 @@ { if (strlen(username)) { + unprotect_user_pass(tk); /* Clear the username before decoding to ensure no old material is left * and also allow decoding to not use all space to ensure the last byte is * always 0 */ @@ -523,6 +531,7 @@ { msg(D_PUSH, "Error decoding auth-token-username"); } + protect_user_pass(tk); } } @@ -779,3 +788,43 @@ return combined_path; } + +void +protect_user_pass(struct user_pass *up) +{ + if (up->protected) + { + return; + } +#ifdef _WIN32 + if (protect_buffer_win32(up->username, sizeof(up->username)) + && protect_buffer_win32(up->password, sizeof(up->password))) + { + up->protected = true; + } + else + { + purge_user_pass(up, true); + } +#endif +} + +void +unprotect_user_pass(struct user_pass *up) +{ + if (!up->protected) + { + return; + } +#ifdef _WIN32 + if (unprotect_buffer_win32(up->username, sizeof(up->username)) + && unprotect_buffer_win32(up->password, sizeof(up->password))) + { + up->protected = false; + } + else + { + purge_user_pass(up, true); + } +#endif +} diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h index 963f3e6..a967ec8 100644 --- a/src/openvpn/misc.h +++ b/src/openvpn/misc.h @@ -60,6 +60,7 @@ * use this second bool to track if the token (password) is defined */ bool token_defined; bool nocache; + bool protected; /* max length of username/password */ #ifdef ENABLE_PKCS11 @@ -207,6 +208,19 @@ struct buffer prepend_dir(const char *dir, const char *path, struct gc_arena *gc); +/** + * Encrypt username and password buffers in user_pass + */ +void +protect_user_pass(struct user_pass *up); + +/** + * Decrypt username and password buffers in user_pass + */ +void +unprotect_user_pass(struct user_pass *up); + + #define _STRINGIFY(S) #S /* *INDENT-OFF* - uncrustify need to ignore this macro */ #define MAC_FMT _STRINGIFY(%02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx) diff --git a/src/openvpn/proxy.c b/src/openvpn/proxy.c index 5de0da4..a379c7a 100644 --- a/src/openvpn/proxy.c +++ b/src/openvpn/proxy.c @@ -289,13 +289,14 @@ UP_TYPE_PROXY, flags); static_proxy_user_pass.nocache = p->options.nocache; + protect_user_pass(&static_proxy_user_pass); } /* * Using cached credentials */ p->queried_creds = true; - p->up = static_proxy_user_pass; + p->up = static_proxy_user_pass; /* this is a copy of protected memory */ } #if 0 @@ -666,6 +667,7 @@ { clear_user_pass_http(); } + unprotect_user_pass(&p->up); } /* are we being called again after getting the digest server nonce in the previous transaction? */ diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 14c38cf..c2c9c29 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -243,6 +243,7 @@ void pem_password_setup(const char *auth_file) { + unprotect_user_pass(&passbuf); if (!strlen(passbuf.password)) { get_user_pass(&passbuf, auth_file, UP_TYPE_PRIVATE_KEY, GET_USER_PASS_MANAGEMENT|GET_USER_PASS_PASSWORD_ONLY); @@ -256,6 +257,7 @@ { /* prompt for password even if --askpass wasn't specified */ pem_password_setup(NULL); + ASSERT(!passbuf.protected); strncpynt(buf, passbuf.password, size); purge_user_pass(&passbuf, false); @@ -295,6 +297,7 @@ if (!auth_user_pass.defined && !auth_token.defined) { + unprotect_user_pass(&auth_user_pass); #ifdef ENABLE_MANAGEMENT if (auth_challenge) /* dynamic challenge/response */ { @@ -2094,6 +2097,7 @@ { up = &auth_token; } + unprotect_user_pass(up); if (!write_string(buf, up->username, -1)) { @@ -2106,8 +2110,11 @@ /* save username for auth-token which may get pushed later */ if (session->opt->pull && up != &auth_token) { + unprotect_user_pass(&auth_token); strncpynt(auth_token.username, up->username, USER_PASS_LEN); + protect_user_pass(&auth_token); } + protect_user_pass(up); /* respect auth-nocache */ purge_user_pass(&auth_user_pass, false); } diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 0b0e2c3..74b2775 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1594,6 +1594,8 @@ { struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ + ASSERT(up && !up->protected); + #ifdef ENABLE_MANAGEMENT int man_def_auth = KMDA_UNDEF; diff --git a/src/openvpn/win32.c b/src/openvpn/win32.c index 72496fa..86556a8 100644 --- a/src/openvpn/win32.c +++ b/src/openvpn/win32.c @@ -1658,4 +1658,40 @@ return wcsnicmp(system_dir, plugin_path, wcslen(system_dir)) == 0; } +bool +protect_buffer_win32(char *buf, size_t len) +{ + bool ret; + if (len % CRYPTPROTECTMEMORY_BLOCK_SIZE) + { + msg(M_NONFATAL, "Error: Unable to encrypt memory: buffer size not a multiple of %d", + CRYPTPROTECTMEMORY_BLOCK_SIZE); + return false; + } + ret = CryptProtectMemory(buf, len, CRYPTPROTECTMEMORY_SAME_PROCESS); + if (!ret) + { + msg(M_NONFATAL | M_ERRNO, "Failed to encrypt memory."); + } + return ret; +} + +bool +unprotect_buffer_win32(char *buf, size_t len) +{ + bool ret; + if (len % CRYPTPROTECTMEMORY_BLOCK_SIZE) + { + msg(M_NONFATAL, "Error: Unable to decrypt memory: buffer size not a multiple of %d", + CRYPTPROTECTMEMORY_BLOCK_SIZE); + return false; + } + ret = CryptUnprotectMemory(buf, len, CRYPTPROTECTMEMORY_SAME_PROCESS); + if (!ret) + { + msg(M_FATAL | M_ERRNO, "Failed to decrypt memory."); + } + return ret; +} + #endif /* ifdef _WIN32 */ diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h index ffad6d9..081beeb 100644 --- a/src/openvpn/win32.h +++ b/src/openvpn/win32.h @@ -351,5 +351,27 @@ bool plugin_in_trusted_dir(const WCHAR *plugin_path); +/** + * Encrypt a region of memory using CryptProtectMemory() + * with access restricted to the current process. + * + * - buf pointer to the memory + * - len number of bytes to encrypt -- must be a multiple of + * CRYPTPROTECTMEMORY_BLOCK_SIZE = 16 + */ +bool +protect_buffer_win32(char *buf, size_t len); + +/** + * Decrypt a previously encrypted region of memory using CryptUnProtectMemory() + * with access restricted to the current process. + * + * - buf pointer to the memory + * - len number of bytes to encrypt -- must be a multiple of + * CRYPTPROTECTMEMORY_BLOCK_SIZE = 16 + */ +bool +unprotect_buffer_win32(char *buf, size_t len); + #endif /* ifndef OPENVPN_WIN32_H */ #endif /* ifdef _WIN32 */ diff --git a/tests/unit_tests/openvpn/test_user_pass.c b/tests/unit_tests/openvpn/test_user_pass.c index de60291..4dc4b83 100644 --- a/tests/unit_tests/openvpn/test_user_pass.c +++ b/tests/unit_tests/openvpn/test_user_pass.c @@ -83,6 +83,18 @@ return 0; } +bool +protect_buffer_win32(char *buf, size_t len) +{ + return true; +} + +bool +unprotect_buffer_win32(char *buf, size_t len) +{ + return true; +} + /* tooling */ static void reset_user_pass(struct user_pass *up)