[PATCH 0/1] MR400: shell32: Fix handling of very long URLs
Move from stack to heap allocations where needed to avoid out-of-bounds writes resulting in stack corruptions caused by strcpy/strcat and alike. Tested using `wine shell32_test.exe.so shlexec` (64-bit only). Expected output: `0020:shlexec: 1724 tests executed (43 marked as todo, 0 failures), 0 skipped.` This change is based on https://www.winehq.org/pipermail/wine-devel/2021-May/187791.html and fixes some of the missing bounds checks in this file. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/400
From: Stefan Rentsch <et14rest(a)gmail.com> Move from stack to heap allocations where needed to avoid out-of-bounds writes resulting in stack corruptions caused by strcpy/strcat and alike. Co-authored-by: Haoyang Chen <chenhaoyang(a)uniontech.com> --- dlls/shell32/shlexec.c | 100 +++++++++++++++++++------------ dlls/shell32/tests/shlexec.c | 113 +++++++++++++++++++++++++---------- 2 files changed, 141 insertions(+), 72 deletions(-) diff --git a/dlls/shell32/shlexec.c b/dlls/shell32/shlexec.c index 8c7e3cf0808..7cdc6657302 100644 --- a/dlls/shell32/shlexec.c +++ b/dlls/shell32/shlexec.c @@ -426,14 +426,21 @@ static void *SHELL_BuildEnvW( const WCHAR *path ) static BOOL SHELL_TryAppPathW( LPCWSTR szName, LPWSTR lpResult, WCHAR **env) { HKEY hkApp = 0; - WCHAR buffer[1024]; - LONG len; + const WCHAR app_path[] = L"Software\\Microsoft\\Windows\\CurrentVersion\\App Paths\\"; + WCHAR *buffer; + LONG bufferlen, len; LONG res; BOOL found = FALSE; if (env) *env = NULL; - lstrcpyW(buffer, L"Software\\Microsoft\\Windows\\CurrentVersion\\App Paths\\"); + + bufferlen = (ARRAYSIZE(app_path) + lstrlenW(szName)) * sizeof(WCHAR); + if (!(buffer = heap_alloc(bufferlen))) + return FALSE; + + lstrcpyW(buffer, app_path); lstrcatW(buffer, szName); + res = RegOpenKeyExW(HKEY_LOCAL_MACHINE, buffer, 0, KEY_READ, &hkApp); if (res) goto end; @@ -444,13 +451,14 @@ static BOOL SHELL_TryAppPathW( LPCWSTR szName, LPWSTR lpResult, WCHAR **env) if (env) { - DWORD count = sizeof(buffer); + DWORD count = bufferlen; if (!RegQueryValueExW(hkApp, L"Path", NULL, NULL, (LPBYTE)buffer, &count) && buffer[0]) *env = SHELL_BuildEnvW( buffer ); } end: if (hkApp) RegCloseKey(hkApp); + heap_free(buffer); return found; } @@ -553,7 +561,7 @@ static UINT SHELL_FindExecutable(LPCWSTR lpPath, LPCWSTR lpFile, LPCWSTR lpVerb, WCHAR xlpFile[256]; /* result of SearchPath */ DWORD attribs; /* file attributes */ - TRACE("%s\n", debugstr_w(lpFile)); + TRACE("file=%s resultLen=%i\n", debugstr_w(lpFile), resultLen); if (!lpResult) return ERROR_INVALID_PARAMETER; @@ -673,33 +681,33 @@ static UINT SHELL_FindExecutable(LPCWSTR lpPath, LPCWSTR lpFile, LPCWSTR lpVerb, /* pass the verb string to SHELL_FindExecutableByVerb() */ retval = SHELL_FindExecutableByVerb(lpVerb, key, classname, command, sizeof(command)); - if (retval > 32) - { - DWORD finishedLen; - SHELL_ArgifyW(lpResult, resultLen, command, xlpFile, pidl, args, &finishedLen); - if (finishedLen > resultLen) - ERR("Argify buffer not large enough.. truncated\n"); - - /* Remove double quotation marks and command line arguments */ - if (*lpResult == '"') - { - WCHAR *p = lpResult; - while (*(p + 1) != '"') - { - *p = *(p + 1); - p++; - } - *p = '\0'; - } + if (retval > 32) + { + DWORD finishedLen; + SHELL_ArgifyW(lpResult, resultLen, command, xlpFile, pidl, args, &finishedLen); + if (finishedLen > resultLen) + ERR("Argify buffer not large enough.. truncated\n"); + + /* Remove double quotation marks and command line arguments */ + if (*lpResult == '"') + { + WCHAR *p = lpResult; + while (*p && *(p + 1) != '"') + { + *p = *(p + 1); + p++; + } + *p = '\0'; + } else { /* Truncate on first space */ - WCHAR *p = lpResult; - while (*p != ' ' && *p != '\0') + WCHAR *p = lpResult; + while (*p != ' ' && *p != '\0') p++; - *p='\0'; + *p = '\0'; } - } + } } else /* Check win.ini */ { @@ -951,8 +959,10 @@ static UINT_PTR execute_from_key(LPCWSTR key, LPCWSTR lpFile, WCHAR *env, LPCWST SHELL_ExecuteW32 execfunc, LPSHELLEXECUTEINFOW psei, LPSHELLEXECUTEINFOW psei_out) { - WCHAR cmd[256], param[1024], ddeexec[256]; + WCHAR cmd[256], ddeexec[256]; LONG cmdlen = sizeof(cmd), ddeexeclen = sizeof(ddeexec); + WCHAR *param; + SIZE_T paramlen; UINT_PTR retval = SE_ERR_NOASSOC; DWORD resultLen; LPWSTR tmp; @@ -961,6 +971,12 @@ static UINT_PTR execute_from_key(LPCWSTR key, LPCWSTR lpFile, WCHAR *env, LPCWST debugstr_w(szCommandline), debugstr_w(executable_name)); cmd[0] = '\0'; + + /* Uncertain output size of SHELL_ArgifyW : reserve enough. */ + paramlen = cmdlen + lstrlenW(lpFile) + 500; + if (!(param = heap_alloc(paramlen * sizeof(WCHAR)))) + return SE_ERR_OOM; + param[0] = '\0'; /* Get the application from the registry */ @@ -973,8 +989,8 @@ static UINT_PTR execute_from_key(LPCWSTR key, LPCWSTR lpFile, WCHAR *env, LPCWST if (cmdlen >= ARRAY_SIZE(cmd)) cmdlen = ARRAY_SIZE(cmd) - 1; cmd[cmdlen] = '\0'; - SHELL_ArgifyW(param, ARRAY_SIZE(param), cmd, lpFile, psei->lpIDList, szCommandline, &resultLen); - if (resultLen > ARRAY_SIZE(param)) + SHELL_ArgifyW(param, paramlen, cmd, lpFile, psei->lpIDList, szCommandline, &resultLen); + if (resultLen > paramlen) ERR("Argify buffer not large enough, truncating\n"); } @@ -998,6 +1014,7 @@ static UINT_PTR execute_from_key(LPCWSTR key, LPCWSTR lpFile, WCHAR *env, LPCWST else WARN("Nothing appropriate found for %s\n", debugstr_w(key)); + heap_free(param); return retval; } @@ -1451,6 +1468,8 @@ static UINT_PTR SHELL_quote_and_execute( LPCWSTR wcmd, LPCWSTR wszParameters, LP lstrcatW(wszQuotedCmd, L" "); lstrcatW(wszQuotedCmd, wszParameters); } + assert(lstrlenW(wszQuotedCmd) < len); + TRACE("%s/%s => %s/%s\n", debugstr_w(wszApplicationName), debugstr_w(psei->lpVerb), debugstr_w(wszQuotedCmd), debugstr_w(wszKeyname)); if (*wszKeyname) retval = execute_from_key(wszKeyname, wszApplicationName, env, psei->lpParameters, wcmd, execfunc, psei, psei_out); @@ -1537,12 +1556,11 @@ static BOOL SHELL_execute( LPSHELLEXECUTEINFOW sei, SHELL_ExecuteW32 execfunc ) SEE_MASK_CONNECTNETDRV | SEE_MASK_FLAG_DDEWAIT | SEE_MASK_UNICODE | SEE_MASK_ASYNCOK | SEE_MASK_HMONITOR; - WCHAR parametersBuffer[1024], dirBuffer[MAX_PATH], wcmdBuffer[1024]; + WCHAR parametersBuffer[1024], dirBuffer[MAX_PATH]; WCHAR *wszApplicationName, *wszParameters, *wszDir, *wcmd = NULL; DWORD dwApplicationNameLen = MAX_PATH+2; DWORD parametersLen = ARRAY_SIZE(parametersBuffer); - DWORD wcmdLen = ARRAY_SIZE(wcmdBuffer); - DWORD len; + DWORD len, wcmdLen; SHELLEXECUTEINFOW sei_tmp; /* modifiable copy of SHELLEXECUTEINFO struct */ WCHAR *env; WCHAR wszKeyname[256]; @@ -1569,10 +1587,11 @@ static BOOL SHELL_execute( LPSHELLEXECUTEINFOW sei, SHELL_ExecuteW32 execfunc ) } else if (*sei_tmp.lpFile == '\"' && sei_tmp.lpFile[(len = lstrlenW(sei_tmp.lpFile))-1] == '\"') { + /* trim leading & tailing quotation marks */ if(len-1 >= dwApplicationNameLen) dwApplicationNameLen = len; wszApplicationName = heap_alloc(dwApplicationNameLen*sizeof(WCHAR)); memcpy(wszApplicationName, sei_tmp.lpFile+1, len*sizeof(WCHAR)); - if(len > 2) + if(len >= 2) wszApplicationName[len-2] = '\0'; TRACE("wszApplicationName=%s\n",debugstr_w(wszApplicationName)); } else { @@ -1755,8 +1774,12 @@ static BOOL SHELL_execute( LPSHELLEXECUTEINFOW sei, SHELL_ExecuteW32 execfunc ) /* Else, try to execute the filename */ TRACE("execute:%s,%s,%s\n", debugstr_w(wszApplicationName), debugstr_w(wszParameters), debugstr_w(wszDir)); lpFile = sei_tmp.lpFile; - wcmd = wcmdBuffer; + + /* spare bytes for SHELL_ArgifyW */ + wcmdLen = lstrlenW(wszApplicationName) + 500; + wcmd = heap_alloc(wcmdLen * sizeof(WCHAR)); lstrcpyW(wcmd, wszApplicationName); + if (sei_tmp.lpDirectory) { LPCWSTR searchPath[] = { @@ -1765,6 +1788,7 @@ static BOOL SHELL_execute( LPSHELLEXECUTEINFOW sei, SHELL_ExecuteW32 execfunc ) }; PathFindOnPathW(wcmd, searchPath); } + retval = SHELL_quote_and_execute( wcmd, wszParameters, L"", wszApplicationName, NULL, &sei_tmp, sei, execfunc ); @@ -1774,8 +1798,7 @@ static BOOL SHELL_execute( LPSHELLEXECUTEINFOW sei, SHELL_ExecuteW32 execfunc ) heap_free(wszParameters); if (wszDir != dirBuffer) heap_free(wszDir); - if (wcmd != wcmdBuffer) - heap_free(wcmd); + heap_free(wcmd); return TRUE; } @@ -1837,8 +1860,7 @@ end: heap_free(wszParameters); if (wszDir != dirBuffer) heap_free(wszDir); - if (wcmd != wcmdBuffer) - heap_free(wcmd); + heap_free(wcmd); sei->hInstApp = (HINSTANCE)(retval > 32 ? 33 : retval); diff --git a/dlls/shell32/tests/shlexec.c b/dlls/shell32/tests/shlexec.c index 50c2e84578b..cee51bdfbc6 100644 --- a/dlls/shell32/tests/shlexec.c +++ b/dlls/shell32/tests/shlexec.c @@ -50,6 +50,8 @@ static int myARGC; static char** myARGV; static char tmpdir[MAX_PATH]; static char child_file[MAX_PATH]; +/* Buffer for writes with unpredictable size */ +static char temp_mem[1024*16]; static DLLVERSIONINFO dllver; static BOOL skip_shlexec_tests = FALSE; static BOOL skip_noassoc_tests = FALSE; @@ -63,23 +65,23 @@ static HANDLE dde_ready_event; * ***/ +/* Encode ASCII to Hex */ static const char* encodeA(const char* str) { - static char encoded[2*1024+1]; - char* ptr; - size_t len,i; + char *ptr; + size_t len, i; if (!str) return ""; - len = strlen(str) + 1; - if (len >= sizeof(encoded)/2) + len = strlen(str) + 1; /* intentional tailing "00" (why?) */ + if (len >= sizeof(temp_mem)/2) { fprintf(stderr, "string is too long!\n"); assert(0); } - ptr = encoded; + ptr = temp_mem; for (i = 0; i < len; i++) sprintf(&ptr[i * 2], "%02x", (unsigned char)str[i]); - ptr[2 * len] = '\0'; + ptr[i * 2] = '\0'; return ptr; } @@ -91,47 +93,57 @@ static unsigned decode_char(char c) return c - 'A' + 10; } -static char* decodeA(const char* str) +/* Decode Hex to ASCII */ +static void decodeA_inplace(char* str) { - static char decoded[1024]; - char* ptr; - size_t len,i; + size_t len, i; - len = strlen(str) / 2; - if (!len--) return NULL; - if (len >= sizeof(decoded)) + len = strlen(str); + + if (len & 1) { - fprintf(stderr, "string is too long!\n"); + fprintf(stderr, "hex string format incorrect!\n"); assert(0); } - ptr = decoded; + + len /= 2; for (i = 0; i < len; i++) - ptr[i] = (decode_char(str[2 * i]) << 4) | decode_char(str[2 * i + 1]); - ptr[len] = '\0'; - return ptr; + str[i] = (decode_char(str[2 * i]) << 4) | decode_char(str[2 * i + 1]); + str[i] = '\0'; } static void WINAPIV __WINE_PRINTF_ATTR(2,3) childPrintf(HANDLE h, const char* fmt, ...) { va_list valist; - char buffer[1024]; - DWORD w; + char *buffer; + DWORD w; + int len; va_start(valist, fmt); + + len = vsprintf(NULL, fmt, valist); + buffer = malloc(len + 1); vsprintf(buffer, fmt, valist); + va_end(valist); + WriteFile(h, buffer, strlen(buffer), &w, NULL); + free(buffer); } +/* Reads a specific value from the logger file -> pointer in heap */ static char* getChildString(const char* sect, const char* key) { - char buf[1024]; - char* ret; + char *buf, *ret; + buf = temp_mem; - GetPrivateProfileStringA(sect, key, "-", buf, sizeof(buf), child_file); + GetPrivateProfileStringA(sect, key, "-", buf, sizeof(temp_mem), child_file); if (buf[0] == '\0' || (buf[0] == '-' && buf[1] == '\0')) return NULL; - assert(!(strlen(buf) & 1)); - ret = decodeA(buf); + + decodeA_inplace(buf); + ret = malloc(strlen(buf) + 1); + strcpy(ret, buf); + return ret; } @@ -318,6 +330,7 @@ static void dump_child_(const char* file, int line) str=getChildString("Child", "cmdlineA"); trace_(file, line)("cmdlineA='%s'\n", str); + free(str); c=GetPrivateProfileIntA("Child", "argcA", -1, child_file); trace_(file, line)("argcA=%d\n",c); for (i=0;i<c;i++) @@ -325,12 +338,14 @@ static void dump_child_(const char* file, int line) sprintf(key, "argvA%d", i); str=getChildString("Child", key); trace_(file, line)("%s='%s'\n", key, str); + free(str); } c=GetPrivateProfileIntA("Child", "ShlexecVarLE", -1, child_file); trace_(file, line)("ShlexecVarLE=%d\n", c); str=getChildString("Child", "ShlexecVar"); trace_(file, line)("ShlexecVar='%s'\n", str); + free(str); c=GetPrivateProfileIntA("Child", "Failures", -1, child_file); trace_(file, line)("Failures=%d\n", c); @@ -348,14 +363,19 @@ static char shell_call[2048]; static void WINAPIV __WINE_PRINTF_ATTR(2,3) _okShell(int condition, const char *msg, ...) { va_list valist; - char buffer[2048]; + char buffer[2048 + 100]; + size_t len; + + va_start(valist, msg); strcpy(buffer, shell_call); strcat(buffer, " "); - va_start(valist, msg); - vsprintf(buffer+strlen(buffer), msg, valist); + len = strlen(buffer); + vsnprintf(buffer + len, sizeof(buffer) - len, msg, valist); + va_end(valist); winetest_ok(condition, "%s", buffer); + free(buffer); } #define okShell_(file, line) (winetest_set_location(file, line), 0) ? (void)0 : _okShell #define okShell okShell_(__FILE__, __LINE__) @@ -378,6 +398,7 @@ static void okChildString_(const char* file, int line, const char* key, const ch okShell_(file, line)(lstrcmpiA(result, expected) == 0 || broken(lstrcmpiA(result, bad) == 0), "%s expected '%s', got '%s'\n", key, expected, result); + free(result); } #define okChildString(key, expected) okChildString_(__FILE__, __LINE__, (key), (expected), (expected)) #define okChildStringBroken(key, expected, broken) okChildString_(__FILE__, __LINE__, (key), (expected), (broken)) @@ -446,6 +467,7 @@ static void okChildPath_(const char* file, int line, const char* key, const char } okShell_(file,line)(equal || broken(shortequal) /* XP SP1 */, "%s expected '%s', got '%s'\n", key, expected, result); + free(result); } #define okChildPath(key, expected) okChildPath_(__FILE__, __LINE__, (key), (expected)) @@ -476,14 +498,17 @@ static void okChildIntBroken_(const char* file, int line, const char* key, int e static void strcat_param(char* str, const char* name, const char* param) { + const size_t reasonable_limit = MAX_PATH; if (param) { if (str[strlen(str)-1] == '"') strcat(str, ", "); strcat(str, name); strcat(str, "=\""); - strcat(str, param); + strncat(str, param, reasonable_limit); strcat(str, "\""); + if (strlen(param) > reasonable_limit) + strcat(str, "..."); } } @@ -506,6 +531,8 @@ static INT_PTR shell_execute_(const char* file, int line, LPCSTR verb, LPCSTR fi strcat_param(shell_call, "dir", directory); strcat(shell_call, ")"); strcat(shell_call, assoc_desc); + assert(strlen(shell_call) < sizeof(shell_call)); + if (winetest_debug > 1) trace_(file, line)("Called %s\n", shell_call); @@ -591,6 +618,8 @@ static INT_PTR shell_execute_ex_(const char* file, int line, strcat_param(shell_call, "class", class); strcat(shell_call, ")"); strcat(shell_call, assoc_desc); + assert(strlen(shell_call) < sizeof(shell_call)); + if (winetest_debug > 1) trace_(file, line)("Called %s\n", shell_call); @@ -830,6 +859,7 @@ static void create_test_verb_dde(const char* classname, const char* verb, strcat_param(assoc_desc, "app", application); strcat_param(assoc_desc, "topic", topic); strcat_param(assoc_desc, "ifexec", ifexec); + assert(strlen(assoc_desc) < sizeof(assoc_desc)); sprintf(shell, "%s\\shell", classname); rc=RegOpenKeyExA(HKEY_CLASSES_ROOT, shell, 0, @@ -1546,8 +1576,8 @@ static void test_argify(void) char fileA[MAX_PATH + 18], params[2 * MAX_PATH + 28]; INT_PTR rc; const argify_tests_t* test; - const char *bad; - const char* cmd; + const char *bad, *cmd; + char *p; /* Test with a long parameter */ for (rc = 0; rc < MAX_PATH; rc++) @@ -1585,7 +1615,8 @@ static void test_argify(void) rc = shell_execute_ex(SEE_MASK_DOENVSUBST, test->verb, fileA, test->params, NULL, NULL); okShell(rc > 32, "failed: rc=%Iu\n", rc); - cmd = getChildString("Child", "cmdlineA"); + p = getChildString("Child", "cmdlineA"); + cmd = p; /* Our commands are such that the verb immediately precedes the * part we are interested in. */ @@ -1595,6 +1626,7 @@ static void test_argify(void) todo_wine_if(test->todo) okShell(!strcmp(cmd, test->cmd) || broken(!strcmp(cmd, bad)), "expected '%s', got '%s'\n", cmd, test->cmd); + free(p); test++; } } @@ -1896,6 +1928,21 @@ static void test_urls(void) okChildString("argvA3", "URL"); okChildString("argvA4", "shlproto://foo/bar"); + if (1) { + char long_url[2048]; + memset(long_url, 0, sizeof(long_url)); + strcpy(long_url, "shlproto://foo/bar"); + memset(long_url + strlen(long_url), 'r', sizeof(long_url) - strlen(long_url) - 5); + strcat(long_url, ".exe"); + assert(strlen(long_url) < sizeof(long_url)); + + rc = shell_execute(NULL, long_url, NULL, NULL); + ok(rc > 32, "%s failed: rc=%lu\n", shell_call, rc); + okChildInt("argcA", 5); + okChildString("argvA3", "URL"); + okChildString("argvA4", long_url); + } + /* Check default verb detection */ rc = shell_execute(NULL, "shlpaverb://foo/bar", NULL, NULL); todo_wine ok(rc > 32 || /* XP+IE7 - Win10 */ -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/400
Cannot reproduce, neither do I have an idea how to debug this. Help is greatly appreciated, otherwise feel free to close this PR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/400#note_3555
This merge request was closed by SmallJoker. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/400
participants (2)
-
SmallJoker (＠SmallJoker) -
Stefan Rentsch