From 7e40d6274ea219ecc43555ce11cfd08032e709f0 Mon Sep 17 00:00:00 2001 From: Mark Andrews Date: Thu, 7 Sep 2017 12:56:16 +1000 Subject: [PATCH] 4703. [bug] BINDInstall.exe was missing some buffer length checks. [RT #45898] --- CHANGES | 3 + bin/win32/BINDInstall/AccountInfo.cpp | 46 ++++- bin/win32/BINDInstall/BINDInstallDlg.cpp | 243 +++++++++++++++-------- bin/win32/BINDInstall/BINDInstallDlg.h | 2 + bin/win32/BINDInstall/VersionInfo.cpp | 19 +- 5 files changed, 212 insertions(+), 101 deletions(-) diff --git a/CHANGES b/CHANGES index 409c638ce1..3333d2ad41 100644 --- a/CHANGES +++ b/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] diff --git a/bin/win32/BINDInstall/AccountInfo.cpp b/bin/win32/BINDInstall/AccountInfo.cpp index 246dd2c093..678cd7686a 100644 --- a/bin/win32/BINDInstall/AccountInfo.cpp +++ b/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); - if (!GetAccountSid(NULL, AccountName, &pSid)) + n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"), name); + 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]); - /* Obtain the SID of the user/group. */ - if (!GetAccountSid(NULL, AccountName, &pSid)) + n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"), + Accounts[i]); + 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); - if (!GetAccountSid(NULL, AccountName, &pSid)) + n = wnsprintf(AccountName, sizeof(AccountName), TEXT("%hS"), name); + 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)); diff --git a/bin/win32/BINDInstall/BINDInstallDlg.cpp b/bin/win32/BINDInstall/BINDInstallDlg.cpp index 341c427b2e..bb38a535aa 100644 --- a/bin/win32/BINDInstall/BINDInstallDlg.cpp +++ b/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\"", - (LPCTSTR) m_currentDir); + n = snprintf(Vcredist_x86, sizeof(Vcredist_x86), + "\"%s\\Vcredist_x64.exe\"", + (LPCTSTR) m_currentDir); else - sprintf(Vcredist_x86, "\"%s\\Vcredist_x86.exe\"", - (LPCTSTR) m_currentDir); - system(Vcredist_x86); + n = snprintf(Vcredist_x86, sizeof(Vcredist_x86), + "\"%s\\Vcredist_x86.exe\"", + (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, + "\\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) { - TCHAR path[MAX_PATH], commonPath[MAX_PATH], fileloc[MAX_PATH], linkpath[MAX_PATH]; +void CBINDInstallDlg::ProgramGroupCreate(TCHAR *commonPath) { 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); - CreateDirectory(path, NULL); - - 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); + ProgramGroupCreate(commonPath); + } else { + ProgramGroupRemove(commonPath); } } diff --git a/bin/win32/BINDInstall/BINDInstallDlg.h b/bin/win32/BINDInstall/BINDInstallDlg.h index 61d085bbf1..88c020e1a2 100644 --- a/bin/win32/BINDInstall/BINDInstallDlg.h +++ b/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; diff --git a/bin/win32/BINDInstall/VersionInfo.cpp b/bin/win32/BINDInstall/VersionInfo.cpp index c5dc2dc799..fab49bf594 100644 --- a/bin/win32/BINDInstall/VersionInfo.cpp +++ b/bin/win32/BINDInstall/VersionInfo.cpp @@ -7,6 +7,8 @@ #include "VersionInfo.h" #include +#include + #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", - codePage, (LPCTSTR) value); - - if(VerQueryValue(m_versionInfo, queryString, &viBlob, &blobLen)) - return((char *)viBlob); + n = snprintf(queryString, sizeof(queryString), + "\\StringFileInfo\\%08X\\%s", + codePage, (LPCTSTR) value); + if (n >= 0 && (size_t)n < sizeof(queryString)) { + if(VerQueryValue(m_versionInfo, queryString, + &viBlob, &blobLen)) + return((char *)viBlob); + } } return("Not Available"); }