Protect and test programs/cmd against path which length exceeds MAX_PATH patch to programs/cmd is applied before adding the tests as the tests make cmd crash with a stack overflow
A+ ---
Eric Pouech (2): programs/cmd: don't overflow internal buffers programs/cmd/tests: test paths which length exceeds MAX_PATH
programs/cmd/builtins.c | 39 +++++++++++++++++------- programs/cmd/tests/test_builtins.cmd | 19 ++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 4 +++ programs/cmd/wcmdmain.c | 10 ++++-- 4 files changed, 58 insertions(+), 14 deletions(-)
Protect and test programs/cmd against path which length exceeds MAX_PATH Patch to programs/cmd is applied before adding the tests as the tests make cmd.exe crash with a stack overflow
A+
v2: - not forgetting to send the patches along - added helper to wrap around testing for overflow (including error message to user) - fixed a couple of other locations for potential overflow
---
Eric Pouech (2): programs/cmd: bail out when full path name exceeds MAX_PATH programs/cmd/tests: test paths which length exceeds MAX_PATH
programs/cmd/batch.c | 2 +- programs/cmd/builtins.c | 21 +++++++++++---------- programs/cmd/cmd.rc | 1 + programs/cmd/directory.c | 2 +- programs/cmd/tests/test_builtins.cmd | 19 +++++++++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 4 ++++ programs/cmd/wcmd.h | 2 ++ programs/cmd/wcmdmain.c | 18 +++++++++++++++--- 8 files changed, 54 insertions(+), 15 deletions(-)
this mimics native behavior and prevents buffer overflows note: this is far from fixing all potential buffer overflows in builtin cmd.exe, just a small step into the right direction
Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/cmd/batch.c | 2 +- programs/cmd/builtins.c | 21 +++++++++++---------- programs/cmd/cmd.rc | 1 + programs/cmd/directory.c | 2 +- programs/cmd/wcmd.h | 2 ++ programs/cmd/wcmdmain.c | 18 +++++++++++++++--- 6 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/programs/cmd/batch.c b/programs/cmd/batch.c index 2ff8e3ba9be..9a262c5fec5 100644 --- a/programs/cmd/batch.c +++ b/programs/cmd/batch.c @@ -472,7 +472,7 @@ void WCMD_HandleTildeModifiers(WCHAR **start, BOOL atExecute) /* After this, we need full information on the file, which is valid not to exist. */ if (!skipFileParsing) { - if (GetFullPathNameW(outputparam, MAX_PATH, fullfilename, &filepart) == 0) { + if (!WCMD_get_fullpath(outputparam, MAX_PATH, fullfilename, &filepart)) { exists = FALSE; fullfilename[0] = 0x00; } else { diff --git a/programs/cmd/builtins.c b/programs/cmd/builtins.c index d14e69e072a..2fc7e07f7aa 100644 --- a/programs/cmd/builtins.c +++ b/programs/cmd/builtins.c @@ -805,7 +805,7 @@ void WCMD_copy(WCHAR * args) { WINE_TRACE("Destination supplied, processing to see if file or directory\n");
/* Convert to fully qualified path/filename */ - GetFullPathNameW(destination->name, ARRAY_SIZE(destname), destname, &filenamepart); + if (!WCMD_get_fullpath(destination->name, ARRAY_SIZE(destname), destname, &filenamepart)) return; WINE_TRACE("Full dest name is '%s'\n", wine_dbgstr_w(destname));
/* If parameter is a directory, ensure it ends in \ */ @@ -887,7 +887,7 @@ void WCMD_copy(WCHAR * args) {
/* Convert to fully qualified path/filename in srcpath, file filenamepart pointing to where the filename portion begins (used for wildcard expansion). */ - GetFullPathNameW(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart); + if (!WCMD_get_fullpath(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart)) return; WINE_TRACE("Full src name is '%s'\n", wine_dbgstr_w(srcpath));
/* If parameter is a directory, ensure it ends in * */ @@ -897,7 +897,7 @@ void WCMD_copy(WCHAR * args) { /* We need to know where the filename part starts, so append * and recalculate the full resulting path */ lstrcatW(thiscopy->name, L"*"); - GetFullPathNameW(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart); + if (!WCMD_get_fullpath(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart)) return; WINE_TRACE("Directory, so full name is now '%s'\n", wine_dbgstr_w(srcpath));
} else if ((wcspbrk(srcpath, L"*?") == NULL) && @@ -907,7 +907,7 @@ void WCMD_copy(WCHAR * args) { /* We need to know where the filename part starts, so append * and recalculate the full resulting path */ lstrcatW(thiscopy->name, L"\*"); - GetFullPathNameW(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart); + if (!WCMD_get_fullpath(thiscopy->name, ARRAY_SIZE(srcpath), srcpath, &filenamepart)) return; WINE_TRACE("Directory, so full name is now '%s'\n", wine_dbgstr_w(srcpath)); }
@@ -1191,7 +1191,7 @@ static BOOL WCMD_delete_confirm_wildcard(const WCHAR *filename, BOOL *pPrompted) WCHAR fpath[MAX_PATH];
/* Convert path into actual directory spec */ - GetFullPathNameW(filename, ARRAY_SIZE(fpath), fpath, NULL); + if (!WCMD_get_fullpath(filename, ARRAY_SIZE(fpath), fpath, NULL)) return FALSE; _wsplitpath(fpath, drive, dir, fname, ext);
/* Only prompt for * and *.*, not *a, a*, *.a* etc */ @@ -1319,7 +1319,8 @@ static BOOL WCMD_delete_one (const WCHAR *thisArg) { WCHAR ext[MAX_PATH];
/* Convert path into actual directory spec */ - GetFullPathNameW(argCopy, ARRAY_SIZE(thisDir), thisDir, NULL); + if (!WCMD_get_fullpath(argCopy, ARRAY_SIZE(thisDir), thisDir, NULL)) return FALSE; + _wsplitpath(thisDir, drive, dir, fname, ext);
lstrcpyW(thisDir, drive); @@ -2933,8 +2934,8 @@ void WCMD_move (void)
/* If 2nd parm is directory, then use original filename */ /* Convert partial path to full path */ - GetFullPathNameW(param1, ARRAY_SIZE(input), input, NULL); - GetFullPathNameW(param2, ARRAY_SIZE(output), output, NULL); + if (!WCMD_get_fullpath(param1, ARRAY_SIZE(input), input, NULL) || + !WCMD_get_fullpath(param2, ARRAY_SIZE(output), output, NULL)) return; WINE_TRACE("Move from '%s'('%s') to '%s'\n", wine_dbgstr_w(input), wine_dbgstr_w(param1), wine_dbgstr_w(output));
@@ -3158,7 +3159,7 @@ void WCMD_rename (void) }
/* Convert partial path to full path */ - GetFullPathNameW(param1, ARRAY_SIZE(input), input, NULL); + if (!WCMD_get_fullpath(param1, ARRAY_SIZE(input), input, NULL)) return; WINE_TRACE("Rename from '%s'('%s') to '%s'\n", wine_dbgstr_w(input), wine_dbgstr_w(param1), wine_dbgstr_w(param2)); dotDst = wcschr(param2, '.'); @@ -3439,7 +3440,7 @@ void WCMD_setshow_default (const WCHAR *args) { WCHAR ext[MAX_PATH];
/* Convert path into actual directory spec */ - GetFullPathNameW(string, ARRAY_SIZE(fpath), fpath, NULL); + if (!WCMD_get_fullpath(string, ARRAY_SIZE(fpath), fpath, NULL)) return; _wsplitpath(fpath, drive, dir, fname, ext);
/* Rebuild path */ diff --git a/programs/cmd/cmd.rc b/programs/cmd/cmd.rc index fa604a6db56..f72ba87a9a2 100644 --- a/programs/cmd/cmd.rc +++ b/programs/cmd/cmd.rc @@ -406,4 +406,5 @@ Enter HELP <command> for further information on any of the above commands.\n" WCMD_NOOPERATOR, "Expected an operator.\n" WCMD_BADPAREN, "Mismatch in parentheses.\n" WCMD_BADHEXOCT, "Badly formed number - must be one of decimal (12),\n hexadecimal (0x34) or octal (056).\n" + WCMD_FILENAMETOOLONG, "File name is too long.\n" } diff --git a/programs/cmd/directory.c b/programs/cmd/directory.c index 24b18bfa81b..807f7b386b1 100644 --- a/programs/cmd/directory.c +++ b/programs/cmd/directory.c @@ -787,7 +787,7 @@ void WCMD_directory (WCHAR *args) } WINE_TRACE("Using location '%s'\n", wine_dbgstr_w(fullname));
- status = GetFullPathNameW(fullname, ARRAY_SIZE(path), path, NULL); + if (!WCMD_get_fullpath(fullname, ARRAY_SIZE(path), path, NULL)) continue;
/* * If the path supplied does not include a wildcard, and the endpoint of the diff --git a/programs/cmd/wcmd.h b/programs/cmd/wcmd.h index a02395ca123..234c253b49a 100644 --- a/programs/cmd/wcmd.h +++ b/programs/cmd/wcmd.h @@ -71,6 +71,7 @@ void WCMD_endlocal (void); void WCMD_enter_paged_mode(const WCHAR *); void WCMD_exit (CMD_LIST **cmdList); void WCMD_for (WCHAR *, CMD_LIST **cmdList); +BOOL WCMD_get_fullpath(const WCHAR *, SIZE_T, WCHAR *, WCHAR **); void WCMD_give_help (const WCHAR *args); void WCMD_goto (CMD_LIST **cmdList); void WCMD_if (WCHAR *, CMD_LIST **cmdList); @@ -317,3 +318,4 @@ extern WCHAR version_string[]; #define WCMD_NOOPERATOR 1043 #define WCMD_BADPAREN 1044 #define WCMD_BADHEXOCT 1045 +#define WCMD_FILENAMETOOLONG 1046 diff --git a/programs/cmd/wcmdmain.c b/programs/cmd/wcmdmain.c index 675d7c5f464..65601348b05 100644 --- a/programs/cmd/wcmdmain.c +++ b/programs/cmd/wcmdmain.c @@ -452,6 +452,18 @@ void WCMD_strsubstW(WCHAR *start, const WCHAR *next, const WCHAR *insert, int le memcpy(start, insert, len * sizeof(*insert)); }
+BOOL WCMD_get_fullpath(const WCHAR* in, SIZE_T outsize, WCHAR* out, WCHAR** start) +{ + DWORD ret = GetFullPathNameW(in, outsize, out, start); + if (!ret) return FALSE; + if (ret > outsize) + { + WCMD_output_asis_stderr(WCMD_LoadMessage(WCMD_FILENAMETOOLONG)); + return FALSE; + } + return TRUE; +} + /*************************************************************************** * WCMD_skip_leading_spaces * @@ -1057,7 +1069,7 @@ void WCMD_run_program (WCHAR *command, BOOL called) } else {
/* Convert eg. ..\fred to include a directory by removing file part */ - GetFullPathNameW(firstParam, ARRAY_SIZE(pathtosearch), pathtosearch, NULL); + if (!WCMD_get_fullpath(firstParam, ARRAY_SIZE(pathtosearch), pathtosearch, NULL)) return; lastSlash = wcsrchr(pathtosearch, '\'); if (lastSlash && wcschr(lastSlash, '.') != NULL) extensionsupplied = TRUE; lstrcpyW(stemofsearch, lastSlash+1); @@ -1125,7 +1137,7 @@ void WCMD_run_program (WCHAR *command, BOOL called)
/* Since you can have eg. .... on the path, need to expand to full information */ - GetFullPathNameW(temp, MAX_PATH, thisDir, NULL); + if (!WCMD_get_fullpath(temp, ARRAY_SIZE(thisDir), thisDir, NULL)) return; }
/* 1. If extension supplied, see if that file exists */ @@ -2564,7 +2576,7 @@ int __cdecl wmain (int argc, WCHAR *argvW[]) WINE_TRACE("First parameter is '%s'\n", wine_dbgstr_w(thisArg)); if (wcschr(thisArg, '\') != NULL) {
- GetFullPathNameW(thisArg, ARRAY_SIZE(string), string, NULL); + if (!WCMD_get_fullpath(thisArg, ARRAY_SIZE(string), string, NULL)) return FALSE; WINE_TRACE("Full path name '%s'\n", wine_dbgstr_w(string)); p = string + lstrlenW(string);
v2: fix failures on windows' VM
Signed-off-by: Eric Pouech eric.pouech@gmail.com
--- programs/cmd/tests/test_builtins.cmd | 19 +++++++++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 4 ++++ 2 files changed, 23 insertions(+)
diff --git a/programs/cmd/tests/test_builtins.cmd b/programs/cmd/tests/test_builtins.cmd index 47b028207ac..56a682577bd 100644 --- a/programs/cmd/tests/test_builtins.cmd +++ b/programs/cmd/tests/test_builtins.cmd @@ -3333,6 +3333,25 @@ if errorlevel 1 echo Normal+tab+garbage drive change failed
popd
+echo ------------ Testing length wrt. MAX_PATH ------------ +rem native cmd limits all path lengths to MAX_PATH=260 +pushd c:\ +set depth=25 +for /L %%d in (0,1,25) do ( + mkdir abcdefghij > NUL 2>&1 + if exist abcdefghij ( + cd abcdefghij + set depth=%%d + ) +) +echo %depth% +rem even relative paths are transformed to absolute, and tested against MAX_PATH +echo abc > 01234567890123 +if exist 01234567890123 (echo Success) else echo Failure +echo abc > 012345678901234 +if exist 012345678901234 (echo Failure) else echo Success +popd +rmdir /s /q c:\abcdefghij echo ------------ Testing combined CALLs/GOTOs ------------ echo @echo off>foo.cmd echo goto :eof>>foot.cmd diff --git a/programs/cmd/tests/test_builtins.cmd.exp b/programs/cmd/tests/test_builtins.cmd.exp index 4c7bc3e0c02..8b6e0914112 100644 --- a/programs/cmd/tests/test_builtins.cmd.exp +++ b/programs/cmd/tests/test_builtins.cmd.exp @@ -1687,6 +1687,10 @@ Normal+space+garbage Quoted should fail Normal+tab Normal+tab+garbage +------------ Testing length wrt. MAX_PATH ------------ +@todo_wine@21 +Success +@todo_wine@Success ------------ Testing combined CALLs/GOTOs ------------ world cheball
Hi Alexandre,
what's the status of these patches? A+ Le 21/01/2022 à 10:32, Eric Pouech a écrit :
Protect and test programs/cmd against path which length exceeds MAX_PATH Patch to programs/cmd is applied before adding the tests as the tests make cmd.exe crash with a stack overflow
A+
v2:
- not forgetting to send the patches along
- added helper to wrap around testing for overflow (including error message to user)
- fixed a couple of other locations for potential overflow
Eric Pouech (2): programs/cmd: bail out when full path name exceeds MAX_PATH programs/cmd/tests: test paths which length exceeds MAX_PATH
programs/cmd/batch.c | 2 +- programs/cmd/builtins.c | 21 +++++++++++---------- programs/cmd/cmd.rc | 1 + programs/cmd/directory.c | 2 +- programs/cmd/tests/test_builtins.cmd | 19 +++++++++++++++++++ programs/cmd/tests/test_builtins.cmd.exp | 4 ++++ programs/cmd/wcmd.h | 2 ++ programs/cmd/wcmdmain.c | 18 +++++++++++++++--- 8 files changed, 54 insertions(+), 15 deletions(-)
-
Eric Pouech -
Eric Pouech