[ros-diffs] [fireball] 29304: - ObReferenceObjectByHandle/ObpReferenceProcessByHandle: Properly return STATUS_INVALID_HANDLE if user-mode tries to reference a kernel-mode handle. - ObReferenceObjectByHandle: Properly validate process/thread access rights before giving a reference to the caller. - Fix definition of "SizeOfHandle" macro in the handle table implementation. Fixes handle leaks at process rundown, handle allocation, and problems with processes that use more than 512 handles. - Remove checks for "

Author: fireball
Date: Sun Sep 30 12:32:34 2007
New Revision: 29304

URL: http://svn.reactos.org/svn/reactos?rev=29304&view=rev
Log:
- ObReferenceObjectByHandle/ObpReferenceProcessByHandle: Properly
return STATUS_INVALID_HANDLE if user-mode tries to reference a
kernel-mode handle.
- ObReferenceObjectByHandle: Properly validate process/thread access
rights before giving a reference to the caller.
- Fix definition of "SizeOfHandle" macro in the handle table
implementation. Fixes handle leaks at process rundown, handle
allocation, and problems with processes that use more than 512
handles.
- Remove checks for "VALID_INHERIT_FLAGS". These flags have nothing to
do with handle table entries and shouldn't appear in them. Please fix
callers if they're attempting to send inherit flags as access masks.
- Thanks to Alex! :)

Modified:
    trunk/reactos/ntoskrnl/ex/handle.c   (contents, props changed)
    trunk/reactos/ntoskrnl/ob/obhandle.c
    trunk/reactos/ntoskrnl/ob/obref.c

Modified: trunk/reactos/ntoskrnl/ex/handle.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/handle.c?rev=2…
==============================================================================
--- trunk/reactos/ntoskrnl/ex/handle.c (original)
+++ trunk/reactos/ntoskrnl/ex/handle.c Sun Sep 30 12:32:34 2007
@@ -17,7 +17,7 @@
 
 LIST_ENTRY HandleTableListHead;
 EX_PUSH_LOCK HandleTableListLock;
-#define SizeOfHandle(x) sizeof(HANDLE) * x
+#define SizeOfHandle(x) (sizeof(HANDLE) * x)
 
 /* PRIVATE FUNCTIONS *********************************************************/
 

Propchange: trunk/reactos/ntoskrnl/ex/handle.c
------------------------------------------------------------------------------
--- svn:needs-lock (original)
+++ svn:needs-lock (removed)
@@ -1,1 +1,0 @@
-*

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 (original)
+++ trunk/reactos/ntoskrnl/ob/obhandle.c Sun Sep 30 12:32:34 2007
@@ -73,55 +73,64 @@
 
     /* Assume failure */
     *Object = NULL;
-
-    /* Check if the caller wants the current process */
-    if (Handle == NtCurrentProcess())
-    {
-        /* Return handle info */
-        HandleInformation->HandleAttributes = 0;
-        HandleInformation->GrantedAccess = Process->GrantedAccess;
-
-        /* No audit mask */
-        *AuditMask = 0;
-
-        /* Reference ourselves */
-        ObjectHeader = OBJECT_TO_OBJECT_HEADER(Process);
-        InterlockedIncrement(&ObjectHeader->PointerCount);
-
-        /* Return the pointer */
-        *Object = Process;
-        ASSERT(*Object != NULL);
-        return STATUS_SUCCESS;
-    }
-
-    /* Check if the caller wants the current thread */
-    if (Handle == NtCurrentThread())
-    {
-        /* Return handle information */
-        HandleInformation->HandleAttributes = 0;
-        HandleInformation->GrantedAccess = Thread->GrantedAccess;
-
-        /* Reference ourselves */
-        ObjectHeader = OBJECT_TO_OBJECT_HEADER(Thread);
-        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
-
-        /* No audit mask */
-        *AuditMask = 0;
-
-        /* Return the pointer */
-        *Object = Thread;
-        ASSERT(*Object != NULL);
-        return STATUS_SUCCESS;
-    }
-
-    /* Check if this is a kernel handle */
-    if (ObIsKernelHandle(Handle, AccessMode))
-    {
-        /* Use the kernel handle table and get the actual handle value */
-        Handle = ObKernelHandleToHandle(Handle);
-        HandleTable = ObpKernelHandleTable;
-    }
-
+    
+    /* Check if this is a special handle */
+    if (HandleToLong(Handle) < 0)
+    {
+        /* Check if the caller wants the current process */
+        if (Handle == NtCurrentProcess())
+        {
+            /* Return handle info */
+            HandleInformation->HandleAttributes = 0;
+            HandleInformation->GrantedAccess = Process->GrantedAccess;
+            
+            /* No audit mask */
+            *AuditMask = 0;
+            
+            /* Reference ourselves */
+            ObjectHeader = OBJECT_TO_OBJECT_HEADER(Process);
+            InterlockedIncrement(&ObjectHeader->PointerCount);
+            
+            /* Return the pointer */
+            *Object = Process;
+            ASSERT(*Object != NULL);
+            return STATUS_SUCCESS;
+        }
+        
+        /* Check if the caller wants the current thread */
+        if (Handle == NtCurrentThread())
+        {
+            /* Return handle information */
+            HandleInformation->HandleAttributes = 0;
+            HandleInformation->GrantedAccess = Thread->GrantedAccess;
+            
+            /* Reference ourselves */
+            ObjectHeader = OBJECT_TO_OBJECT_HEADER(Thread);
+            InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+            
+            /* No audit mask */
+            *AuditMask = 0;
+            
+            /* Return the pointer */
+            *Object = Thread;
+            ASSERT(*Object != NULL);
+            return STATUS_SUCCESS;
+        }
+        
+        /* This is a kernel handle... do we have access? */
+        if (AccessMode == KernelMode)
+        {
+            /* Use the kernel handle table and get the actual handle value */
+            Handle = ObKernelHandleToHandle(Handle);
+            HandleTable = ObpKernelHandleTable;
+        }
+        else
+        {
+            /* This is an illegal attempt to access a kernel handle */
+            return STATUS_INVALID_HANDLE;
+        }
+    }
+        
     /* Enter a critical region while we touch the handle table */
     ASSERT(HandleTable != NULL);
     KeEnterCriticalRegion();
@@ -2053,12 +2062,7 @@
     ExSweepHandleTable(HandleTable,
                        ObpCloseHandleCallback,
                        &Context);
-    //ASSERT(HandleTable->HandleCount == 0);
-    /* HACK: Until the problem is investigated... */
-    if (HandleTable->HandleCount != 0)
-    {
-        DPRINT1("Leaking %d handles!\n", HandleTable->HandleCount);
-    }
+    ASSERT(HandleTable->HandleCount == 0);
 
     /* Leave the critical region */
     KeLeaveCriticalRegion();

Modified: trunk/reactos/ntoskrnl/ob/obref.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ob/obref.c?rev=29…
==============================================================================
--- trunk/reactos/ntoskrnl/ob/obref.c (original)
+++ trunk/reactos/ntoskrnl/ob/obref.c Sun Sep 30 12:32:34 2007
@@ -480,70 +480,111 @@
     /* Assume failure */
     *Object = NULL;
 
-    /* Check if the caller wants the current process */
-    if ((Handle == NtCurrentProcess()) &&
-        ((ObjectType == PsProcessType) || !(ObjectType)))
-    {
-        /* Get the current process */
-        CurrentProcess = PsGetCurrentProcess();
-
-        /* Check if the caller wanted handle information */
-        if (HandleInformation)
-        {
-            /* Return it */
-            HandleInformation->HandleAttributes = 0;
-            HandleInformation->GrantedAccess = PROCESS_ALL_ACCESS;
-        }
-
-        /* Reference ourselves */
-        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentProcess);
-        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
-
-        /* Return the pointer */
-        *Object = CurrentProcess;
-        return STATUS_SUCCESS;
-    }
-    else if (Handle == NtCurrentProcess())
-    {
-        /* The caller used this special handle value with a non-process type */
-        return STATUS_OBJECT_TYPE_MISMATCH;
-    }
-
-    /* Check if the caller wants the current thread */
-    if ((Handle == NtCurrentThread()) &&
-        ((ObjectType == PsThreadType) || !(ObjectType)))
-    {
-        /* Get the current thread */
-        CurrentThread = PsGetCurrentThread();
-
-        /* Check if the caller wanted handle information */
-        if (HandleInformation)
-        {
-            /* Return it */
-            HandleInformation->HandleAttributes = 0;
-            HandleInformation->GrantedAccess = THREAD_ALL_ACCESS;
-        }
-
-        /* Reference ourselves */
-        ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentThread);
-        InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
-
-        /* Return the pointer */
-        *Object = CurrentThread;
-        return STATUS_SUCCESS;
-    }
-    else if (Handle == NtCurrentThread())
-    {
-        /* The caller used this special handle value with a non-thread type */
-        return STATUS_OBJECT_TYPE_MISMATCH;
-    }
-
-    /* Check if this is a kernel handle */
-    if (ObIsKernelHandle(Handle, AccessMode))
-    {
-        /* Use the kernel handle table and get the actual handle value */
-        Handle = ObKernelHandleToHandle(Handle);
-        HandleTable = ObpKernelHandleTable;
+    /* Check if this is a special handle */
+    if (HandleToLong(Handle) < 0)
+    {
+        /* Check if this is the current process */
+        if (Handle == NtCurrentProcess())
+        {
+            /* Check if this is the right object type */
+            if ((ObjectType == PsProcessType) || !(ObjectType))
+            {
+                /* Get the current process and granted access */
+                CurrentProcess = PsGetCurrentProcess();
+                GrantedAccess = CurrentProcess->GrantedAccess;
+                
+                /* Validate access */
+                if ((AccessMode == KernelMode) ||
+                    !(~GrantedAccess & DesiredAccess))
+                {                   
+                    /* Check if the caller wanted handle information */
+                    if (HandleInformation)
+                    {
+                        /* Return it */
+                        HandleInformation->HandleAttributes = 0;
+                        HandleInformation->GrantedAccess = GrantedAccess;
+                    }
+                    
+                    /* Reference ourselves */
+                    ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentProcess);
+                    InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+                    
+                    /* Return the pointer */
+                    *Object = CurrentProcess;
+                    ASSERT(*Object != NULL);
+                    Status = STATUS_SUCCESS;
+                }
+                else
+                {
+                    /* Access denied */
+                    Status = STATUS_ACCESS_DENIED;
+                }
+            }
+            else
+            {
+                /* The caller used this special handle value with a non-process type */
+                Status = STATUS_OBJECT_TYPE_MISMATCH;
+            }
+            
+            /* Return the status */
+            return Status;
+        }
+        else if (Handle == NtCurrentThread())
+        {
+            /* Check if this is the right object type */
+            if ((ObjectType == PsThreadType) || !(ObjectType))
+            {
+                /* Get the current process and granted access */
+                CurrentThread = PsGetCurrentThread();
+                GrantedAccess = CurrentThread->GrantedAccess;
+                
+                /* Validate access */
+                if ((AccessMode == KernelMode) ||
+                    !(~GrantedAccess & DesiredAccess))
+                {                   
+                    /* Check if the caller wanted handle information */
+                    if (HandleInformation)
+                    {
+                        /* Return it */
+                        HandleInformation->HandleAttributes = 0;
+                        HandleInformation->GrantedAccess = GrantedAccess;
+                    }
+                    
+                    /* Reference ourselves */
+                    ObjectHeader = OBJECT_TO_OBJECT_HEADER(CurrentThread);
+                    InterlockedExchangeAdd(&ObjectHeader->PointerCount, 1);
+                    
+                    /* Return the pointer */
+                    *Object = CurrentThread;
+                    ASSERT(*Object != NULL);
+                    Status = STATUS_SUCCESS;
+                }
+                else
+                {
+                    /* Access denied */
+                    Status = STATUS_ACCESS_DENIED;
+                }
+            }
+            else
+            {
+                /* The caller used this special handle value with a non-process type */
+                Status = STATUS_OBJECT_TYPE_MISMATCH;
+            }
+            
+            /* Return the status */
+            return Status;
+        }
+        else if (AccessMode == KernelMode)
+        {
+            /* Use the kernel handle table and get the actual handle value */
+            Handle = ObKernelHandleToHandle(Handle);
+            HandleTable = ObpKernelHandleTable;
+        }
+        else
+        {
+            /* Invalid access, fail */
+            return STATUS_INVALID_HANDLE;
+        }
     }
     else
     {
@@ -565,11 +606,10 @@
         {
             /* Get the granted access and validate it */
             GrantedAccess = HandleEntry->GrantedAccess;
-            /* Inherit flags are not a kind of access right, they indicate 
-             * the disposition of access rights.  Therefore, they should not
-             * be considered. */
+            
+            /* Validate access */
             if ((AccessMode == KernelMode) ||
-                !((~GrantedAccess & DesiredAccess & ~VALID_INHERIT_FLAGS)))
+                !(~GrantedAccess & DesiredAccess))
             {
                 /* Reference the object directly since we have its header */
                 InterlockedIncrement(&ObjectHeader->PointerCount);