On 2024-11-12 Lasse Collin wrote:
> On 2024-11-12 Pali Rohár wrote:
> > At least every character which is by best-fit mapped to double quote
> > has to be replaced.
>
> Yes, that seems mandatory.
This alone might not be enough because a double quote can be escaped
with a backslash. Thus unexpected backslashes would need to be
sanitized too. It's simpler to convert with WC_NO_BEST_FIT_CHARS.
I attached a version of _acmdln sanitization for discussion and
testing. I'm not suggesting to include this in MinGW-w64 for now. Let's
see first what Microsoft does.
The patch does nothing with UTF-8 code page because U+FFFD seems the
least bad replacement character for unpaired surrogates. With other code
pages, underscore is used as the replacement character. A question mark
is perhaps clearer for humans but if _dowildcard is enabled and the
sanitized string contained a filename, those question marks would
expand back to the problematic name.
A problem with underscore is that it's a valid char in filenames. If
_dowildcard is disabled and the app itself doesn't expand wildcards
either, then a question mark is better because it's invalid in
filenames. That is, a wrong file wouldn't be accidentally accessed. I
don't know what is the least bad choice.
If best-fit mapping from wildcard expansion was ignored, something like
this patch might perhaps be an OK compromise as the default behavior
(not completely safe but safer than the old behavior). Otherwise also
the wargv-to-argv code is needed. Even then, other best-fit issues can
exist in apps like FindFirstFileA().
On 2024-11-12 LIU Hao wrote:
> The security issue is actually about file names (path injection).
Filenames are a common case but it can also be a problem if an app needs
to escape special characters in an untrusted non-filename string before
using it as a command line argument to another program. Best-fit
mapping can introduce new characters that should have been escaped.
--
Lasse Collin
diff --git a/mingw-w64-crt/crt/crtexe.c b/mingw-w64-crt/crt/crtexe.c
index cdf5dcd25..45c264e0b 100644
--- a/mingw-w64-crt/crt/crtexe.c
+++ b/mingw-w64-crt/crt/crtexe.c
@@ -130,6 +130,29 @@ pre_cpp_init (void)
#ifdef _UNICODE
argret = __wgetmainargs(&argc,&argv,&envp,_dowildcard,&startinfo);
#else
+ if (GetACP() != CP_UTF8 && _wcmdln != NULL) {
+ const char replacement_mbchar[] = "_";
+ BOOL conv_was_lossy = TRUE;
+ int acmdln_size = WideCharToMultiByte(
+ CP_ACP, WC_NO_BEST_FIT_CHARS,
+ _wcmdln, -1,
+ NULL, 0,
+ replacement_mbchar, &conv_was_lossy);
+
+ // If the above WideCharToMultiByte() failed, _acmdln isn't changed.
+ if (acmdln_size > 0 && conv_was_lossy) {
+ _acmdln = malloc(acmdln_size);
+ if (_acmdln == NULL)
+ _amsg_exit(8); // R6008 "not enough space for arguments" with MSVCRT
+
+ WideCharToMultiByte(
+ CP_ACP, WC_NO_BEST_FIT_CHARS,
+ _wcmdln, -1,
+ _acmdln, acmdln_size,
+ replacement_mbchar, NULL);
+ }
+ }
+
argret = __getmainargs(&argc,&argv,&envp,_dowildcard,&startinfo);
#endif
}
_______________________________________________
Mingw-w64-public mailing list
Mingw-w64-public@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
