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, gadamopou...@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; > } > > /* > > _______________________________________________ Ros-dev mailing list Ros-dev@reactos.org http://www.reactos.org/mailman/listinfo/ros-dev
