On Saturday 09 November 2024 22:39:08 LIU Hao wrote: > 在 2024-11-09 02:23, Pali Rohár 写道: > > On Friday 08 November 2024 19:49:24 Lasse Collin wrote: > > > Hello! > > > > > > I apologize for the hassle. :-( It's a security issue that (I think) is > > > only half-public, so I didn't want to post to the public mailing list. > > > Apparently the tiny group of people I discussed this was too small (all > > > are in the Cc list now). I don't know a lot about Windows so I rely on > > > others to check my ideas. > > Frankly speaking, I am not too interested by Windows 98. '_mingw.h' used to > define `_WIN32_WINNT` to Windows XP 64-bit and Windows Server 2003 (both are > 0x0502), and that's really the minimal supported platform. > > As there's real security risk, the costs of maintenance and issues of > interoperability with ancient systems may be tolerable. > > > > > It seems that there are severe enough issues that reverting the commit > > > makes sense. But I hope something can be done still. It's not realistic > > > to fix a large number of apps that use main() instead of wmain(). In > > > my opinion, it's not really the apps that are broken. > > The secure issue is that the default wide-to-multibyte conversion can > convert Unicode full-width double quotes to ASCII double quotes, altering > how arguments are interpreted. Therefore, the narrow `main()` function may > not receive the same argv as passed from `_wspawnv()` or `_wexecv()`, even > if arguments are quoted properly. > > Specifically about security issues, the fact is that they are just there. It > doesn't matter whether X is broken or Y is broken; it's not helpful to say > 'there wouldn't have been such an issue if it had not worked this way.' > > > > I understand that this is a security issue. And it is too fragile > > because of (insane) MS APIs. That is why I wrote in end of the previous > > email that I can look at it and try to prepare some better fix. > > > > Anyway, as mingw-w64 is just using msvcrt / UCRT API, same problem is > > also compiled by MS VC, no? If MS is going to fix it then it would be a > > good idea to apply compatible fix into mingw-w64. > > Source code for the routines which parse and assemble `argv` and `envp` are > actually installable with Visual Studio, which you can examine yourself. > > Really, I don't think this should be fixed in the CRT. It should be fixed by > sanitizing the result in `GetCommandLineA()`. Reverting the commit makes > sense if Microsoft will fix it sooner or latter.
Has somebody already reported this security issue to Microsoft? I have also feeling that this is not something which should be fixed in CRT as basically everything which use result from GetCommandLineA() is affected. But on the other hand, I agree that it needs to be fixed for compiled application, so mingw-w64 is the only place for now. Note that GetCommandLineA() explicitly mention that conversion of U+FF02 to 0x22 is by design: https://learn.microsoft.com/en-us/windows/win32/api/processenv/nf-processenv-getcommandlinea So it means that GetCommandLineA() cannot be used for splitting command line string to argv[] array. > > > > From Microsoft docs, I understood that mixing calls to narrow and wide > > > environment functions is OK. CRT will convert in that case: > > > > > > https://learn.microsoft.com/en-us/cpp/c-runtime-library/environ-wenviron?view=msvc-170 > > > > > > But I admit that I wasn't fully confident that initializing both > > > environments this way is truly OK. > > Source files for all those routines are installed with Visual Studio 2022, > which you can read. For mingw-w64 we always link against the DLL, so I > suspect there's always both a narrow environment and a wide environment. > > > > As mingw-w64 runtime can fail in more cases, I have some ideas of having > > general "printf-like" function which will work for both CUI and GUI > > application (based on application type it will print either to stderr or > > to MessageBox). I can try prepare prototype of such function. > > Is the function called `abort()`? Abort does not take additional string which will be displayed to user. I was thinking about new/custom mingw-w64 specific function for this purpose. For example mingw-64 pseudo reloc code also can fail during startup (which is fatal error for application) but cannot display its error message for GUI application without stdout/stderr. So I was thinking about generic error displaying function which could be used from any mingw-w64 crt part during startup. > > > > A global variable that reverts to best-fit mapping would be reasonable. > > > However, the default should be such that main() is safe to use. It > > > would be weird if every modern app with main() would need > > > "int _secure_argv_parsing = 1;" or such. In practice it wouldn't be > > > done in most apps. > > > > > > If compatibility with old CRTs is complicated, perhaps fixing the issue > > > only with UCRT could be an intermediate step. > > > > I think that there are only two APIs which needs to be handled: UCRT and > > everything-before-UCRT. If we move the fix into misc/ucrt__getmainargs.c > > I can look at what is needed to be done for non-UCRT version. > > I have a crazy idea now. Does it make sense to overwrite `_acmdln` (for > MSVCRT) or `*__p__acmdln()` (for UCRT) with a sanitized string, so existent > argument parsing may be reused? I was thinking about exactly same thing. Note that we have __p__acmdln() wrapper also for msvcrt.dll builds, so it would not need to be UCRT specific. I think that we just need to read UCRT and msvcrt source code which is available as part of Visual Studio installations and decide if all those overwritings are safe. > > > I also do not think that it is fixable. But it is a memory leak? I do > > not think so, it is valid usage of memory which needs to be available > > during running of application. I'm just not sure if C/C++ requires that > > argv[] pointer has to be memory safe also after finishing main(). But > > what is the problem? Memory which is used during whole run of process > > does not have to call free() and return it. > > > > > Note that duplicate_ppstrings() doesn't currently check if malloc() > > > returns NULL. > > Those may be changed to `HeapAlloc(GetProcessHeap(), > HEAP_GENERATE_EXCEPTIONS, ...)`. Out-of-memory conditions during startup are > irrecoverable anyway. I think that this is easy to fix. There are lot of options, including check return value of malloc and call abort / exit / etc... _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
