From a42b21dc61f1611b07945bdd1bcfd15f7688ce54 Mon Sep 17 00:00:00 2001 From: Rob Mensching Date: Sun, 14 Jul 2024 23:58:39 -0700 Subject: [PATCH] Fix faulty memory access in Util's User custom actions Generally, clean up the handling of getting the domain from a server name by centralizing and simplifying it behind an improved GetDomainFromServerName() based on the buggy GetServerName(). Fixes 8576 --- src/ext/Util/ca/precomp.h | 3 +- src/ext/Util/ca/scaexec.cpp | 100 ++++++++---------------------------- src/ext/Util/ca/scauser.cpp | 48 +++-------------- src/ext/Util/ca/utilca.cpp | 56 ++++++++++++++++++++ src/ext/Util/ca/utilca.h | 8 +++ 5 files changed, 95 insertions(+), 120 deletions(-) create mode 100644 src/ext/Util/ca/utilca.h diff --git a/src/ext/Util/ca/precomp.h b/src/ext/Util/ca/precomp.h index efde32a61..9456c6916 100644 --- a/src/ext/Util/ca/precomp.h +++ b/src/ext/Util/ca/precomp.h @@ -15,7 +15,7 @@ #include #include -#include +#include #include // NetApi32.lib #include #include @@ -50,5 +50,6 @@ #include "scauser.h" #include "scasmb.h" #include "scasmbexec.h" +#include "utilca.h" #include "..\..\caDecor.h" diff --git a/src/ext/Util/ca/scaexec.cpp b/src/ext/Util/ca/scaexec.cpp index 5119bc11e..8579b8bba 100644 --- a/src/ext/Util/ca/scaexec.cpp +++ b/src/ext/Util/ca/scaexec.cpp @@ -613,8 +613,7 @@ static HRESULT RemoveUserInternal( LPWSTR pwz = NULL; LPWSTR pwzGroup = NULL; LPWSTR pwzGroupDomain = NULL; - LPCWSTR wz = NULL; - PDOMAIN_CONTROLLER_INFOW pDomainControllerInfo = NULL; + LPWSTR pwzDomainName = NULL; // // Remove the logon as service privilege. @@ -644,30 +643,10 @@ static HRESULT RemoveUserInternal( // if (!(SCAU_DONT_CREATE_USER & iAttributes)) { - if (wzDomain && *wzDomain) - { - er = ::DsGetDcNameW(NULL, (LPCWSTR)wzDomain, NULL, NULL, NULL, &pDomainControllerInfo); - if (RPC_S_SERVER_UNAVAILABLE == er) - { - // MSDN says, if we get the above error code, try again with the "DS_FORCE_REDISCOVERY" flag - er = ::DsGetDcNameW(NULL, (LPCWSTR)wzDomain, NULL, NULL, DS_FORCE_REDISCOVERY, &pDomainControllerInfo); - } - if (ERROR_SUCCESS == er) - { - if (2 <= wcslen(pDomainControllerInfo->DomainControllerName)) - { - wz = pDomainControllerInfo->DomainControllerName + 2; // Add 2 so that we don't get the \\ prefix. - // Pass the entire string if it is too short - // to have a \\ prefix. - } - } - else - { - wz = wzDomain; - } - } + hr = GetDomainFromServerName(&pwzDomainName, wzDomain, 0); + ExitOnFailure(hr, "Failed to get domain to remove user from server name: %ls", wzDomain); - er = ::NetUserDel(wz, wzName); + er = ::NetUserDel(pwzDomainName, wzName); if (NERR_UserNotFound == er) { er = NERR_Success; @@ -707,52 +686,13 @@ static HRESULT RemoveUserInternal( } LExit: - if (pDomainControllerInfo) - { - ::NetApiBufferFree(static_cast(pDomainControllerInfo)); - } + ReleaseStr(pwzDomainName); + ReleaseStr(pwzGroupDomain); + ReleaseStr(pwzGroup); return hr; } -static void GetServerName(LPWSTR pwzDomain, LPWSTR* ppwzServerName) -{ - DWORD er = ERROR_SUCCESS; - PDOMAIN_CONTROLLER_INFOW pDomainControllerInfo = NULL; - - if (pwzDomain && *pwzDomain) - { - er = ::DsGetDcNameW(NULL, (LPCWSTR)pwzDomain, NULL, NULL, NULL, &pDomainControllerInfo); - if (RPC_S_SERVER_UNAVAILABLE == er) - { - // MSDN says, if we get the above error code, try again with the "DS_FORCE_REDISCOVERY" flag - er = ::DsGetDcNameW(NULL, (LPCWSTR)pwzDomain, NULL, NULL, DS_FORCE_REDISCOVERY, &pDomainControllerInfo); - } - - if (ERROR_SUCCESS == er && pDomainControllerInfo->DomainControllerName) - { - // Skip the \\ prefix if present. - if ('\\' == *pDomainControllerInfo->DomainControllerName && '\\' == *pDomainControllerInfo->DomainControllerName + 1) - { - *ppwzServerName = pDomainControllerInfo->DomainControllerName + 2; - } - else - { - *ppwzServerName = pDomainControllerInfo->DomainControllerName; - } - } - else - { - *ppwzServerName = pwzDomain; - } - } - - if (pDomainControllerInfo) - { - ::NetApiBufferFree((LPVOID)pDomainControllerInfo); - } -} - /******************************************************************** CreateUser - CUSTOM ACTION ENTRY POINT for creating users @@ -776,6 +716,7 @@ extern "C" UINT __stdcall CreateUser( LPWSTR pwzPassword = NULL; LPWSTR pwzGroup = NULL; LPWSTR pwzGroupDomain = NULL; + LPWSTR pwzDomainName = NULL; int iAttributes = 0; BOOL fInitializedCom = FALSE; @@ -786,7 +727,6 @@ extern "C" UINT __stdcall CreateUser( USER_INFO_1 userInfo1; USER_INFO_1* pUserInfo1 = NULL; DWORD dw; - LPWSTR pwzServerName = NULL; hr = WcaInitialize(hInstall, "CreateUser"); ExitOnFailure(hr, "failed to initialize"); @@ -845,9 +785,10 @@ extern "C" UINT __stdcall CreateUser( // // Create the User // - GetServerName(pwzDomain, &pwzServerName); + hr = GetDomainFromServerName(&pwzDomainName, pwzDomain, 0); + ExitOnFailure(hr, "Failed to get domain from server name: %ls", pwzDomain); - er = ::NetUserAdd(pwzServerName, 1, reinterpret_cast(pUserInfo1), &dw); + er = ::NetUserAdd(pwzDomainName, 1, reinterpret_cast(pUserInfo1), &dw); if (NERR_UserExists == er) { if (SCAU_FAIL_IF_EXISTS & iAttributes) @@ -862,7 +803,7 @@ extern "C" UINT __stdcall CreateUser( if (SCAU_UPDATE_IF_EXISTS & iAttributes) { pUserInfo1 = NULL; - er = ::NetUserGetInfo(pwzServerName, pwzName, 1, reinterpret_cast(&pUserInfo1)); + er = ::NetUserGetInfo(pwzDomainName, pwzName, 1, reinterpret_cast(&pUserInfo1)); if (ERROR_SUCCESS == er) { // There is no rollback scheduled if the key is empty. @@ -922,28 +863,28 @@ extern "C" UINT __stdcall CreateUser( if (ERROR_SUCCESS == er) { - hr = SetUserPassword(pwzServerName, pwzName, pwzPassword); + hr = SetUserPassword(pwzDomainName, pwzName, pwzPassword); if (FAILED(hr)) { - WcaLogError(hr, "failed to set user password for user %ls\\%ls, continuing anyway.", pwzServerName, pwzName); + WcaLogError(hr, "failed to set user password for user %ls\\%ls, continuing anyway.", pwzDomainName, pwzName); hr = S_OK; } if (SCAU_REMOVE_COMMENT & iAttributes) { - hr = SetUserComment(pwzServerName, pwzName, L""); + hr = SetUserComment(pwzDomainName, pwzName, L""); if (FAILED(hr)) { - WcaLogError(hr, "failed to clear user comment for user %ls\\%ls, continuing anyway.", pwzServerName, pwzName); + WcaLogError(hr, "failed to clear user comment for user %ls\\%ls, continuing anyway.", pwzDomainName, pwzName); hr = S_OK; } } else if (pwzComment && *pwzComment) { - hr = SetUserComment(pwzServerName, pwzName, pwzComment); + hr = SetUserComment(pwzDomainName, pwzName, pwzComment); if (FAILED(hr)) { - WcaLogError(hr, "failed to set user comment to %ls for user %ls\\%ls, continuing anyway.", pwzComment, pwzServerName, pwzName); + WcaLogError(hr, "failed to set user comment to %ls for user %ls\\%ls, continuing anyway.", pwzComment, pwzDomainName, pwzName); hr = S_OK; } } @@ -952,10 +893,10 @@ extern "C" UINT __stdcall CreateUser( ApplyAttributes(iAttributes, &flags); - hr = SetUserFlags(pwzServerName, pwzName, flags); + hr = SetUserFlags(pwzDomainName, pwzName, flags); if (FAILED(hr)) { - WcaLogError(hr, "failed to set user flags for user %ls\\%ls, continuing anyway.", pwzServerName, pwzName); + WcaLogError(hr, "failed to set user flags for user %ls\\%ls, continuing anyway.", pwzDomainName, pwzName); hr = S_OK; } } @@ -1018,6 +959,7 @@ extern "C" UINT __stdcall CreateUser( ReleaseStr(pwzPassword); ReleaseStr(pwzGroup); ReleaseStr(pwzGroupDomain); + ReleaseStr(pwzDomainName) if (fInitializedCom) { diff --git a/src/ext/Util/ca/scauser.cpp b/src/ext/Util/ca/scauser.cpp index b643a8429..79da155fa 100644 --- a/src/ext/Util/ca/scauser.cpp +++ b/src/ext/Util/ca/scauser.cpp @@ -487,7 +487,7 @@ HRESULT ScaUserExecute( { HRESULT hr = S_OK; DWORD er = 0; - PDOMAIN_CONTROLLER_INFOW pDomainControllerInfo = NULL; + LPWSTR pwzDomainName = NULL; LPWSTR pwzBaseScriptKey = NULL; DWORD cScriptKey = 0; @@ -518,36 +518,11 @@ HRESULT ScaUserExecute( ExitOnFailure(hr, "Failed to add user comment to custom action data: %ls", psu->wzComment); // Check to see if the user already exists since we have to be very careful when adding - // and removing users. Note: MSDN says that it is safe to call these APIs from any - // user, so we should be safe calling it during immediate mode. - er = ::NetApiBufferAllocate(sizeof(USER_INFO_0), reinterpret_cast(&pUserInfo)); - hr = HRESULT_FROM_WIN32(er); - ExitOnFailure(hr, "Failed to allocate memory to check existence of user: %ls", psu->wzName); - - LPCWSTR wzDomain = psu->wzDomain; - if (wzDomain && *wzDomain) - { - er = ::DsGetDcNameW(NULL, wzDomain, NULL, NULL, NULL, &pDomainControllerInfo); - if (RPC_S_SERVER_UNAVAILABLE == er) - { - // MSDN says, if we get the above error code, try again with the "DS_FORCE_REDISCOVERY" flag - er = ::DsGetDcNameW(NULL, wzDomain, NULL, NULL, DS_FORCE_REDISCOVERY, &pDomainControllerInfo); - } - if (ERROR_SUCCESS == er && pDomainControllerInfo->DomainControllerName) - { - // If the \\ prefix on the queried domain was present, skip it. - if ('\\' == *pDomainControllerInfo->DomainControllerName && '\\' == *pDomainControllerInfo->DomainControllerName + 1) - { - wzDomain = pDomainControllerInfo->DomainControllerName + 2; - } - else - { - wzDomain = pDomainControllerInfo->DomainControllerName; - } - } - } + // and removing users. + hr = GetDomainFromServerName(&pwzDomainName, psu->wzDomain, 0); + ExitOnFailure(hr, "Failed to get domain from server name: %ls", psu->wzDomain); - er = ::NetUserGetInfo(wzDomain, psu->wzName, 0, reinterpret_cast(pUserInfo)); + er = ::NetUserGetInfo(pwzDomainName, psu->wzName, 0, reinterpret_cast(&pUserInfo)); if (NERR_Success == er) { ueUserExists = USER_EXISTS_YES; @@ -560,7 +535,7 @@ HRESULT ScaUserExecute( { ueUserExists = USER_EXISTS_INDETERMINATE; hr = HRESULT_FROM_WIN32(er); - WcaLog(LOGMSG_VERBOSE, "Failed to check existence of domain: %ls, user: %ls (error code 0x%x) - continuing", wzDomain, psu->wzName, hr); + WcaLog(LOGMSG_VERBOSE, "Failed to check existence of domain: %ls, user: %ls (error code 0x%x) - continuing", pwzDomainName, psu->wzName, hr); hr = S_OK; er = ERROR_SUCCESS; } @@ -685,11 +660,6 @@ HRESULT ScaUserExecute( ::NetApiBufferFree(static_cast(pUserInfo)); pUserInfo = NULL; } - if (pDomainControllerInfo) - { - ::NetApiBufferFree(static_cast(pDomainControllerInfo)); - pDomainControllerInfo = NULL; - } } LExit: @@ -697,14 +667,12 @@ HRESULT ScaUserExecute( ReleaseStr(pwzScriptKey); ReleaseStr(pwzActionData); ReleaseStr(pwzRollbackData); + ReleaseStr(pwzDomainName); + if (pUserInfo) { ::NetApiBufferFree(static_cast(pUserInfo)); } - if (pDomainControllerInfo) - { - ::NetApiBufferFree(static_cast(pDomainControllerInfo)); - } return hr; } diff --git a/src/ext/Util/ca/utilca.cpp b/src/ext/Util/ca/utilca.cpp index 37664a1c8..d41f00c29 100644 --- a/src/ext/Util/ca/utilca.cpp +++ b/src/ext/Util/ca/utilca.cpp @@ -1,3 +1,59 @@ // Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information. #include "precomp.h" + +HRESULT GetDomainFromServerName( + __deref_out_z LPWSTR* ppwzDomainName, + __in_z LPCWSTR wzServerName, + __in DWORD dwFlags + ) +{ + HRESULT hr = S_OK; + DWORD er = ERROR_SUCCESS; + PDOMAIN_CONTROLLER_INFOW pDomainControllerInfo = NULL; + LPCWSTR wz = wzServerName ? wzServerName : L""; // initialize the domain to the provided server name (or empty string). + + // If the server name was not empty, try to get the domain name out of it. + if (*wz) + { + er = ::DsGetDcNameW(NULL, wz, NULL, NULL, dwFlags, &pDomainControllerInfo); + if (RPC_S_SERVER_UNAVAILABLE == er) + { + // MSDN says, if we get the above error code, try again with the "DS_FORCE_REDISCOVERY" flag. + er = ::DsGetDcNameW(NULL, wz, NULL, NULL, dwFlags | DS_FORCE_REDISCOVERY, &pDomainControllerInfo); + } + ExitOnWin32Error(er, hr, "Could not get domain name from server name: %ls", wz); + + if (pDomainControllerInfo->DomainControllerName) + { + // Skip the \\ prefix if present. + if ('\\' == *pDomainControllerInfo->DomainControllerName && '\\' == *(pDomainControllerInfo->DomainControllerName + 1)) + { + wz = pDomainControllerInfo->DomainControllerName + 2; + } + else + { + wz = pDomainControllerInfo->DomainControllerName; + } + } + } + +LExit: + // Note: we overwrite the error code here as failure to contact domain controller above is not a fatal error. + if (wz && *wz) + { + hr = StrAllocString(ppwzDomainName, wz, 0); + } + else // return NULL the server name ended up empty. + { + ReleaseNullStr(*ppwzDomainName); + hr = S_OK; + } + + if (pDomainControllerInfo) + { + ::NetApiBufferFree((LPVOID)pDomainControllerInfo); + } + + return hr; +} diff --git a/src/ext/Util/ca/utilca.h b/src/ext/Util/ca/utilca.h new file mode 100644 index 000000000..97c545619 --- /dev/null +++ b/src/ext/Util/ca/utilca.h @@ -0,0 +1,8 @@ +#pragma once +// Copyright (c) .NET Foundation and contributors. All rights reserved. Licensed under the Microsoft Reciprocal License. See LICENSE.TXT file in the project root for full license information. + +HRESULT GetDomainFromServerName( + __deref_out_z LPWSTR* ppwzDomainName, + __in_z LPCWSTR wzServerName, + __in DWORD dwFlags + );