I thought fixing bug #51599 would be quick and simple... Fixing was quick, but I shouldn't have tried to run the non regression tests... which opened a can of worms <g>
patch #2 fixes some error cases on input handling
the can of worms: it happens that I have a single symlinks in my (not pristine) prefix (well a very very simple one: Z: -> /) cmd.exe (and attrib.exe) **NEVER** test for buffer overflow when recursing inside the directory tree structure (I haven't checked the other utilities yet)
I tested native behavior: depth-recursion stops around MAX_PATH length... while breadth-recursion continues; and there's no detection about DAG or entering twice the same directory
so patches #3 and #4 are about not recursing when buffer would overflow in attrib.exe and cmd.exe
Notes: - not all the places (in cmd) have been fixed by those patches for buffer overflow - there are some places that take care of allocating larger buffers than MAX_PATH, hence preventing the recursion to stop as native does - with those patches, I still can't successfully run the cmd regression test with the symlink in place (maybe I need to be more patient than one hour :-( - regression tests pass on a pristine prefix though
A+
---
Eric Pouech (4): programs/cmd: handle white space only lines within ( ) block programs/cmd: handle read errors in WCMD_ask_confirm programs/attrib: don't overflow internal path buffers programs/cmd: don't overflow internal path buffers
programs/attrib/attrib.c | 10 ++++++---- programs/cmd/builtins.c | 25 ++++++++++++++++++------- programs/cmd/directory.c | 1 + programs/cmd/wcmdmain.c | 8 ++++++-- 4 files changed, 31 insertions(+), 13 deletions(-)
Wine-bugs: https://bugs.winehq.org/show_bug.cgi?id=51599 Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/cmd/wcmdmain.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 675d7c5f464..45aec76c89c 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -1828,6 +1828,7 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE BOOL lastWasCaret = FALSE; BOOL ignoreBracket = FALSE; /* Some expressions after if (set) require */ /* handling brackets as a normal character */ + BOOL atEOF = FALSE; int lineCurDepth; /* Bracket depth when line was read in */ BOOL resetAtEndOfLine = FALSE; /* Do we need to reset curdepth at EOL */
@@ -2278,8 +2279,8 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE
/* If we have reached the end of the string, see if bracketing or final caret is outstanding */ - if (*curPos == 0x00 && (curDepth > 0 || lastWasCaret) && - readFrom != INVALID_HANDLE_VALUE) { + while (*curPos == 0x00 && (curDepth > 0 || lastWasCaret) && + readFrom != INVALID_HANDLE_VALUE && !atEOF) { WCHAR *extraData;
WINE_TRACE("Need to read more data as outstanding brackets or carets\n"); @@ -2294,7 +2295,10 @@ WCHAR *WCMD_ReadAndParseLine(const WCHAR *optionalcmd, CMD_LIST **output, HANDLE WINE_TRACE("Read more input\n"); if (!context) WCMD_output_asis( WCMD_LoadMessage(WCMD_MOREPROMPT)); if (!WCMD_fgets(extraData, MAXSTRING, readFrom)) + { + atEOF = TRUE; break; + }
/* Edge case for carets - a completely blank line (i.e. was just CRLF) is oddly added as an LF but then more data is received (but
Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/cmd/builtins.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index d14e69e072a..91e4453a4e7 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -193,7 +193,9 @@ static BOOL WCMD_ask_confirm (const WCHAR *message, BOOL showSureText, if (showSureText) WCMD_output_asis (confirm); WCMD_output_asis (options); - WCMD_ReadFile(GetStdHandle(STD_INPUT_HANDLE), answer, ARRAY_SIZE(answer), &count); + if (!WCMD_ReadFile(GetStdHandle(STD_INPUT_HANDLE), answer, ARRAY_SIZE(answer), &count)) + return FALSE; + if (!count) continue; answer[0] = towupper(answer[0]); if (answer[0] == Ybuffer[0]) return TRUE;
this happens in recursive mode with symlinks
Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/attrib/attrib.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/programs/attrib/attrib.c b/programs/attrib/attrib.c index f8257461fe2..77789fb760e 100644 --- a/programs/attrib/attrib.c +++ b/programs/attrib/attrib.c @@ -149,6 +149,7 @@ static BOOL ATTRIB_processdirectory(const WCHAR *rootdir, const WCHAR *filespec,
if (recurse) {
+ if (wcslen(rootdir) + 1 + 1 > ARRAY_SIZE(buffer)) return FALSE; /* Build spec to search for */ lstrcpyW(buffer, rootdir); lstrcatW(buffer, L"*"); @@ -163,6 +164,7 @@ static BOOL ATTRIB_processdirectory(const WCHAR *rootdir, const WCHAR *filespec, !lstrcmpW(fd.cFileName, L".") || !lstrcmpW(fd.cFileName, L"..")) continue;
+ if (wcslen(rootdir) + wcslen(fd.cFileName) + 1 + 1 > ARRAY_SIZE(buffer)) continue; /* Build new root dir to go searching in */ lstrcpyW(buffer, rootdir); lstrcatW(buffer, fd.cFileName); @@ -175,6 +177,7 @@ static BOOL ATTRIB_processdirectory(const WCHAR *rootdir, const WCHAR *filespec, FindClose (hff); }
+ if (wcslen(rootdir) + wcslen(filespec) + 1 > ARRAY_SIZE(buffer)) return FALSE; /* Build spec to search for */ lstrcpyW(buffer, rootdir); lstrcatW(buffer, filespec); @@ -193,13 +196,14 @@ static BOOL ATTRIB_processdirectory(const WCHAR *rootdir, const WCHAR *filespec, if (!includedirs && (fd.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY)) continue;
+ if (wcslen(rootdir) + wcslen(fd.cFileName) + 1 > ARRAY_SIZE(buffer)) continue; + lstrcpyW(buffer, rootdir); + lstrcatW(buffer, fd.cFileName); if (attrib_set || attrib_clear) { fd.dwFileAttributes &= ~attrib_clear; fd.dwFileAttributes |= attrib_set; if (!fd.dwFileAttributes) fd.dwFileAttributes |= FILE_ATTRIBUTE_NORMAL; - lstrcpyW(buffer, rootdir); - lstrcatW(buffer, fd.cFileName); SetFileAttributesW(buffer, fd.dwFileAttributes); found = TRUE; } else { @@ -221,8 +225,6 @@ static BOOL ATTRIB_processdirectory(const WCHAR *rootdir, const WCHAR *filespec, if (fd.dwFileAttributes & FILE_ATTRIBUTE_COMPRESSED) { flags[7] = 'C'; } - lstrcpyW(buffer, rootdir); - lstrcatW(buffer, fd.cFileName); ATTRIB_wprintf(L"%1 %2\n", flags, buffer); for (count = 0; count < (ARRAY_SIZE(flags) - 1); count++) flags[count] = ' '; found = TRUE;
this can happen with symbolic links
Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/cmd/builtins.c | 21 +++++++++++++++------ programs/cmd/directory.c | 1 + 2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index 91e4453a4e7..6966a4229b3 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -1256,12 +1256,14 @@ static BOOL WCMD_delete_one (const WCHAR *thisArg) { { WCHAR modifiedParm[MAX_PATH];
- lstrcpyW(modifiedParm, argCopy); - lstrcatW(modifiedParm, L"\*"); - FindClose(hff); - found = TRUE; - WCMD_delete_one(modifiedParm); - + if (wcslen(argCopy) + 2 + 1 <= ARRAY_SIZE(modifiedParm)) + { + lstrcpyW(modifiedParm, argCopy); + lstrcatW(modifiedParm, L"\*"); + FindClose(hff); + found = TRUE; + WCMD_delete_one(modifiedParm); + } } else if (handleParm) {
/* Build the filename to delete as <supplied directory><findfirst filename> */ @@ -1269,6 +1271,7 @@ static BOOL WCMD_delete_one (const WCHAR *thisArg) { do { p = wcsrchr (fpath, '\'); if (p != NULL) { + if ((p + 1 - fpath) + wcslen(fd.cFileName) + 1 > ARRAY_SIZE(fpath)) continue; *++p = '\0'; lstrcatW (fpath, fd.cFileName); } @@ -1324,6 +1327,7 @@ static BOOL WCMD_delete_one (const WCHAR *thisArg) { GetFullPathNameW(argCopy, ARRAY_SIZE(thisDir), thisDir, NULL); _wsplitpath(thisDir, drive, dir, fname, ext);
+ if (wcslen(drive) + wcslen(dir) + 1 + 1 > ARRAY_SIZE(thisDir)) return found; lstrcpyW(thisDir, drive); lstrcatW(thisDir, dir); cPos = lstrlenW(thisDir); @@ -1350,6 +1354,7 @@ static BOOL WCMD_delete_one (const WCHAR *thisArg) { DIRECTORY_STACK *nextDir; WCHAR subParm[MAX_PATH];
+ if (wcslen(thisDir) + wcslen(fd.cFileName) + 1 + wcslen(fname) + wcslen(ext) + 1 > ARRAY_SIZE(subParm)) continue; /* Work out search parameter in sub dir */ lstrcpyW (subParm, thisDir); lstrcatW (subParm, fd.cFileName); @@ -1746,6 +1751,7 @@ static void WCMD_add_dirstowalk(DIRECTORY_STACK *dirsToWalk) {
/* Build a generic search and add all directories on the list of directories still to walk */ + if (wcslen(dirsToWalk->dirName) + 2 + 1 > ARRAY_SIZE(fullitem)) return; lstrcpyW(fullitem, dirsToWalk->dirName); lstrcatW(fullitem, L"\*"); hff = FindFirstFileW(fullitem, &fd); @@ -2316,12 +2322,14 @@ void WCMD_for (WCHAR *p, CMD_LIST **cmdList) { WINE_TRACE("Processing FOR filename %s\n", wine_dbgstr_w(fd.cFileName));
if (doRecurse) { + if (wcslen(dirsToWalk->dirName) + 1 + wcslen(fd.cFileName) + 1 > ARRAY_SIZE(fullitem)) continue; lstrcpyW(fullitem, dirsToWalk->dirName); lstrcatW(fullitem, L"\"); lstrcatW(fullitem, fd.cFileName); } else { if (prefixlen) lstrcpynW(fullitem, item, prefixlen + 1); fullitem[prefixlen] = 0x00; + if (wcslen(fullitem) + wcslen(fd.cFileName) + 1 > ARRAY_SIZE(fullitem)) continue; lstrcatW(fullitem, fd.cFileName); } doExecuted = TRUE; @@ -2964,6 +2972,7 @@ void WCMD_move (void) attribs = GetFileAttributesW(output); if (attribs != INVALID_FILE_ATTRIBUTES && (attribs & FILE_ATTRIBUTE_DIRECTORY)) { + if (wcslen(output) + 1 + wcslen(fd.cFileName) + 1 > ARRAY_SIZE(dest)) continue; lstrcpyW(dest, output); lstrcatW(dest, L"\"); lstrcatW(dest, fd.cFileName); diff --git a/programs/cmd/directory.c b/programs/cmd/directory.c index 24b18bfa81b..3eba2c3e4fc 100644 --- a/programs/cmd/directory.c +++ b/programs/cmd/directory.c @@ -480,6 +480,7 @@ static DIRECTORY_STACK *WCMD_list_directory (DIRECTORY_STACK *inputparms, int le while (dirsToCopy > 0) { dirsToCopy--;
+ if (wcslen(inputparms->dirName) + wcslen(finddata.cFileName) + 1 + 1 > ARRAY_SIZE(string)) continue; /* Work out search parameter in sub dir */ lstrcpyW (string, inputparms->dirName); lstrcatW (string, finddata.cFileName);