[ros-dev] [ros-diffs] [gadamopoulos] 51115: [ntoskrnl] - Implement calling OkayToCloseProcedure callouts to win32k for desktop and window station objects - Fix a bug that caused ObpCloseHandle to return success even whe...
Alex Ionescu
ionucu at videotron.ca
Tue Mar 22 14:19:05 UTC 2011
It's hilarious how this new code has the exact same Windows security bug I gave a talk about at BlackHat 2-3 years ago (which Microsoft fixed in Vista).
It's sad how this code ignores the exported PsSetProcessWindowStation API and relevant EPROCESS field.
It's awesome how nothing changes whenever I prop up to see the "progress".
--
Best regards,
Alex Ionescu
On 2011-03-22, at 5:19 AM, gadamopoulos at svn.reactos.org wrote:
> Author: gadamopoulos
> Date: Tue Mar 22 09:19:26 2011
> New Revision: 51115
>
> URL: http://svn.reactos.org/svn/reactos?rev=51115&view=rev
> Log:
> [ntoskrnl]
> - Implement calling OkayToCloseProcedure callouts to win32k for desktop and window station objects
> - Fix a bug that caused ObpCloseHandle to return success even when OkayToCloseProcedure failed
>
> [win32k]
> - Rewrite SetProcessWindowStation to actually set the current window station and close the previous one
> - Implement OkayToCloseProcedure callouts from the kernel to prevent closing the current desktop or window station
>
> Modified:
> trunk/reactos/ntoskrnl/ex/win32k.c
> trunk/reactos/ntoskrnl/ob/obhandle.c
> trunk/reactos/ntoskrnl/ps/win32.c
> trunk/reactos/subsystems/win32/win32k/include/desktop.h
> trunk/reactos/subsystems/win32/win32k/include/win32.h
> trunk/reactos/subsystems/win32/win32k/include/winsta.h
> trunk/reactos/subsystems/win32/win32k/main/dllmain.c
> trunk/reactos/subsystems/win32/win32k/ntuser/desktop.c
> trunk/reactos/subsystems/win32/win32k/ntuser/winsta.c
>
> Modified: trunk/reactos/ntoskrnl/ex/win32k.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/win32k.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/ntoskrnl/ex/win32k.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/ex/win32k.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -37,9 +37,45 @@
>
> PKWIN32_PARSEMETHOD_CALLOUT ExpWindowStationObjectParse = NULL;
> PKWIN32_DELETEMETHOD_CALLOUT ExpWindowStationObjectDelete = NULL;
> +PKWIN32_OKTOCLOSEMETHOD_CALLOUT ExpWindowStationObjectOkToClose = NULL;
> +PKWIN32_OKTOCLOSEMETHOD_CALLOUT ExpDesktopObjectOkToClose = NULL;
> PKWIN32_DELETEMETHOD_CALLOUT ExpDesktopObjectDelete = NULL;
>
> /* FUNCTIONS ****************************************************************/
> +
> +NTSTATUS
> +NTAPI
> +ExpDesktopOkToClose( IN PEPROCESS Process OPTIONAL,
> + IN PVOID Object,
> + IN HANDLE Handle,
> + IN KPROCESSOR_MODE AccessMode)
> +{
> + WIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters;
> +
> + Parameters.Process = Process;
> + Parameters.Object = Object;
> + Parameters.Handle = Handle;
> + Parameters.PreviousMode = AccessMode;
> +
> + return ExpDesktopObjectOkToClose(&Parameters);
> +}
> +
> +NTSTATUS
> +NTAPI
> +ExpWindowStationOkToClose( IN PEPROCESS Process OPTIONAL,
> + IN PVOID Object,
> + IN HANDLE Handle,
> + IN KPROCESSOR_MODE AccessMode)
> +{
> + WIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters;
> +
> + Parameters.Process = Process;
> + Parameters.Object = Object;
> + Parameters.Handle = Handle;
> + Parameters.PreviousMode = AccessMode;
> +
> + return ExpWindowStationObjectOkToClose(&Parameters);
> +}
>
> VOID
> NTAPI
> @@ -114,6 +150,7 @@
> ObjectTypeInitializer.PoolType = NonPagedPool;
> ObjectTypeInitializer.DeleteProcedure = ExpWinStaObjectDelete;
> ObjectTypeInitializer.ParseProcedure = ExpWinStaObjectParse;
> + ObjectTypeInitializer.OkayToCloseProcedure = ExpWindowStationOkToClose;
> ObCreateObjectType(&Name,
> &ObjectTypeInitializer,
> NULL,
> @@ -124,6 +161,7 @@
> ObjectTypeInitializer.GenericMapping = ExpDesktopMapping;
> ObjectTypeInitializer.DeleteProcedure = ExpDesktopDelete;
> ObjectTypeInitializer.ParseProcedure = NULL;
> + ObjectTypeInitializer.OkayToCloseProcedure = ExpDesktopOkToClose;
> ObCreateObjectType(&Name,
> &ObjectTypeInitializer,
> NULL,
>
> Modified: trunk/reactos/ntoskrnl/ob/obhandle.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ob/obhandle.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/ntoskrnl/ob/obhandle.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/ob/obhandle.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -1752,7 +1752,6 @@
>
> /* Detach and return success */
> if (AttachedToProcess) KeUnstackDetachProcess(&ApcState);
> - Status = STATUS_SUCCESS;
> }
> else
> {
>
> Modified: trunk/reactos/ntoskrnl/ps/win32.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ps/win32.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/ntoskrnl/ps/win32.c [iso-8859-1] (original)
> +++ trunk/reactos/ntoskrnl/ps/win32.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -20,6 +20,8 @@
> PGDI_BATCHFLUSH_ROUTINE KeGdiFlushUserBatch = NULL;
> extern PKWIN32_PARSEMETHOD_CALLOUT ExpWindowStationObjectParse;
> extern PKWIN32_DELETEMETHOD_CALLOUT ExpWindowStationObjectDelete;
> +extern PKWIN32_OKTOCLOSEMETHOD_CALLOUT ExpWindowStationObjectOkToClose;
> +extern PKWIN32_OKTOCLOSEMETHOD_CALLOUT ExpDesktopObjectOkToClose;
> extern PKWIN32_DELETEMETHOD_CALLOUT ExpDesktopObjectDelete;
> extern PKWIN32_POWEREVENT_CALLOUT PopEventCallout;
>
> @@ -116,6 +118,8 @@
> PspW32ThreadCallout = CalloutData->ThreadCallout;
> ExpWindowStationObjectParse = CalloutData->WindowStationParseProcedure;
> ExpWindowStationObjectDelete = CalloutData->WindowStationDeleteProcedure;
> + ExpWindowStationObjectOkToClose = CalloutData->WindowStationOkToCloseProcedure;
> + ExpDesktopObjectOkToClose = CalloutData->DesktopOkToCloseProcedure;
> ExpDesktopObjectDelete = CalloutData->DesktopDeleteProcedure;
> PopEventCallout = CalloutData->PowerEventCallout;
> KeGdiFlushUserBatch = CalloutData->BatchFlushRoutine;
>
> Modified: trunk/reactos/subsystems/win32/win32k/include/desktop.h
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/include/desktop.h?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/include/desktop.h [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/include/desktop.h [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -69,6 +69,9 @@
> VOID APIENTRY
> IntDesktopObjectDelete(PWIN32_DELETEMETHOD_PARAMETERS Parameters);
>
> +NTSTATUS NTAPI
> +IntDesktopOkToClose(PWIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters);
> +
> LRESULT CALLBACK
> IntDesktopWindowProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam);
>
>
> Modified: trunk/reactos/subsystems/win32/win32k/include/win32.h
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/include/win32.h?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/include/win32.h [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/include/win32.h [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -166,6 +166,7 @@
> PCLS pclsPrivateList;
> PCLS pclsPublicList;
> INT cThreads;
> + HDESK hdeskStartup;
> DWORD dwhmodLibLoadedMask;
> HANDLE ahmodLibLoaded[CLIBS];
> struct _WINSTATION_OBJECT *prpwinsta;
>
> Modified: trunk/reactos/subsystems/win32/win32k/include/winsta.h
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/include/winsta.h?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/include/winsta.h [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/include/winsta.h [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -82,6 +82,9 @@
> APIENTRY
> IntWinStaObjectParse(PWIN32_PARSEMETHOD_PARAMETERS Parameters);
>
> +NTSTATUS NTAPI
> +IntWinstaOkToClose(PWIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters);
> +
> NTSTATUS FASTCALL
> IntValidateWindowStationHandle(
> HWINSTA WindowStation,
> @@ -106,4 +109,7 @@
>
> PWINSTATION_OBJECT FASTCALL IntGetWinStaObj(VOID);
>
> +BOOL FASTCALL
> +UserSetProcessWindowStation(HWINSTA hWindowStation);
> +
> /* EOF */
>
> Modified: trunk/reactos/subsystems/win32/win32k/main/dllmain.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/main/dllmain.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/main/dllmain.c [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/main/dllmain.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -119,6 +119,7 @@
> {
> DPRINT("Destroying W32 process PID:%d at IRQ level: %lu\n", Process->UniqueProcessId, KeGetCurrentIrql());
> Win32Process->W32PF_flags |= W32PF_TERMINATED;
> +
> if (Win32Process->InputIdleEvent)
> {
> EngFreeMem((PVOID)Win32Process->InputIdleEvent);
> @@ -144,6 +145,9 @@
> {
> LogonProcess = NULL;
> }
> +
> + UserSetProcessWindowStation(NULL);
> +
> }
>
> RETURN( STATUS_SUCCESS);
> @@ -220,25 +224,14 @@
> {
> if(hWinSta != NULL)
> {
> - if(Process != CsrProcess)
> + if(!UserSetProcessWindowStation(hWinSta))
> {
> - HWINSTA hProcessWinSta = (HWINSTA)InterlockedCompareExchangePointer((PVOID)&Process->Win32WindowStation, (PVOID)hWinSta, NULL);
> - if(hProcessWinSta != NULL)
> - {
> - /* our process is already assigned to a different window station, we don't need the handle anymore */
> - NtClose(hWinSta);
> - }
> - }
> - else
> - {
> - NtClose(hWinSta);
> + DPRINT1("Failed to set process window station\n");
> }
> }
>
> if (hDesk != NULL)
> {
> - Win32Thread->rpdesk = NULL;
> - Win32Thread->hdesk = NULL;
> if (!IntSetThreadDesktop(hDesk, FALSE))
> {
> DPRINT1("Unable to set thread desktop\n");
> @@ -441,6 +434,8 @@
> CalloutData.ProcessCallout = Win32kProcessCallback;
> CalloutData.ThreadCallout = Win32kThreadCallback;
> CalloutData.BatchFlushRoutine = NtGdiFlushUserBatch;
> + CalloutData.DesktopOkToCloseProcedure = IntDesktopOkToClose;
> + CalloutData.WindowStationOkToCloseProcedure = IntWinstaOkToClose;
>
> /* Register our per-process and per-thread structures. */
> PsEstablishWin32Callouts((PWIN32_CALLOUTS_FPNS)&CalloutData);
>
> Modified: trunk/reactos/subsystems/win32/win32k/ntuser/desktop.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntuser/desktop.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/ntuser/desktop.c [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/ntuser/desktop.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -166,6 +166,29 @@
> RemoveEntryList(&Desktop->ListEntry);
>
> IntFreeDesktopHeap(Desktop);
> +}
> +
> +NTSTATUS NTAPI
> +IntDesktopOkToClose(PWIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters)
> +{
> + PTHREADINFO pti;
> +
> + pti = PsGetCurrentThreadWin32Thread();
> +
> + if( pti == NULL)
> + {
> + /* This happens when we leak desktop handles */
> + return TRUE;
> + }
> +
> + /* Do not allow the current desktop or the initial desktop to be closed */
> + if( Parameters->Handle == pti->ppi->hdeskStartup ||
> + Parameters->Handle == pti->hdesk)
> + {
> + return FALSE;
> + }
> +
> + return TRUE;
> }
>
> /* PRIVATE FUNCTIONS **********************************************************/
>
> Modified: trunk/reactos/subsystems/win32/win32k/ntuser/winsta.c
> URL: http://svn.reactos.org/svn/reactos/trunk/reactos/subsystems/win32/win32k/ntuser/winsta.c?rev=51115&r1=51114&r2=51115&view=diff
> ==============================================================================
> --- trunk/reactos/subsystems/win32/win32k/ntuser/winsta.c [iso-8859-1] (original)
> +++ trunk/reactos/subsystems/win32/win32k/ntuser/winsta.c [iso-8859-1] Tue Mar 22 09:19:26 2011
> @@ -187,6 +187,21 @@
> return STATUS_OBJECT_TYPE_MISMATCH;
> }
>
> +NTSTATUS NTAPI
> +IntWinstaOkToClose(PWIN32_OKAYTOCLOSEMETHOD_PARAMETERS Parameters)
> +{
> + PPROCESSINFO ppi;
> +
> + ppi = PsGetCurrentProcessWin32Process();
> +
> + if(Parameters->Handle == ppi->hwinsta)
> + {
> + return FALSE;
> + }
> +
> + return TRUE;
> +}
> +
> /* PRIVATE FUNCTIONS **********************************************************/
>
> /*
> @@ -915,6 +930,57 @@
> return WinStaObj;
> }
>
> +BOOL FASTCALL
> +UserSetProcessWindowStation(HWINSTA hWindowStation)
> +{
> + PPROCESSINFO ppi;
> + NTSTATUS Status;
> + HWINSTA hwinstaOld;
> + PWINSTATION_OBJECT NewWinSta = NULL, OldWinSta;
> +
> + ppi = PsGetCurrentProcessWin32Process();
> +
> + if(hWindowStation !=NULL)
> + {
> + Status = IntValidateWindowStationHandle( hWindowStation,
> + KernelMode,
> + 0,
> + &NewWinSta);
> + if (!NT_SUCCESS(Status))
> + {
> + DPRINT("Validation of window station handle (0x%X) failed\n",
> + hWindowStation);
> + SetLastNtError(Status);
> + return FALSE;
> + }
> + }
> +
> + OldWinSta = ppi->prpwinsta;
> + hwinstaOld = ppi->hwinsta;
> +
> + /*
> + * FIXME - don't allow changing the window station if there are threads that are attached to desktops and own gui objects
> + */
> +
> + InterlockedExchangePointer(&PsGetCurrentProcess()->Win32WindowStation, hWindowStation);
> +
> + ppi->prpwinsta = NewWinSta;
> + ppi->hwinsta = hWindowStation;
> +
> +
> + if(OldWinSta != NULL)
> + {
> + ObDereferenceObject(OldWinSta);
> + }
> +
> + if(hwinstaOld != NULL)
> + {
> + ZwClose(hwinstaOld);
> + }
> +
> + return TRUE;
> +}
> +
> /*
> * NtUserSetProcessWindowStation
> *
> @@ -934,44 +1000,15 @@
> BOOL APIENTRY
> NtUserSetProcessWindowStation(HWINSTA hWindowStation)
> {
> - PWINSTATION_OBJECT NewWinSta;
> - NTSTATUS Status;
> -
> - DPRINT("About to set process window station with handle (0x%X)\n",
> - hWindowStation);
> -
> - if(PsGetCurrentProcess() == CsrProcess)
> - {
> - DPRINT1("CSRSS is not allowed to change it's window station!!!\n");
> - EngSetLastError(ERROR_ACCESS_DENIED);
> - return FALSE;
> - }
> -
> - Status = IntValidateWindowStationHandle(
> - hWindowStation,
> - KernelMode,
> - 0,
> - &NewWinSta);
> -
> - if (!NT_SUCCESS(Status))
> - {
> - DPRINT("Validation of window station handle (0x%X) failed\n",
> - hWindowStation);
> - SetLastNtError(Status);
> - return FALSE;
> - }
> -
> - /*
> - * FIXME - don't allow changing the window station if there are threads that are attached to desktops and own gui objects
> - */
> -
> - /* FIXME - dereference the old window station, etc... */
> - InterlockedExchangePointer(&PsGetCurrentProcess()->Win32WindowStation, hWindowStation);
> -
> - DPRINT("PsGetCurrentProcess()->Win32WindowStation 0x%X\n",
> - PsGetCurrentProcess()->Win32WindowStation);
> -
> - return TRUE;
> + BOOL ret;
> +
> + UserEnterExclusive();
> +
> + ret = UserSetProcessWindowStation(hWindowStation);
> +
> + UserLeave();
> +
> + return ret;
> }
>
> /*
>
>
More information about the Ros-dev
mailing list