[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=29... ============================================================================== --- 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=293... ============================================================================== --- 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);
-
fireball＠svn.reactos.org