4703. [bug] BINDInstall.exe was missing some buffer length checks.

[RT #45898]

CHANGES

@@ -1,3 +1,6 @@
+4703.	[bug]		BINDInstall.exe was missing some buffer length checks.
+			[RT #45898]
+
 4702.	[func]		Update function declarations to use
 			dns_masterstyle_flags_t for style flags. [RT #45924]
 

bin/win32/BINDInstall/AccountInfo.cpp

@@ -106,6 +106,7 @@ GetAccountPrivileges(char *name, wchar_t **PrivList, unsigned int *PrivCount,
 	NTSTATUS Status;
 	isc_result_t istatus;
 	int iRetVal = RTN_ERROR;	/* assume error from main */
+	int n;
 
 	/*
 	 * Open the policy on the target machine.
@@ -118,18 +119,29 @@ GetAccountPrivileges(char *name, wchar_t **PrivList, unsigned int *PrivCount,
 	/*
 	 * Let's see if the account exists. Return if not
 	 */
-	wsprintf(AccountName, TEXT("%hS"), name);
+	n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"), name);
-	if (!GetAccountSid(NULL, AccountName, &pSid))
+	if (n < 0 || (size_t)n >= sizeof(AccountName)) {
+		LsaClose(PolicyHandle);
+		return (RTN_ERROR);
+	}
+
+	if (!GetAccountSid(NULL, AccountName, &pSid)) {
+		LsaClose(PolicyHandle);
 		return (RTN_NOACCOUNT);
+	}
+
 	/*
 	 * Find out what groups the account belongs to
 	 */
 	istatus = isc_ntsecurity_getaccountgroups(name, Accounts, maxAccounts,
 						  totalAccounts);
-	if (istatus == ISC_R_NOMEMORY)
+	if (istatus == ISC_R_NOMEMORY) {
+		LsaClose(PolicyHandle);
 		return (RTN_NOMEMORY);
-	else if (istatus != ISC_R_SUCCESS)
+	} else if (istatus != ISC_R_SUCCESS) {
+		LsaClose(PolicyHandle);
 		return (RTN_ERROR);
+	}
 
 	Accounts[*totalAccounts] = name; /* Add the account to the list */
 	(*totalAccounts)++;
@@ -138,10 +150,17 @@ GetAccountPrivileges(char *name, wchar_t **PrivList, unsigned int *PrivCount,
 	 * Loop through each Account to get the list of privileges
 	 */
 	for (i = 0; i < *totalAccounts; i++) {
-		wsprintf(AccountName, TEXT("%hS"), Accounts[i]);
+		n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"),
-		 /* Obtain the SID of the user/group. */
+			      Accounts[i]);
-		if (!GetAccountSid(NULL, AccountName, &pSid))
+		if (n < 0 || (size_t)n >= sizeof(AccountName)) {
+			continue;
+		}
+
+		/* Obtain the SID of the user/group. */
+		if (!GetAccountSid(NULL, AccountName, &pSid)) {
 			continue;	/* Try the next one */
+		}
+
 		/* Get the Privileges allocated to this SID */
 		if ((Status = GetPrivilegesOnAccount(PolicyHandle, pSid,
 			PrivList, PrivCount)) == STATUS_SUCCESS)
@@ -155,6 +174,7 @@ GetAccountPrivileges(char *name, wchar_t **PrivList, unsigned int *PrivCount,
 			continue;	/* Try the next one */
 		}
 	}
+
 	/*
 	 * Close the policy handle.
 	 */
@@ -213,6 +233,7 @@ AddPrivilegeToAcccount(LPTSTR name, LPWSTR PrivilegeName) {
 	PSID pSid;
 	NTSTATUS Status;
 	unsigned long err;
+	int n;
 
 	/*
 	 * Open the policy on the target machine.
@@ -224,9 +245,16 @@ AddPrivilegeToAcccount(LPTSTR name, LPWSTR PrivilegeName) {
 	/*
 	 * Let's see if the account exists. Return if not
 	 */
-	wsprintf(AccountName, TEXT("%hS"), name);
+	n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"), name);
-	if (!GetAccountSid(NULL, AccountName, &pSid))
+	if (n < 0 || (size_t)n >= sizeof(AccountName)) {
+		LsaClose(PolicyHandle);
+		return (RTN_ERROR);
+	}
+
+	if (!GetAccountSid(NULL, AccountName, &pSid)) {
+		LsaClose(PolicyHandle);
 		return (RTN_NOACCOUNT);
+	}
 
 	err = LsaNtStatusToWinError(SetPrivilegeOnAccount(PolicyHandle,
 		pSid, PrivilegeName, TRUE));

bin/win32/BINDInstall/BINDInstallDlg.cpp

@@ -228,7 +228,6 @@ CBINDInstallDlg::CBINDInstallDlg(CWnd* pParent /*=NULL*/)
 	m_startOnInstall = FALSE;
 	m_accountName = _T("");
 	m_accountPassword = _T("");
-	m_accountName = _T("");
 	//}}AFX_DATA_INIT
 	// Note that LoadIcon does not require a subsequent
 	// DestroyIcon in Win32
@@ -470,6 +469,7 @@ void CBINDInstallDlg::OnUninstall() {
 void CBINDInstallDlg::OnInstall() {
 	BOOL success = FALSE;
 	int oldlen;
+	int n;
 
 	if (CheckBINDService())
 		StopBINDService();
@@ -578,12 +578,15 @@ void CBINDInstallDlg::OnInstall() {
 	if (runvcredist) {
 		char Vcredist_x86[MAX_PATH];
 		if (forwin64)
-			sprintf(Vcredist_x86, "\"%s\\Vcredist_x64.exe\"",
+			n = snprintf(Vcredist_x86, sizeof(Vcredist_x86),
-				(LPCTSTR) m_currentDir);
+				     "\"%s\\Vcredist_x64.exe\"",
+				     (LPCTSTR) m_currentDir);
 		else
-			sprintf(Vcredist_x86, "\"%s\\Vcredist_x86.exe\"",
+			n = snprintf(Vcredist_x86, sizeof(Vcredist_x86),
-				(LPCTSTR) m_currentDir);
+				     "\"%s\\Vcredist_x86.exe\"",
-		system(Vcredist_x86);
+				     (LPCTSTR) m_currentDir);
+		if (n >= 0 && (size_t)n < sizeof(Vcredist_x86))
+			system(Vcredist_x86);
 	}
 	try {
 		CreateDirs();
@@ -1158,8 +1161,13 @@ void CBINDInstallDlg::RegisterMessages() {
 	HKEY hKey;
 	DWORD dwData;
 	char pszMsgDLL[MAX_PATH];
+	int n;
 
-	sprintf(pszMsgDLL, "%s\\%s", (LPCTSTR)m_binDir, "bindevt.dll");
+	n = snprintf(pszMsgDLL, sizeof(pszMsgDLL), "%s\\%s",
+		     (LPCTSTR)m_binDir, "bindevt.dll");
+	if (n < 0 || (size_t)n >= sizeof(pszMsgDLL))
+		throw(Exception(IDS_ERR_CREATE_KEY,
+			        "<m_binDir>\\bindevt.dll too long"));
 
 	SetCurrent(IDS_REGISTER_MESSAGES);
 	/* Create a new key for named */
@@ -1282,7 +1290,8 @@ void CBINDInstallDlg::SetCurrent(int id, ...) {
 	memset(buf, 0, 128);
 
 	va_start(va, id);
-	vsprintf(buf, format, va);
+	(void)vsnprintf(buf, sizeof(buf), format, va);
+	buf[sizeof(buf) - 1] = 0;
 	va_end(va);
 
 	m_current.Format("%s", buf);
@@ -1365,7 +1374,8 @@ int CBINDInstallDlg::MsgBox(int id, ...) {
 	memset(buf, 0, BUFSIZ);
 
 	va_start(va, id);
-	vsprintf(buf, format, va);
+	(void)vsnprintf(buf, sizeof(buf), format, va);
+	buf[sizeof(buf) - 1] = 0;
 	va_end(va);
 
 	return (MessageBox(buf));
@@ -1380,7 +1390,8 @@ int CBINDInstallDlg::MsgBox(int id, UINT type, ...) {
 	memset(buf, 0, BUFSIZ);
 
 	va_start(va, type);
-	vsprintf(buf, format, va);
+	(void)vsnprintf(buf, sizeof(buf), format, va);
+	buf[sizeof(buf) - 1] = 0;
 	va_end(va);
 
 	return(MessageBox(buf, NULL, type));
@@ -1404,22 +1415,149 @@ CString CBINDInstallDlg::GetErrMessage(DWORD err) {
 	return(buf);
 }
 
-void CBINDInstallDlg::ProgramGroup(BOOL create) {
+void CBINDInstallDlg::ProgramGroupCreate(TCHAR *commonPath) {
-	TCHAR path[MAX_PATH], commonPath[MAX_PATH], fileloc[MAX_PATH], linkpath[MAX_PATH];
 	HRESULT hres;
 	IShellLink *psl = NULL;
-	LPMALLOC pMalloc = NULL;
 	ITEMIDLIST *itemList = NULL;
+	TCHAR fileloc[MAX_PATH];
+	TCHAR linkpath[MAX_PATH];
+	TCHAR path[MAX_PATH];
+	int n;
 
-	HRESULT hr = SHGetMalloc(&pMalloc);
+	n = snprintf(path, sizeof(path), "%s\\ISC", commonPath);
+	if (n < 0 || (size_t)n >= sizeof(path))
+		return;
+	CreateDirectory(path, NULL);
+
+	n = snprintf(path, sizeof(path), "%s\\ISC\\BIND", commonPath);
+	if (n < 0 || (size_t)n >= sizeof(path))
+		return;
+	CreateDirectory(path, NULL);
+
+	hres = CoInitialize(NULL);
+	if (!SUCCEEDED(hres))
+		return;
+
+	// Get a pointer to the IShellLink interface.
+	hres = CoCreateInstance(CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER,
+				IID_IShellLink, (LPVOID *)&psl);
+	if (!SUCCEEDED(hres)) {
+		goto cleanup;
+	}
+
+	IPersistFile* ppf;
+	n = snprintf(linkpath, sizeof(linkpath), "%s\\BINDCtrl.lnk", path);
+	if (n < 0 || (size_t)n >= sizeof(path)) {
+		goto cleanup;
+	}
+
+	n = snprintf(fileloc, sizeof(fileloc), "%s\\BINDCtrl.exe",
+		     (LPCTSTR) m_binDir);
+	if (n < 0 || (size_t)n >= sizeof(path)) {
+		goto cleanup;
+	}
+
+	psl->SetPath(fileloc);
+	psl->SetDescription("BIND Control Panel");
+
+	hres = psl->QueryInterface(IID_IPersistFile, (void **)&ppf);
+	if (SUCCEEDED(hres)) {
+		WCHAR wsz[MAX_PATH];
+
+		MultiByteToWideChar(CP_ACP, 0, linkpath, -1, wsz, MAX_PATH);
+		hres = ppf->Save(wsz, TRUE);
+		ppf->Release();
+	}
+
+	if (GetFileAttributes("readme.txt") == -1) {
+		goto cleanup;
+	}
+
+	n = snprintf(fileloc, sizeof(fileloc), "%s\\Readme.txt",
+		     (LPCTSTR) m_targetDir);
+	if (n < 0 || (size_t)n >= sizeof(fileloc)) {
+		goto cleanup;
+	}
+
+	n = snprintf(linkpath, sizeof(linkpath), "%s\\Readme.lnk", path);
+	if (n < 0 || (size_t)n >= sizeof(linkpath)) {
+		goto cleanup;
+	}
+
+	psl->SetPath(fileloc);
+	psl->SetDescription("BIND Readme");
+
+	hres = psl->QueryInterface(IID_IPersistFile, (void **)&ppf);
+	if (SUCCEEDED(hres)) {
+		WCHAR wsz[MAX_PATH];
+
+		MultiByteToWideChar(CP_ACP, 0, linkpath, -1, wsz, MAX_PATH);
+		hres = ppf->Save(wsz, TRUE);
+		ppf->Release();
+	}
+
+ cleanup:
+	if (psl)
+		psl->Release();
+	CoUninitialize();
+}
+
+void CBINDInstallDlg::ProgramGroupRemove(TCHAR *commonPath) {
+	HANDLE hFind;
+	TCHAR filename[MAX_PATH];
+	TCHAR path[MAX_PATH];
+	WIN32_FIND_DATA fd;
+	int n;
+
+	n = snprintf(path, sizeof(path), "%s\\ISC\\BIND", commonPath);
+	if (n < 0 || (size_t)n >= sizeof(path))
+		goto remove_isc;
+
+	n = snprintf(filename, sizeof(filename), "%s\\*.*", path);
+	if (n < 0 || (size_t)n >= sizeof(path))
+		goto remove_isc_bind;
+
+	hFind = FindFirstFile(filename, &fd);
+	if (hFind != INVALID_HANDLE_VALUE) {
+		do {
+			if (strcmp(fd.cFileName, ".") == 0 ||
+			    strcmp(fd.cFileName, "..") == 0)
+				continue;
+			n = snprintf(filename, sizeof(filename), "%s\\%s",
+				     path, fd.cFileName);
+			if (n >= 0 && (size_t)n < sizeof(filename)) {
+				DeleteFile(filename);
+			}
+		} while (FindNextFile(hFind, &fd));
+		FindClose(hFind);
+	}
+
+ remove_isc_bind:
+	RemoveDirectory(path);
+
+ remove_isc:
+	n = snprintf(path, sizeof(path), "%s\\ISC", commonPath);
+	if (n >= 0 && (size_t)n < sizeof(path))
+		RemoveDirectory(path);
+}
+
+void CBINDInstallDlg::ProgramGroup(BOOL create) {
+	HRESULT hr;
+	ITEMIDLIST *itemList = NULL;
+	LPMALLOC pMalloc = NULL;
+	TCHAR commonPath[MAX_PATH];
+
+	hr = SHGetMalloc(&pMalloc);
 	if (hr != NOERROR) {
 		MessageBox("Could not get a handle to Shell memory object");
 		return;
 	}
 
-	hr = SHGetSpecialFolderLocation(m_hWnd, CSIDL_COMMON_PROGRAMS, &itemList);
+	hr = SHGetSpecialFolderLocation(m_hWnd, CSIDL_COMMON_PROGRAMS,
+					&itemList);
 	if (hr != NOERROR) {
-		MessageBox("Could not get a handle to the Common Programs folder");
+		MessageBox("Could not get a handle to the Common Programs "
+			   "folder");
 		if (itemList) {
 			pMalloc->Free(itemList);
 		}
@@ -1430,76 +1568,9 @@ void CBINDInstallDlg::ProgramGroup(BOOL create) {
 	pMalloc->Free(itemList);
 
 	if (create) {
-		sprintf(path, "%s\\ISC", commonPath);
+		ProgramGroupCreate(commonPath);
-		CreateDirectory(path, NULL);
+	} else {
-
+		ProgramGroupRemove(commonPath);
-		sprintf(path, "%s\\ISC\\BIND", commonPath);
-		CreateDirectory(path, NULL);
-
-		hres = CoInitialize(NULL);
-
-		if (SUCCEEDED(hres)) {
-			// Get a pointer to the IShellLink interface.
-			hres = CoCreateInstance(CLSID_ShellLink, NULL, CLSCTX_INPROC_SERVER, IID_IShellLink, (LPVOID *)&psl);
-			if (SUCCEEDED(hres))
-			{
-				IPersistFile* ppf;
-				sprintf(linkpath, "%s\\BINDCtrl.lnk", path);
-				sprintf(fileloc, "%s\\BINDCtrl.exe", (LPCTSTR) m_binDir);
-
-				psl->SetPath(fileloc);
-				psl->SetDescription("BIND Control Panel");
-
-				hres = psl->QueryInterface(IID_IPersistFile, (void **)&ppf);
-				if (SUCCEEDED(hres)) {
-					WCHAR wsz[MAX_PATH];
-
-					MultiByteToWideChar(CP_ACP, 0, linkpath, -1, wsz, MAX_PATH);
-					hres = ppf->Save(wsz, TRUE);
-					ppf->Release();
-				}
-
-				if (GetFileAttributes("readme.txt") != -1) {
-					sprintf(fileloc, "%s\\Readme.txt", (LPCTSTR) m_targetDir);
-					sprintf(linkpath, "%s\\Readme.lnk", path);
-
-					psl->SetPath(fileloc);
-					psl->SetDescription("BIND Readme");
-
-					hres = psl->QueryInterface(IID_IPersistFile, (void **)&ppf);
-					if (SUCCEEDED(hres)) {
-						WCHAR wsz[MAX_PATH];
-
-						MultiByteToWideChar(CP_ACP, 0, linkpath, -1, wsz, MAX_PATH);
-						hres = ppf->Save(wsz, TRUE);
-						ppf->Release();
-					}
-					psl->Release();
-				}
-			}
-			CoUninitialize();
-		}
-	}
-	else {
-		TCHAR filename[MAX_PATH];
-		WIN32_FIND_DATA fd;
-
-		sprintf(path, "%s\\ISC\\BIND", commonPath);
-
-		sprintf(filename, "%s\\*.*", path);
-		HANDLE hFind = FindFirstFile(filename, &fd);
-		if (hFind != INVALID_HANDLE_VALUE) {
-			do {
-				if (strcmp(fd.cFileName, ".") && strcmp(fd.cFileName, "..")) {
-					sprintf(filename, "%s\\%s", path, fd.cFileName);
-					DeleteFile(filename);
-				}
-			} while (FindNextFile(hFind, &fd));
-			FindClose(hFind);
-		}
-		RemoveDirectory(path);
-		sprintf(path, "%s\\ISC", commonPath);
-		RemoveDirectory(path);
 	}
 }
 

bin/win32/BINDInstall/BINDInstallDlg.h

@@ -86,6 +86,8 @@ protected:
 	BOOL CheckBINDService();
 	void SetCurrent(int id, ...);
 	void ProgramGroup(BOOL create = TRUE);
+	void ProgramGroupCreate(TCHAR *commonPath);
+	void ProgramGroupRemove(TCHAR *commonPath);
 
 	HICON m_hIcon;
 	CString m_defaultDir;

bin/win32/BINDInstall/VersionInfo.cpp

@@ -7,6 +7,8 @@
 #include "VersionInfo.h"
 #include <winver.h>
 
+#include <config.h>
+
 #ifdef _DEBUG
 #undef THIS_FILE
 static char THIS_FILE[]=__FILE__;
@@ -268,20 +270,25 @@ CString CVersionInfo::QueryStringValue(CString value)
 {
 	UINT blobLen = 0;
 	LPVOID viBlob = NULL;
+	int n;
 
 	if(m_versionInfo)
 	{
 		char queryString[256];
 
-		// This code page value is for American English.  If you change the resources to be other than that
+		// This code page value is for American English.
+		// If you change the resources to be other than that
 		// You probably should change this to match it.
 		DWORD codePage = 0x040904B0;
 
-		sprintf(queryString, "\\StringFileInfo\\%08X\\%s",
+		n = snprintf(queryString, sizeof(queryString),
-			codePage, (LPCTSTR) value);
+			     "\\StringFileInfo\\%08X\\%s",
-
+			     codePage, (LPCTSTR) value);
-		if(VerQueryValue(m_versionInfo, queryString, &viBlob, &blobLen))
+		if (n >= 0 && (size_t)n < sizeof(queryString)) {
-			return((char *)viBlob);
+			if(VerQueryValue(m_versionInfo, queryString,
+					 &viBlob, &blobLen))
+				return((char *)viBlob);
+		}
 	}	
 	return("Not Available");
 }