Re: [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]

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, [e-mail] 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=51...
> ==============================================================================
> --- 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=...
> ==============================================================================
> --- 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=511...
> ==============================================================================
> --- 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/inc...
> ==============================================================================
> --- 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/inc...
> ==============================================================================
> --- 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/inc...
> ==============================================================================
> --- 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/mai...
> ==============================================================================
> --- 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/ntu...
> ==============================================================================
> --- 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/ntu...
> ==============================================================================
> --- 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;
>
> }
>
> /*