[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter #6884

Doug-Lyons · 2024-05-14T04:03:02Z

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.

CORE-19372 CORE-19583

whindsaks · 2024-05-14T11:13:23Z

The WM_SETTINGCHANGE string in lparam can be any string but this should fix the most common broadcasts.

oleg-dubinskiy

In other LGTM. ✅

win32ss/user/ntuser/message.c

HBelusca · 2024-05-14T15:28:39Z

win32ss/user/ntuser/message.c

+
+/* 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"}};


Why putting these strings inside "sublists"? Why not just doing something like:

Suggested change

WCHAR StrUserKernel[3][20] = {{L"intl"}, {L"Environment"}, {L"Policy"}};

PCWSTR StrUserKernel[] = {L"intl", L"Environment", L"Policy"};

?

@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.

HBelusca · 2024-05-14T15:30:15Z

win32ss/user/ntuser/message.c

@@ -2721,6 +2767,12 @@ NtUserMessageCall( HWND hWnd,
                        }
                        ExFreePoolWithTag(List, USERTAG_WINDOWLIST);
                     }
+                     if (lParam && !wParam && wcscmp((WCHAR*)lParam, L"Environment") == 0)


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?)

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).

Where does this lParam parameter come from?

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.

checking for "Environment" seems like a hack to me.

I agree.

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.

@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.

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.

HBelusca · 2024-05-14T15:30:36Z

win32ss/user/ntuser/message.c

+            _SEH2_TRY
+            {
+                if (UserModeMsg->lParam)
+                    RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));


Suggested change

RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));

RtlCopyMemory(lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));

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.

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.

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.

@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.

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

HBelusca · 2024-05-14T15:31:52Z

win32ss/user/ntuser/message.c

@@ -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 };


Suggested change

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

HBelusca · 2024-05-15T17:12:52Z

win32ss/user/ntuser/message.c

+
+    for (i = 0; i < End; ++i)
+    {
+        if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0)


Why sizeof(StrUserKernel[0]) ? since the wcsncmp takes a number of characters (and not a number of bytes)

Suggested change

if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0)

if (wcsncmp(String, StrUserKernel[i], _countof(StrUserKernel[0])) == 0)

This only checks the first 4 chars of each string indeed.

@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.

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.

Changed sizeof to _countof. Thanks.

Right, it tries to use 20 for every string, even strings that are smaller.
Why is that used as max length?

HBelusca · 2024-05-15T17:15:36Z

win32ss/user/ntuser/message.c

+            _SEH2_TRY
+            {
+                if (UserModeMsg->lParam)
+                    RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));


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.

win32ss/user/ntuser/message.c

yagoulas · 2024-05-16T17:07:54Z

Are you sure that in windows the wparam is 0 in these cases? If I remember correctly most of the times when the win32k marshals an lparam pointer the wParam has inforumation about the size (when it can be variable like in a string). My suspicion is that windows would pass the lenght of the string as the wParam.

whindsaks · 2024-05-16T17:12:42Z

Are you sure that in windows the wparam is 0 in these cases?

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.

yagoulas · 2024-05-17T19:51:01Z

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:

Test if this message supports arbitrary strings in windows
Test how an ANSI window proc receives such a string sent with BroadcastSystemMessageExW
Test how a unicode window proc receives such a string sent with BroadcastSystemMessageExA (there are test cases like that in user32 tests)

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,

whindsaks · 2024-05-17T21:28:22Z

Test if this message supports arbitrary strings in windows

I can confirm that you can send any string you want (in the < WM_USER range the kernel knows how to marshal broadcast string WPARAM and LPARAMs for).

WNDPROC g;
void Append(HWND hWnd, LPCSTR s)
{
	SendMessage(hWnd, EM_SETSEL, 0, -1);
	SendMessage(hWnd, EM_SETSEL, -1, -1);
	SendMessageA(hWnd, EM_REPLACESEL, FALSE, (LPARAM)s);
}
LRESULT CALLBACK Sub(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM lParam)
{
	CHAR b[MAX_PATH];
	if (Msg == WM_APP)
	{
		wsprintfA(b, ("\r\nI'm=%u %hs\r\n"), GetCurrentProcessId(), IsWindowUnicode(hWnd) ? "Unicode" : "Ansi");
		Append(hWnd, b);
	}
	if (Msg == WM_SETTINGCHANGE && lParam) Append(hWnd, (LPSTR)lParam), Append(hWnd, ("\r\n"));
	return CallWindowProcA(g, hWnd, Msg, wParam, lParam);
}

void __cdecl main()
{
	HANDLE mutex = CreateMutexA(NULL, TRUE, "ROS.PR#6884");
	BOOL another = WaitForSingleObject(mutex, 0) != WAIT_OBJECT_0;
	UINT cw = 300, ch = 200, x = 0, y = 0;
	UINT style = WS_VISIBLE|WS_OVERLAPPEDWINDOW|ES_MULTILINE|ES_AUTOVSCROLL|ES_AUTOHSCROLL;
	HWND h = CreateWindowExA(0, ("EDIT"), ("ROS.PR#6884"), style, x, another ? y + ch : y, cw, ch, 0, 0, 0, 0);
	g = (WNDPROC)SetWindowLongPtrA(h, GWLP_WNDPROC, (LONG_PTR)Sub);
	SendMessage(h, WM_APP, 0, 0);
	if (!another)
	{
		WCHAR p[MAX_PATH];
		GetModuleFileNameW(NULL, p, MAX_PATH);
		ShellExecuteW(NULL, NULL, p, NULL, NULL, SW_SHOW);
		Sleep(2222); // Wait for the other guy to start
		BroadcastSystemMessageW(BSF_SENDNOTIFYMESSAGE, NULL, WM_SETTINGCHANGE, 0, (LPARAM)L"BSend WM_SETTINGCHANGE"); // W func -> A window
		BroadcastSystemMessageW(BSF_POSTMESSAGE      , NULL, WM_SETTINGCHANGE, 0, (LPARAM)L"BPost WM_SETTINGCHANGE"); // W func -> A window
		PostMessage(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)L"Post BROADCAST WM_SETTINGCHANGE"); // Does not work
		SendNotifyMessage(HWND_BROADCAST, WM_SETTINGCHANGE, 0, (LPARAM)L"Send BROADCAST WM_SETTINGCHANGE"); // W func -> A window
	}
	MessageBoxA(0, "cheap messageloop", another ? "2nd" : "first", 0);
}

Edit: And for WM_SETTINGCHANGE , it does not care if WPARAM is 0 or not, it will marshal the LPARAM string either way.

yagoulas · 2024-05-18T10:26:07Z

So here is my assumption what needs to be done:
user32!IntBroadcastSystemMessage: When BSF_POSTMESSAGE is missing, should do MsgiUMToKMMessage and MsgiUMToKMCleanup just like SendMessageW. When Ansi is true it should also do MsgiAnsiToUnicodeMessage and MsgiAnsiToUnicodeCleanup just like SendMessageCallbackA.

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!CopyMsgToKernelMem: should assume that messages that have the MMS_SIZE_LPARAMSZ flag, come with a UNICODE_STRING* in lparam. I guess after the string is copied into kernel memory we can keep passing the lparam as a WCHAR*

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?

whindsaks · 2024-05-18T10:31:30Z

When BSF_POSTMESSAGE is missing

And when BSF_POSTMESSAGE is present? It still needs to marshal the string. How Windows does this, I don't know.

yagoulas · 2024-05-18T12:06:20Z

When BSF_POSTMESSAGE is missing

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.

whindsaks · 2024-05-18T12:10:36Z

This is just a guess

But I already tested this on Windows and BSF_POSTMESSAGE does work when LPARAM is a string!

yagoulas · 2024-05-18T12:24:09Z

But I already tested this on Windows and BSF_POSTMESSAGE does work when LPARAM is a string!

even across processes?

whindsaks · 2024-05-18T12:31:29Z

even across processes?

Yes

learn-more · 2024-05-23T18:22:07Z

even across processes?

Yes

Educated guess, could it be adding atoms for this?
There is an atom viewer in my WindowsHookEx application:
https://learn-more.github.io/WindowsHookEx/

[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter

b87a335

CORE-19372 CORE-19583

github-actions bot added the Win32SS For Win32 subsystem (Win32k, GDI/USER DLLs, etc.) related components PRs. label May 14, 2024

HBelusca requested review from binarymaster, learn-more, HBelusca, katahiromz, cbialorucki, DarkFire01, oleg-dubinskiy and whindsaks May 14, 2024 14:59

oleg-dubinskiy approved these changes May 14, 2024

View reviewed changes

win32ss/user/ntuser/message.c Outdated Show resolved Hide resolved

HBelusca reviewed May 14, 2024

View reviewed changes

HBelusca self-requested a review May 14, 2024 15:32

HBelusca added the review pending For PRs undergoing review. label May 14, 2024

HBelusca reviewed May 15, 2024

View reviewed changes

learn-more reviewed May 16, 2024

View reviewed changes

win32ss/user/ntuser/message.c Outdated Show resolved Hide resolved

Doug-Lyons added 2 commits May 18, 2024 19:47

Changes based on reviewer comments.

d1a4bf8

Make sure that lParamMsg has a UNICODE_NULL in it.

52689f2

DarkFire01 approved these changes May 23, 2024

View reviewed changes

	WCHAR StrUserKernel[3][20] = {{L"intl"}, {L"Environment"}, {L"Policy"}};
	PCWSTR StrUserKernel[] = {L"intl", L"Environment", L"Policy"};

	RtlCopyMemory( lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));
	RtlCopyMemory(lParamMsg, (WCHAR*)UserModeMsg->lParam, sizeof(lParamMsg));

	WCHAR lParamMsg[sizeof(StrUserKernel[0])/2 + 1] = { 0 };
	WCHAR lParamMsg[_countof(StrUserKernel[0]) + 1] = { 0 };

	if (wcsncmp(String, StrUserKernel[i], sizeof(StrUserKernel[0])) == 0)
	if (wcsncmp(String, StrUserKernel[i], _countof(StrUserKernel[0])) == 0)

[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter #6884

Are you sure you want to change the base?

[NTUSER] Implement BroadcastSystemMessage with 'Environment' parameter #6884

Conversation

Doug-Lyons commented May 14, 2024 • edited by HBelusca

Purpose

Proposed changes

whindsaks commented May 14, 2024

oleg-dubinskiy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whindsaks May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doug-Lyons May 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doug-Lyons May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doug-Lyons May 16, 2024 • edited

Choose a reason for hiding this comment

Doug-Lyons May 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Doug-Lyons May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yagoulas commented May 16, 2024 • edited

whindsaks commented May 16, 2024

yagoulas commented May 17, 2024 • edited

whindsaks commented May 17, 2024 • edited

yagoulas commented May 18, 2024

whindsaks commented May 18, 2024

yagoulas commented May 18, 2024 • edited

whindsaks commented May 18, 2024

yagoulas commented May 18, 2024

whindsaks commented May 18, 2024 • edited

learn-more commented May 23, 2024

Doug-Lyons commented May 14, 2024 •

edited by HBelusca

whindsaks May 14, 2024 •

edited

Doug-Lyons May 19, 2024 •

edited

Doug-Lyons May 15, 2024 •

edited

Doug-Lyons May 16, 2024 •

edited

Doug-Lyons May 19, 2024 •

edited

Doug-Lyons May 16, 2024 •

edited

yagoulas commented May 16, 2024 •

edited

yagoulas commented May 17, 2024 •

edited

whindsaks commented May 17, 2024 •

edited

yagoulas commented May 18, 2024 •

edited

whindsaks commented May 18, 2024 •

edited