-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter #6884
base: master
Are you sure you want to change the base?
[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter #6884
Conversation
CORE-19372 CORE-19583
The WM_SETTINGCHANGE string in lparam can be any string but this should fix the most common broadcasts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other LGTM. ✅
|
||
/* Strings that are OK to pass between user and kernel mode | ||
* There may be other strings needed that can easily be added here. */ | ||
WCHAR StrUserKernel[3][20] = {{L"intl"}, {L"Environment"}, {L"Policy"}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why putting these strings inside "sublists"? Why not just doing something like:
WCHAR StrUserKernel[3][20] = {{L"intl"}, {L"Environment"}, {L"Policy"}}; | |
PCWSTR StrUserKernel[] = {L"intl", L"Environment", L"Policy"}; |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HBelusca, I know that what I have here will allow me to use understandable derived values as follows:
ARRAYSIZE(StrUserKernel) = 3
sizeof(StrUserKernel[0]) = 40
_countof(StrUserKernel[0]) = 20
To me what you are suggesting seems to be the very definition of "ill-defined".
What would be the values above when the array is defined without integer constant numbers?
With what you have proposed, I cannot tell what the above values would return by simple inspection.
but with the values [3][20]
it seems obvious to me. Admittedly, this is because of my poor understanding.
Nevertheless, I do appreciate your comments and your trying to help me with this. Thanks.
win32ss/user/ntuser/message.c
Outdated
@@ -2721,6 +2767,12 @@ NtUserMessageCall( HWND hWnd, | |||
} | |||
ExFreePoolWithTag(List, USERTAG_WINDOWLIST); | |||
} | |||
if (lParam && !wParam && wcscmp((WCHAR*)lParam, L"Environment") == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this comparison be done case-sensitively, or not? (i.e. if the string "enVIroNmEnT"
is passed instead, should the broadcast still be done successfully or not?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string is not case sensitive. Not that it matters, all strings are supposed to be supported. Currently it's missing "SystemDockMode" and "ConvertibleSlateMode" (and all possible registry key names).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this lParam parameter come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this lParam parameter come from?
When the message is WM_SETTINGCHANGE, lParam can be NULL or a string that says which setting you changed. There are maybe about 7-8 strings that are "standard" but it can also be the name of a registry key.
I believe it is supposed to marshal all strings in this case and checking for "Environment" seems like a hack to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checking for "Environment" seems like a hack to me.
I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In accordance with the title of this PR, it was intended to only handle Environment
strings. It can be expanded later in another PR. Anything else is "out of scope" for this PR. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whindsaks, I see that you have given a "thumbs down" comment here. I just do not have the background in this area of the code to do much more with it for now. Are you willing to pick this up? If so, you are free to make any changes here that you want. Or if you want to create a completely new PR, I can close this one. I do appreciate your willingness to help here and want to encourage you in any way that I can. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know much about the kernel part of the window manager either, I just feel that we should try to make it work with all strings from the start, not a hack like this for just one string.
win32ss/user/ntuser/message.c
Outdated
_SEH2_TRY | ||
{ | ||
if (UserModeMsg->lParam) | ||
RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg)); | |
RtlCopyMemory(lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fully sure this can work reliably.
If someone passes an arbitrary pointer to a non-null-terminated string via the lParam with a size completely unrelated to the "hardcoded" sizeof(...) .
Later down you call PosInArray(lParamMsg)
and wcscpy outside a SEH2 block, assuming the copied string is properly NUL-terminated, but you haven't verified that this assumption is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone passes a bad value string as you suggest, when this goes to PosInArray
then it will not match any of the 3 strings there and will return a -1 which will disallow it getting to wcscpy
function. Maybe it can be extended later for more general strings. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1- The local lParamMsg
buffer is somewhere in memory. Let's suppose it can contain 12 WCHARs (so 24 bytes), but what's after it is undefined (let's say they can be non-zero bytes, or the beginning of an unaccessible new memory page).
2- Now, the caller (the user) passes in the lParam
(which gets placed in UserModeMsg->lParam
) a user-mode pointer to a non-NUL-terminated "string", i.e. pointer to some crafted random garbage, let's say to this string:
L"EnvironmentXXXXXXXXX" (it has more than 12 WCHARs ie. more than 24 bytes).
3- Your RtlCopyMemory will copy only the first 12 WCHARs into lParamMsg (without any NUL-terminator), so that lParamMsg will contain:
"Environment<GARBAGE_FOLLOWS>" (as described in point 1).
4- The PosInArray(lParamMsg)
does a if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0)
, scanning i = 0 to 2 or whatever (to try all the hardcoded strings in StrUserKernel
array).
When it tries the second string (index i == 1
), i.e. "Environment", it'll try to match the "N-th" first characters; but: first, your sizeof(StrUserKernel[0])
is ill-defined, and also the wcsncmp may try to compare whether the terminating NUL-character of the trial string "Environment" exists also in the caller-given String
(where, I recall, there is garbage following). In that case it'll read into forbidden garbage and can cause a BSOD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HBelusca, is your statement that says your sizeof(StrUserKernel[0] is ill-defined
still correct when I have used this define at line 19?
WCHAR StrUserKernel[3][20]...
I have done prints of the values of sizeof and _countof as you can see in my response to Mark and they seem to know that this is a WCHAR of length 20 wchar's or 40 bytes.
Thanks for your comments, explanation, and consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra space removed. I have added code to make sure that there is a UNICODE_NULL in lParamMsg now. See if this is OK with you. Thanks.
win32ss/user/ntuser/message.c
Outdated
@@ -459,8 +483,30 @@ CopyMsgToKernelMem(MSG *KernelModeMsg, MSG *UserModeMsg, PMSGMEMORY MsgMemoryEnt | |||
/* Copy data if required */ | |||
if (0 != (MsgMemoryEntry->Flags & MMS_FLAG_READ)) | |||
{ | |||
WCHAR lParamMsg[sizeof(StrUserKernel[0])/2 + 1] = { 0 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WCHAR lParamMsg[sizeof(StrUserKernel[0])/2 + 1] = { 0 }; | |
WCHAR lParamMsg[_countof(StrUserKernel[0]) + 1] = { 0 }; |
or using RTL_NUMBER_OF
or ARRAYSIZE
.
win32ss/user/ntuser/message.c
Outdated
|
||
for (i = 0; i < End; ++i) | ||
{ | ||
if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why sizeof(StrUserKernel[0])
? since the wcsncmp takes a number of characters (and not a number of bytes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0) | |
if (wcsncmp(String, StrUserKernel[i], _countof(StrUserKernel[0])) == 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only checks the first 4 chars of each string indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@learn-more, Why do you say "This only checks the first 4 chars of each string indeed."?
Maybe I am not understanding something here?
I added this statement just above this "if" statement now at Line 36.
ERR("sizeof is %d, countof is %d\n", sizeof(StrUserKernel[0]), _countof(StrUserKernel[0]));
When the program is run, its debug output shows the following:
(win32ss/user/ntuser/message.c:36) err: sizeof is 40, countof is 20
What am I missing here? Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
sizeof(StrUserKernel[0])
? since the wcsncmp takes a number of characters (and not a number of bytes)
@HBelusca, I will correct this, I missed the fact that wcsncmp takes a number of characters and not bytes. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed sizeof
to _countof
. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it tries to use 20
for every string, even strings that are smaller.
Why is that used as max length?
win32ss/user/ntuser/message.c
Outdated
_SEH2_TRY | ||
{ | ||
if (UserModeMsg->lParam) | ||
RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1- The local lParamMsg
buffer is somewhere in memory. Let's suppose it can contain 12 WCHARs (so 24 bytes), but what's after it is undefined (let's say they can be non-zero bytes, or the beginning of an unaccessible new memory page).
2- Now, the caller (the user) passes in the lParam
(which gets placed in UserModeMsg->lParam
) a user-mode pointer to a non-NUL-terminated "string", i.e. pointer to some crafted random garbage, let's say to this string:
L"EnvironmentXXXXXXXXX" (it has more than 12 WCHARs ie. more than 24 bytes).
3- Your RtlCopyMemory will copy only the first 12 WCHARs into lParamMsg (without any NUL-terminator), so that lParamMsg will contain:
"Environment<GARBAGE_FOLLOWS>" (as described in point 1).
4- The PosInArray(lParamMsg)
does a if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0)
, scanning i = 0 to 2 or whatever (to try all the hardcoded strings in StrUserKernel
array).
When it tries the second string (index i == 1
), i.e. "Environment", it'll try to match the "N-th" first characters; but: first, your sizeof(StrUserKernel[0])
is ill-defined, and also the wcsncmp may try to compare whether the terminating NUL-character of the trial string "Environment" exists also in the caller-given String
(where, I recall, there is garbage following). In that case it'll read into forbidden garbage and can cause a BSOD.
|
For WM_SETTINGCHANGE, wParam is one of the SPI constants. I don't know if it is legal to combine a SPI with a string but I see no reason to block it. |
Ok, after thinking about this a bit more, here are my thoughts. Whenever a pointer to a buffer of variable size enters the kernel from user32 to win32k, user32 does some preparation in order for win32k to be able to consume it. There has to be some work done in user32 to facilitate delivering of these strings. To make matters worse, some applications are ANSI only and some are unicode, some send messages with ansi strings and some send messages with unicode strings. You need to take all these details into consideration. There is one fundamental question about how this feature works: if it supports only predefined strings or if it supports any kind of arbitrary string. If it is the first I wouldn't be surprised if windows' user32 replaced the predefined string with an enumeration and leave win32k oblivious to the details, and likewise replace the enumeration with a real string pointer right before calling a message proc. (solution a) If it has to carry an arbitrary string then it is most likely that user32 replaces the string pointer with a pointer to a UNICODE_STRING on the stack that will also carry the length of the string. Then the kernel in this case would marshal the UNICODE_STRING from one process to the other and right before the message proc gets called, user32 would replace the pointer to the UNICODE_STRING, with a pointer to the raw string. (solution b) So please first of all write some tests to see the following:
Depending on the test from 1 we should proceed with either solution a or solution b. Unfortunately there is no other way around getting this string pointer into the kernel , it can't enter as is without its length. User32 needs to be involved before the kernel call. This is can be a bit daunting but one of the reasons such a feature hasn't been implemented yet is because it is not trivial. In your solution for example you copy the string without probing and without knowing the real size of the string which is absolutely wrong. PS: It would help if someone could check with a debugger to see what parameters user32 passes to NtUserMessageCall win windows. PS2: Please also check what windows does when you send such a message with an lparam string with the BSF_POSTMESSAGE parameter. I guess it will return an error, |
I can confirm that you can send any string you want (in the
Edit: And for WM_SETTINGCHANGE , it does not care if WPARAM is 0 or not, it will marshal the LPARAM string either way. |
So here is my assumption what needs to be done: user32!MsgiUMToKMMessage: for WM_SETTINGCHANGE that has lparam, allocate a UNICODE_STRING, initialize it from the user mode lparam and use its pointer as the lparam for the KMMsg. user32!MsgiUMToKMCleanup: deallocate the UNICODE_STRING mentioend above user32!MsgiKMToUMMessage: use the buffer of the UNICODE_STRING as the lparam for the user mode message user32!MsgiAnsiToUnicodeMessage: handle WM_SETTINGCHANGE like WM_SETTEXT user32!MsgiAnsiToUnicodeCleanup: handle WM_SETTINGCHANGE like WM_SETTEXT The above are needed to let win32k always handle proper UNICODE_STRINGs instead of a mishmash of unicode or ansi string pointers. Now for win32k I found this totally retarded thing. Right now, without the changes of this MR the first thing that win32k does when someone sends a WM_SETTEXT or a WM_SETTINGCHANGEm win32k goes one and does a wcslen on the lparam coming from user mode, without any probing and assuming that the string will be null terminated. (Only SEH prevents the kernel from crashing). So for win32k the correct way to go would be to assume that we get a UNICODE_STRING when a message has the MMS_SIZE_LPARAMSZ flag. This got me into a never ending rabbit hole (typical for win32k stuff) and realized that our implementation of WM_SETTEXT is also a total mess and probably needs something like the UNICODE_STRING treatment I mentioned above. So continuing with what needs to be done in win32k: win32k!CopyMsgToUserMem: I'm not sure if this needs to be involved Here is another important question however: win32k already has a (bad) implementation of marshaling the WM_SETTINGCHANGE lparam string (with the MMS_SIZE_LPARAMSZ flag). Why is this MR needed when some infrastructure for that is already in place? |
And when BSF_POSTMESSAGE is present? It still needs to marshal the string. How Windows does this, I don't know. |
In this case it probably returns an error. This is just a guess but this is what I conclude judging how PostMessage works, which does not marshal pointers accross processes. If it did, win32k would need to keep track on its own the marshaled memory. What is a process never receives? The message stays forever in kernel memory? What windows does is that it marshals messages only when two threads synchronize through SendMessage (and similar funcitons). SendMessage blocks till the recipient message proc returns and during this time windows also do marshal pointers. |
But I already tested this on Windows and BSF_POSTMESSAGE does work when LPARAM is a string! |
even across processes? |
Educated guess, could it be adding atoms for this? |
Purpose
Implement BroadcastSystemMessage with Environment parameter.
JIRA issues: CORE-19372 and CORE-19583
Proposed changes
BroadcastSystemMessage needs to be implemented.
This allows programs to update Environment variables like PATH.