在 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.
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()`?
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 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.
-- Best regards, LIU Hao
OpenPGP_signature.asc
Description: OpenPGP digital signature
_______________________________________________ Mingw-w64-public mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/mingw-w64-public
