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. > > 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.
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. > On 2024-11-08 Pali Rohár wrote: > > It breaks usage of global variables __argc and __argv[]. > > It breaks support for global variable _dowildcard. > > _dowildcard is passed to __wgetmainargs() so the argv[] for main() can > have wildcards expanded like before. It's the __getmainargs() at the > end that doesn't use _dowildcard so __argv[] won't have wildcards > expanded. I see. So the __argv[] does respect _dowildcard and argv[] respect it. > I wonder if it could be OK to set __argv = argv (and also __argc) right > before calling _tmain() and restore the old values immediately after > _tmain() returns. This is something about which I was thinking too. In my opinion this could work but it would be better to create a wrapper function which do all needed stuff. I do not think that restoring previous value into __argv is needed after _tmain() returns. But needed to recheck. > > And it also affects how environment variables works. With this > > change, wide variant of environment variables are initialized even > > when compiling in non-unicode mode. > > 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. Yes, this is my point. I'm really not sure if initializing both environment at startup will work correctly if CRT narrow and wide functions, and also with external / 3rd libraries. This looks like something which application libraries do not have to expect. > For UCRT only use, I had a prototype version which modified > misc/ucrt__getmainargs.c instead of crt/crtexe.c. The modified > __getmainargs() called _initialize_narrow_environment() like it does > now. However, argv was constructed from _configure_wide_argv() with > character set conversion. It also set *__p___argv() to point to the > properly-constructor argv[] but I'm not sure if that was completely > safe. It depends on how UCRT itself uses that pointer. In my opinion modification of misc/ucrt__getmainargs.c could be a better approach. And maybe creating a misc/msvcrt__getmainargs.c for msvcrt variant. I think that overwriting *__p___argv() should be safe. But all needs to be rechecked. > > Also the error message about unsupported command line characters are > > redirected to nul when compiling with gcc's -mwindows option. Which > > makes all GUI applications less user friendly as they will silently > > fail without any notice. > > WinMain() takes only one string so I had assumed that it's a separate > problem to solve but I didn't test or even look how WinMain() is > actually called in MinGW-w64. Sorry. WinMain() is called by main() which is called from the same code. Application compiled with -mwindows has Windows GUI subsystem in PE header and Windows by default do not create stdout/stderr when running such PE binaries. So error message printed to stdout or stderr is not available in this case. Error messages for these cases should be printed to GUI by MessageBox function. 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. > > Btw, I think that this change also breaks all mingw-w64 applications > > for running on older Windows versions as IIRC flag > > WC_NO_BEST_FIT_CHARS was not available in older Windows versions. > > I had used WC_NO_BEST_FIT_CHARS when trying more permissive variants > but I suppose it's not needed in the version that was committed. The > last argument to WideCharToMultiByte() (lpUsedDefaultChar / > conv_was_lossy) should be enough to detect if conversion was lossy. > When the program will exit on any lossy conversion, it doesn't matter > if such conversion used best-fit mapping. > > > Change has really big potential to break lot of applications which > > are working fine. And because this new behavior cannot be turned off > > by any compile option there is no workaround for those applications. > > > > Instead of applying it globally for all applications, it could be > > backward compatible. For example enabled by some global variable, > > like it was done for all other new msvcrt backward incompatible > > features. > > A large number of applications that use main() instead of wmain() are > potentially vulnerable at the moment. They are working fine only if it > is guaranteed that nothing will pass malicious arguments to them. Well, the vulnerability is with file / disk access. But if the application does not use argv[] for files, but for example for connecting to TCP server, or for printing messages to stdout or similar cases then such application should not be vulnerable. And such application rather want "best fit" mapping than crashing at 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. > > Note that above change does not fix following bug > > https://sourceforge.net/p/mingw-w64/bugs/809/ because argv is > > still leaking memory after "Avoid best-fit mapping" change. > > I had a very short discussion about this memory leak on IRC with LIU Hao > before he committed my "Avoid best-fit" patch. I got an impression that > it's not really fixable because something from argv[] may have been > passed to another thread which may still be running when the main() or > wmain() returns. Freeing would be possible only after all other threads > have terminated. 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. > > -- > Lasse Collin This is of course bug and should be fixed. _______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
