Re: [ros-dev] [ros-diffs] [dgorbachev] 41610: Remove a hack from NtAccessCheck(). Bug #4169.

[Alex Ionescu]

Oh, I see. All these white spaces were hacks.

Thanks for mixing in 500 whitespace and formatting changes with 5
lines of code changes. It makes it really clear!

Has the kernel become a no-man's land of garbage? I'm thinking of
removing my name from the sources if this keeps up.

Best regards,
Alex Ionescu



On Thu, Jun 25, 2009 at 6:29 AM, <dgorbachev(a)svn.reactos.org> wrote:
>   Author: dgorbachev
>  Date: Thu Jun 25 17:29:58 2009
>  New Revision: 41610
>
>  URL: http://svn.reactos.org/svn/reactos?rev=41610&view=rev
>  Log:
>  Remove a hack from NtAccessCheck(). Bug #4169.
>
>  Modified:
>     trunk/reactos/ntoskrnl/se/semgr.c
>
>  Modified: trunk/reactos/ntoskrnl/se/semgr.c
>  URL:
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/se/semgr.c?rev=41…
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] (original)
>  +++ trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] Thu Jun 25 17:29:58 2009
>  @@ -49,7 +49,7 @@
>      SepExports.SeSystemEnvironmentPrivilege = SeSystemEnvironmentPrivilege;
>      SepExports.SeChangeNotifyPrivilege = SeChangeNotifyPrivilege;
>      SepExports.SeRemoteShutdownPrivilege = SeRemoteShutdownPrivilege;
>  -
>  +
>      SepExports.SeNullSid = SeNullSid;
>      SepExports.SeWorldSid = SeWorldSid;
>      SepExports.SeLocalSid = SeLocalSid;
>  @@ -72,11 +72,11 @@
>      SepExports.SeAuthenticatedUsersSid = SeAuthenticatedUsersSid;
>      SepExports.SeRestrictedSid = SeRestrictedSid;
>      SepExports.SeAnonymousLogonSid = SeAnonymousLogonSid;
>  -
>  +
>      SepExports.SeUndockPrivilege = SeUndockPrivilege;
>      SepExports.SeSyncAgentPrivilege = SeSyncAgentPrivilege;
>      SepExports.SeEnableDelegationPrivilege = SeEnableDelegationPrivilege;
>  -
>  +
>      SeExports = &SepExports;
>      return TRUE;
>   }
>  @@ -92,18 +92,18 @@
>      if (!SepInitSDs()) return FALSE;
>      SepInitPrivileges();
>      if (!SepInitExports()) return FALSE;
>  -
>  +
>      /* Initialize the subject context lock */
>      ExInitializeResource(&SepSubjectContextLock);
>  -
>  +
>      /* Initialize token objects */
>      SepInitializeTokenImplementation();
>  -
>  +
>      /* Clear impersonation info for the idle thread */
>      PsGetCurrentThread()->ImpersonationInfo = NULL;
>      PspClearCrossThreadFlag(PsGetCurrentThread(),
>                              CT_ACTIVE_IMPERSONATION_INFO_BIT);
>  -
>  +
>      /* Initialize the boot token */
>      ObInitializeFastReference(&PsGetCurrentProcess()->Token, NULL);
>      ObInitializeFastReference(&PsGetCurrentProcess()->Token,
>  @@ -117,7 +117,7 @@
>   {
>      NTSTATUS Status;
>      PAGED_CODE();
>  -
>  +
>      /* Insert the system token into the tree */
>      Status = ObInsertObject((PVOID)(PsGetCurrentProcess()->Token.Value &
>                                      ~MAX_FAST_REFS),
>  @@ -127,7 +127,7 @@
>                              NULL,
>                              NULL);
>      ASSERT(NT_SUCCESS(Status));
>  -
>  +
>      /* FIXME: TODO \\ Security directory */
>      return TRUE;
>   }
>  @@ -140,17 +140,17 @@
>      switch (ExpInitializationPhase)
>      {
>          case 0:
>  -
>  +
>              /* Do Phase 0 */
>              return SepInitializationPhase0();
>  -
>  +
>          case 1:
>  -
>  +
>              /* Do Phase 1 */
>              return SepInitializationPhase1();
>  -
>  +
>          default:
>  -
>  +
>              /* Don't know any other phase! Bugcheck! */
>              KeBugCheckEx(UNEXPECTED_INITIALIZATION_CALL,
>                           0,
>  @@ -170,7 +170,7 @@
>      HANDLE DirectoryHandle;
>      HANDLE EventHandle;
>      NTSTATUS Status;
>  -
>  +
>      /* Create '\Security' directory */
>      RtlInitUnicodeString(&Name,
>                           L"\\Security");
>  @@ -187,7 +187,7 @@
>          DPRINT1("Failed to create 'Security' directory!\n");
>          return FALSE;
>      }
>  -
>  +
>      /* Create 'LSA_AUTHENTICATION_INITALIZED' event */
>      RtlInitUnicodeString(&Name,
>                           L"\\LSA_AUTHENTICATION_INITALIZED");
>  @@ -207,12 +207,12 @@
>          NtClose(DirectoryHandle);
>          return FALSE;
>      }
>  -
>  +
>      ZwClose(EventHandle);
>      ZwClose(DirectoryHandle);
>  -
>  +
>      /* FIXME: Create SRM port and listener thread */
>  -
>  +
>      return TRUE;
>   }
>
>  @@ -228,16 +228,16 @@
>                        IN PGENERIC_MAPPING GenericMapping)
>   {
>      PAGED_CODE();
>  -
>  +
>      /* Select the operation type */
>      switch (OperationType)
>      {
>              /* Setting a new descriptor */
>          case SetSecurityDescriptor:
>  -
>  +
>              /* Sanity check */
>              ASSERT((PoolType == PagedPool) || (PoolType == NonPagedPool));
>  -
>  +
>              /* Set the information */
>              return ObSetSecurityDescriptorInfo(Object,
>                                                 SecurityInformation,
>  @@ -245,33 +245,33 @@
>                                                 OldSecurityDescriptor,
>                                                 PoolType,
>                                                 GenericMapping);
>  -
>  +
>          case QuerySecurityDescriptor:
>  -
>  +
>              /* Query the information */
>              return ObQuerySecurityDescriptorInfo(Object,
>                                                   SecurityInformation,
>                                                   SecurityDescriptor,
>                                                   ReturnLength,
>                                                   OldSecurityDescriptor);
>  -
>  +
>          case DeleteSecurityDescriptor:
>  -
>  +
>              /* De-assign it */
>              return ObDeassignSecurity(OldSecurityDescriptor);
>  -
>  +
>          case AssignSecurityDescriptor:
>  -
>  +
>              /* Assign it */
>              ObAssignObjectSecurityDescriptor(Object, SecurityDescriptor, PoolType);
>              return STATUS_SUCCESS;
>  -
>  +
>          default:
>  -
>  +
>              /* Bug check */
>              KeBugCheckEx(SECURITY_SYSTEM, 0, STATUS_INVALID_PARAMETER, 0, 0);
>      }
>  -
>  +
>      /* Should never reach here */
>      ASSERT(FALSE);
>      return STATUS_SUCCESS;
>  @@ -284,14 +284,14 @@
>   {
>      ULONG i;
>      PTOKEN Token = (PTOKEN)_Token;
>  -
>  +
>      PAGED_CODE();
>  -
>  +
>      if (Token->UserAndGroupCount == 0)
>      {
>          return FALSE;
>      }
>  -
>  +
>      for (i=0; i<Token->UserAndGroupCount; i++)
>      {
>          if (RtlEqualSid(Sid, Token->UserAndGroups[i].Sid))
>  @@ -300,11 +300,11 @@
>              {
>                  return TRUE;
>              }
>  -
>  +
>              return FALSE;
>          }
>      }
>  -
>  +
>      return FALSE;
>   }
>
>  @@ -314,7 +314,7 @@
>                            OUT PACCESS_MASK DesiredAccess)
>   {
>      *DesiredAccess = 0;
>  -
>  +
>      if (SecurityInformation & (OWNER_SECURITY_INFORMATION |
>                                 GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION))
>      {
>  @@ -331,7 +331,7 @@
>                          OUT PACCESS_MASK DesiredAccess)
>   {
>      *DesiredAccess = 0;
>  -
>  +
>      if (SecurityInformation & (OWNER_SECURITY_INFORMATION |
> GROUP_SECURITY_INFORMATION))
>      {
>          *DesiredAccess |= WRITE_OWNER;
>  @@ -374,7 +374,7 @@
>      PSID Sid;
>      NTSTATUS Status;
>      PAGED_CODE();
>  -
>  +
>      /* Check if this is kernel mode */
>      if (AccessMode == KernelMode)
>      {
>  @@ -391,12 +391,12 @@
>              /* Give the desired and previous access */
>              *GrantedAccess = DesiredAccess | PreviouslyGrantedAccess;
>          }
>  -
>  +
>          /* Success */
>          *AccessStatus = STATUS_SUCCESS;
>          return TRUE;
>      }
>  -
>  +
>      /* Check if we didn't get an SD */
>      if (!SecurityDescriptor)
>      {
>  @@ -404,7 +404,7 @@
>          *AccessStatus = STATUS_ACCESS_DENIED;
>          return FALSE;
>      }
>  -
>  +
>      /* Check for invalid impersonation */
>      if ((SubjectSecurityContext->ClientToken) &&
>          (SubjectSecurityContext->ImpersonationLevel < SecurityImpersonation))
>  @@ -412,7 +412,7 @@
>          *AccessStatus = STATUS_BAD_IMPERSONATION_LEVEL;
>          return FALSE;
>      }
>  -
>  +
>      /* Check for no access desired */
>      if (!DesiredAccess)
>      {
>  @@ -423,31 +423,31 @@
>              *AccessStatus = STATUS_ACCESS_DENIED;
>              return FALSE;
>          }
>  -
>  +
>          /* Return the previous access only */
>          *GrantedAccess = PreviouslyGrantedAccess;
>          *AccessStatus = STATUS_SUCCESS;
>          *Privileges = NULL;
>          return TRUE;
>      }
>  -
>  +
>      /* Acquire the lock if needed */
>      if (!SubjectContextLocked) SeLockSubjectContext(SubjectSecurityContext);
>  -
>  +
>      /* Map given accesses */
>      RtlMapGenericMask(&DesiredAccess, GenericMapping);
>      if (PreviouslyGrantedAccess)
>          RtlMapGenericMask(&PreviouslyGrantedAccess, GenericMapping);
>  -
>  -
>  -
>  +
>  +
>  +
>      CurrentAccess = PreviouslyGrantedAccess;
>  -
>  -
>  -
>  +
>  +
>  +
>      Token = SubjectSecurityContext->ClientToken ?
>      SubjectSecurityContext->ClientToken : SubjectSecurityContext->PrimaryToken;
>  -
>  +
>      /* Get the DACL */
>      Status = RtlGetDaclSecurityDescriptor(SecurityDescriptor,
>                                            &Present,
>  @@ -459,11 +459,11 @@
>          {
>              SeUnlockSubjectContext(SubjectSecurityContext);
>          }
>  -
>  +
>          *AccessStatus = Status;
>          return FALSE;
>      }
>  -
>  +
>      /* RULE 1: Grant desired access if the object is unprotected */
>      if (Present == TRUE && Dacl == NULL)
>      {
>  @@ -471,18 +471,18 @@
>          {
>              SeUnlockSubjectContext(SubjectSecurityContext);
>          }
>  -
>  +
>          *GrantedAccess = DesiredAccess;
>          *AccessStatus = STATUS_SUCCESS;
>          return TRUE;
>      }
>  -
>  +
>      CurrentAccess = PreviouslyGrantedAccess;
>  -
>  +
>      /* RULE 2: Check token for 'take ownership' privilege */
>      Privilege.Luid = SeTakeOwnershipPrivilege;
>      Privilege.Attributes = SE_PRIVILEGE_ENABLED;
>  -
>  +
>      if (SepPrivilegeCheck(Token,
>                            &Privilege,
>                            1,
>  @@ -497,13 +497,13 @@
>              {
>                  SeUnlockSubjectContext(SubjectSecurityContext);
>              }
>  -
>  +
>              *GrantedAccess = CurrentAccess;
>              *AccessStatus = STATUS_SUCCESS;
>              return TRUE;
>          }
>      }
>  -
>  +
>      /* RULE 3: Check whether the token is the owner */
>      Status = RtlGetOwnerSecurityDescriptor(SecurityDescriptor,
>                                             &Sid,
>  @@ -515,11 +515,11 @@
>          {
>              SeUnlockSubjectContext(SubjectSecurityContext);
>          }
>  -
>  +
>          *AccessStatus = Status;
>          return FALSE;
>      }
>  -
>  +
>      if (Sid && SepSidInToken(Token, Sid))
>      {
>          CurrentAccess |= (READ_CONTROL | WRITE_DAC);
>  @@ -530,13 +530,13 @@
>              {
>                  SeUnlockSubjectContext(SubjectSecurityContext);
>              }
>  -
>  +
>              *GrantedAccess = CurrentAccess;
>              *AccessStatus = STATUS_SUCCESS;
>              return TRUE;
>          }
>      }
>  -
>  +
>      /* Fail if DACL is absent */
>      if (Present == FALSE)
>      {
>  @@ -544,12 +544,12 @@
>          {
>              SeUnlockSubjectContext(SubjectSecurityContext);
>          }
>  -
>  +
>          *GrantedAccess = 0;
>          *AccessStatus = STATUS_ACCESS_DENIED;
>          return FALSE;
>      }
>  -
>  +
>      /* RULE 4: Grant rights according to the DACL */
>      CurrentAce = (PACE)(Dacl + 1);
>      for (i = 0; i < Dacl->AceCount; i++)
>  @@ -563,13 +563,13 @@
>                  {
>                      SeUnlockSubjectContext(SubjectSecurityContext);
>                  }
>  -
>  +
>                  *GrantedAccess = 0;
>                  *AccessStatus = STATUS_ACCESS_DENIED;
>                  return FALSE;
>              }
>          }
>  -
>  +
>          else if (CurrentAce->Header.AceType == ACCESS_ALLOWED_ACE_TYPE)
>          {
>              if (SepSidInToken(Token, Sid))
>  @@ -585,17 +585,17 @@
>          }
>          CurrentAce = (PACE)((ULONG_PTR)CurrentAce + CurrentAce->Header.AceSize);
>      }
>  -
>  +
>      if (SubjectContextLocked == FALSE)
>      {
>          SeUnlockSubjectContext(SubjectSecurityContext);
>      }
>  -
>  +
>      DPRINT("CurrentAccess %08lx\n DesiredAccess %08lx\n",
>             CurrentAccess, DesiredAccess);
>  -
>  +
>      *GrantedAccess = CurrentAccess & DesiredAccess;
>  -
>  +
>      if (DesiredAccess & MAXIMUM_ALLOWED)
>      {
>          *GrantedAccess = CurrentAccess;
>  @@ -688,32 +688,24 @@
>      SubjectSecurityContext.ProcessAuditId = NULL;
>      SeLockSubjectContext(&SubjectSecurityContext);
>
>  -    /* FIXME */
>      /* Now perform the access check */
>  -    if (SeAccessCheck(SecurityDescriptor,
>  -                      &SubjectSecurityContext,
>  -                      TRUE,
>  -                      DesiredAccess,
>  -                      0,
>  -                      &PrivilegeSet, //FIXME
>  -                      GenericMapping,
>  -                      PreviousMode,
>  -                      GrantedAccess,
>  -                      AccessStatus))
>  -    {
>  -        Status = *AccessStatus;
>  -    }
>  -    else
>  -    {
>  -        Status = STATUS_ACCESS_DENIED;
>  -    }
>  +    SeAccessCheck(SecurityDescriptor,
>  +                  &SubjectSecurityContext,
>  +                  TRUE,
>  +                  DesiredAccess,
>  +                  0,
>  +                  &PrivilegeSet, //FIXME
>  +                  GenericMapping,
>  +                  PreviousMode,
>  +                  GrantedAccess,
>  +                  AccessStatus);
>
>      /* Unlock subject context and dereference the token */
>      SeUnlockSubjectContext(&SubjectSecurityContext);
>      ObDereferenceObject(Token);
>
>  -    /* Check succeeded? */
>  -    return Status;
>  +    /* Check succeeded */
>  +    return STATUS_SUCCESS;
>   }
>
>
>
>

[Aleksey Bragin]

It's awful and goes against all written and unwritten rules, not to  
say common sense.


Also, as in discussion in bug #4169 I see total lack of  
understanding. Johannes fixes how he thinks it's right, Dmitry fixes  
another place thinking he is right, Stefan blames everyone thinking  
he is right.

What anyone of you have done to ensure correct behaviour? Was testing  
of the kernel exported routine has been done? And then testing  
advapi's code above the "correct" implementation? Even if the current  
kernel's implementation is bad, it's not a reason to hack it further.

Also asking in the ros-dev ML helps if any of you have no idea what  
they are fixing. There are still some knowledgable people here.


WBR,
Aleksey Bragin.


On Jun 26, 2009, at 1:59 AM, Alex Ionescu wrote:

>   Oh, I see. All these white spaces were hacks.
>
>  Thanks for mixing in 500 whitespace and formatting changes with 5
>  lines of code changes. It makes it really clear!
>
>  Has the kernel become a no-man's land of garbage? I'm thinking of
>  removing my name from the sources if this keeps up.
>
>  Best regards,
>  Alex Ionescu
>
>
>
>  On Thu, Jun 25, 2009 at 6:29 AM, <dgorbachev(a)svn.reactos.org> wrote:
> >   Author: dgorbachev
> >  Date: Thu Jun 25 17:29:58 2009
> >  New Revision: 41610
> >
> >  URL: http://svn.reactos.org/svn/reactos?rev=41610&view=rev
> >  Log:
> >  Remove a hack from NtAccessCheck(). Bug #4169.
> >
> >  Modified:
> >     trunk/reactos/ntoskrnl/se/semgr.c
> >
> >  Modified: trunk/reactos/ntoskrnl/se/semgr.c
> >  URL: http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/se/ 
> >  semgr.c?rev=41610&r1=41609&r2=41610&view=diff
> >  ===================================================================== 
> >  =========
> >  --- trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] (original)
> >  +++ trunk/reactos/ntoskrnl/se/semgr.c [iso-8859-1] Thu Jun 25  
> >  17:29:58 2009
> >  @@ -49,7 +49,7 @@
> >      SepExports.SeSystemEnvironmentPrivilege =  
> >  SeSystemEnvironmentPrivilege;
> >      SepExports.SeChangeNotifyPrivilege = SeChangeNotifyPrivilege;
> >      SepExports.SeRemoteShutdownPrivilege = SeRemoteShutdownPrivilege;
> >  -
> >  +
> >      SepExports.SeNullSid = SeNullSid;
> >      SepExports.SeWorldSid = SeWorldSid;
> >      SepExports.SeLocalSid = SeLocalSid;
> >  @@ -72,11 +72,11 @@
> >      SepExports.SeAuthenticatedUsersSid = SeAuthenticatedUsersSid;
> >      SepExports.SeRestrictedSid = SeRestrictedSid;
> >      SepExports.SeAnonymousLogonSid = SeAnonymousLogonSid;
> >  -
> >  +
> >      SepExports.SeUndockPrivilege = SeUndockPrivilege;
> >      SepExports.SeSyncAgentPrivilege = SeSyncAgentPrivilege;
> >      SepExports.SeEnableDelegationPrivilege =  
> >  SeEnableDelegationPrivilege;
> >  -
> >  +
> >      SeExports = &SepExports;
> >      return TRUE;
> >   }
> >  @@ -92,18 +92,18 @@
> >      if (!SepInitSDs()) return FALSE;
> >      SepInitPrivileges();
> >      if (!SepInitExports()) return FALSE;
> >  -
> >  +
> >      /* Initialize the subject context lock */
> >      ExInitializeResource(&SepSubjectContextLock);
> >  -
> >  +
> >      /* Initialize token objects */
> >      SepInitializeTokenImplementation();
> >  -
> >  +
> >      /* Clear impersonation info for the idle thread */
> >      PsGetCurrentThread()->ImpersonationInfo = NULL;
> >      PspClearCrossThreadFlag(PsGetCurrentThread(),
> >                              CT_ACTIVE_IMPERSONATION_INFO_BIT);
> >  -
> >  +
> >      /* Initialize the boot token */
> >      ObInitializeFastReference(&PsGetCurrentProcess()->Token, NULL);
> >      ObInitializeFastReference(&PsGetCurrentProcess()->Token,
> >  @@ -117,7 +117,7 @@
> >   {
> >      NTSTATUS Status;
> >      PAGED_CODE();
> >  -
> >  +
> >      /* Insert the system token into the tree */
> >      Status = ObInsertObject((PVOID)(PsGetCurrentProcess()- 
> > >  Token.Value & 
> >                           
> >           ~MAX_FAST_REFS),
> >  @@ -127,7 +127,7 @@
> >                              NULL,
> >                              NULL);
> >      ASSERT(NT_SUCCESS(Status));
> >  -
> >  +
> >      /* FIXME: TODO \\ Security directory */
> >      return TRUE;
> >   }
> >  @@ -140,17 +140,17 @@
> >      switch (ExpInitializationPhase)
> >      {
> >          case 0:
> >  -
> >  +
> >              /* Do Phase 0 */
> >              return SepInitializationPhase0();
> >  -
> >  +
> >          case 1:
> >  -
> >  +
> >              /* Do Phase 1 */
> >              return SepInitializationPhase1();
> >  -
> >  +
> >          default:
> >  -
> >  +
> >              /* Don't know any other phase! Bugcheck! */
> >              KeBugCheckEx(UNEXPECTED_INITIALIZATION_CALL,
> >                           0,
> >  @@ -170,7 +170,7 @@
> >      HANDLE DirectoryHandle;
> >      HANDLE EventHandle;
> >      NTSTATUS Status;
> >  -
> >  +
> >      /* Create '\Security' directory */
> >      RtlInitUnicodeString(&Name,
> >                           L"\\Security");
> >  @@ -187,7 +187,7 @@
> >          DPRINT1("Failed to create 'Security' directory!\n");
> >          return FALSE;
> >      }
> >  -
> >  +
> >      /* Create 'LSA_AUTHENTICATION_INITALIZED' event */
> >      RtlInitUnicodeString(&Name,
> >                           L"\\LSA_AUTHENTICATION_INITALIZED");
> >  @@ -207,12 +207,12 @@
> >          NtClose(DirectoryHandle);
> >          return FALSE;
> >      }
> >  -
> >  +
> >      ZwClose(EventHandle);
> >      ZwClose(DirectoryHandle);
> >  -
> >  +
> >      /* FIXME: Create SRM port and listener thread */
> >  -
> >  +
> >      return TRUE;
> >   }
> >
> >  @@ -228,16 +228,16 @@
> >                        IN PGENERIC_MAPPING GenericMapping)
> >   {
> >      PAGED_CODE();
> >  -
> >  +
> >      /* Select the operation type */
> >      switch (OperationType)
> >      {
> >              /* Setting a new descriptor */
> >          case SetSecurityDescriptor:
> >  -
> >  +
> >              /* Sanity check */
> >              ASSERT((PoolType == PagedPool) || (PoolType ==  
> >  NonPagedPool));
> >  -
> >  +
> >              /* Set the information */
> >              return ObSetSecurityDescriptorInfo(Object,
> >                                                 SecurityInformation,
> >  @@ -245,33 +245,33 @@
> >                                                 OldSecurityDescriptor,
> >                                                 PoolType,
> >                                                 GenericMapping);
> >  -
> >  +
> >          case QuerySecurityDescriptor:
> >  -
> >  +
> >              /* Query the information */
> >              return ObQuerySecurityDescriptorInfo(Object,
> >                                                   SecurityInformation,
> >                                                   SecurityDescriptor,
> >                                                   ReturnLength,
> >                                                    
> >  OldSecurityDescriptor);
> >  -
> >  +
> >          case DeleteSecurityDescriptor:
> >  -
> >  +
> >              /* De-assign it */
> >              return ObDeassignSecurity(OldSecurityDescriptor);
> >  -
> >  +
> >          case AssignSecurityDescriptor:
> >  -
> >  +
> >              /* Assign it */
> >              ObAssignObjectSecurityDescriptor(Object,  
> >  SecurityDescriptor, PoolType);
> >              return STATUS_SUCCESS;
> >  -
> >  +
> >          default:
> >  -
> >  +
> >              /* Bug check */
> >              KeBugCheckEx(SECURITY_SYSTEM, 0,  
> >  STATUS_INVALID_PARAMETER, 0, 0);
> >      }
> >  -
> >  +
> >      /* Should never reach here */
> >      ASSERT(FALSE);
> >      return STATUS_SUCCESS;
> >  @@ -284,14 +284,14 @@
> >   {
> >      ULONG i;
> >      PTOKEN Token = (PTOKEN)_Token;
> >  -
> >  +
> >      PAGED_CODE();
> >  -
> >  +
> >      if (Token->UserAndGroupCount == 0)
> >      {
> >          return FALSE;
> >      }
> >  -
> >  +
> >      for (i=0; i<Token->UserAndGroupCount; i++)
> >      {
> >          if (RtlEqualSid(Sid, Token->UserAndGroups[i].Sid))
> >  @@ -300,11 +300,11 @@
> >              {
> >                  return TRUE;
> >              }
> >  -
> >  +
> >              return FALSE;
> >          }
> >      }
> >  -
> >  +
> >      return FALSE;
> >   }
> >
> >  @@ -314,7 +314,7 @@
> >                            OUT PACCESS_MASK DesiredAccess)
> >   {
> >      *DesiredAccess = 0;
> >  -
> >  +
> >      if (SecurityInformation & (OWNER_SECURITY_INFORMATION |
> >                                 GROUP_SECURITY_INFORMATION |  
> >  DACL_SECURITY_INFORMATION))
> >      {
> >  @@ -331,7 +331,7 @@
> >                          OUT PACCESS_MASK DesiredAccess)
> >   {
> >      *DesiredAccess = 0;
> >  -
> >  +
> >      if (SecurityInformation & (OWNER_SECURITY_INFORMATION |  
> >  GROUP_SECURITY_INFORMATION))
> >      {
> >          *DesiredAccess |= WRITE_OWNER;
> >  @@ -374,7 +374,7 @@
> >      PSID Sid;
> >      NTSTATUS Status;
> >      PAGED_CODE();
> >  -
> >  +
> >      /* Check if this is kernel mode */
> >      if (AccessMode == KernelMode)
> >      {
> >  @@ -391,12 +391,12 @@
> >              /* Give the desired and previous access */
> >              *GrantedAccess = DesiredAccess | PreviouslyGrantedAccess;
> >          }
> >  -
> >  +
> >          /* Success */
> >          *AccessStatus = STATUS_SUCCESS;
> >          return TRUE;
> >      }
> >  -
> >  +
> >      /* Check if we didn't get an SD */
> >      if (!SecurityDescriptor)
> >      {
> >  @@ -404,7 +404,7 @@
> >          *AccessStatus = STATUS_ACCESS_DENIED;
> >          return FALSE;
> >      }
> >  -
> >  +
> >      /* Check for invalid impersonation */
> >      if ((SubjectSecurityContext->ClientToken) &&
> >          (SubjectSecurityContext->ImpersonationLevel <  
> >  SecurityImpersonation))
> >  @@ -412,7 +412,7 @@
> >          *AccessStatus = STATUS_BAD_IMPERSONATION_LEVEL;
> >          return FALSE;
> >      }
> >  -
> >  +
> >      /* Check for no access desired */
> >      if (!DesiredAccess)
> >      {
> >  @@ -423,31 +423,31 @@
> >              *AccessStatus = STATUS_ACCESS_DENIED;
> >              return FALSE;
> >          }
> >  -
> >  +
> >          /* Return the previous access only */
> >          *GrantedAccess = PreviouslyGrantedAccess;
> >          *AccessStatus = STATUS_SUCCESS;
> >          *Privileges = NULL;
> >          return TRUE;
> >      }
> >  -
> >  +
> >      /* Acquire the lock if needed */
> >      if (!SubjectContextLocked) SeLockSubjectContext 
> >  (SubjectSecurityContext);
> >  -
> >  +
> >      /* Map given accesses */
> >      RtlMapGenericMask(&DesiredAccess, GenericMapping);
> >      if (PreviouslyGrantedAccess)
> >          RtlMapGenericMask(&PreviouslyGrantedAccess, GenericMapping);
> >  -
> >  -
> >  -
> >  +
> >  +
> >  +
> >      CurrentAccess = PreviouslyGrantedAccess;
> >  -
> >  -
> >  -
> >  +
> >  +
> >  +
> >      Token = SubjectSecurityContext->ClientToken ?
> >      SubjectSecurityContext->ClientToken : SubjectSecurityContext- 
> > >  PrimaryToken; 
> >  -
> >  +
> >      /* Get the DACL */
> >      Status = RtlGetDaclSecurityDescriptor(SecurityDescriptor,
> >                                            &Present,
> >  @@ -459,11 +459,11 @@
> >          {
> >              SeUnlockSubjectContext(SubjectSecurityContext);
> >          }
> >  -
> >  +
> >          *AccessStatus = Status;
> >          return FALSE;
> >      }
> >  -
> >  +
> >      /* RULE 1: Grant desired access if the object is unprotected */
> >      if (Present == TRUE && Dacl == NULL)
> >      {
> >  @@ -471,18 +471,18 @@
> >          {
> >              SeUnlockSubjectContext(SubjectSecurityContext);
> >          }
> >  -
> >  +
> >          *GrantedAccess = DesiredAccess;
> >          *AccessStatus = STATUS_SUCCESS;
> >          return TRUE;
> >      }
> >  -
> >  +
> >      CurrentAccess = PreviouslyGrantedAccess;
> >  -
> >  +
> >      /* RULE 2: Check token for 'take ownership' privilege */
> >      Privilege.Luid = SeTakeOwnershipPrivilege;
> >      Privilege.Attributes = SE_PRIVILEGE_ENABLED;
> >  -
> >  +
> >      if (SepPrivilegeCheck(Token,
> >                            &Privilege,
> >                            1,
> >  @@ -497,13 +497,13 @@
> >              {
> >                  SeUnlockSubjectContext(SubjectSecurityContext);
> >              }
> >  -
> >  +
> >              *GrantedAccess = CurrentAccess;
> >              *AccessStatus = STATUS_SUCCESS;
> >              return TRUE;
> >          }
> >      }
> >  -
> >  +
> >      /* RULE 3: Check whether the token is the owner */
> >      Status = RtlGetOwnerSecurityDescriptor(SecurityDescriptor,
> >                                             &Sid,
> >  @@ -515,11 +515,11 @@
> >          {
> >              SeUnlockSubjectContext(SubjectSecurityContext);
> >          }
> >  -
> >  +
> >          *AccessStatus = Status;
> >          return FALSE;
> >      }
> >  -
> >  +
> >      if (Sid && SepSidInToken(Token, Sid))
> >      {
> >          CurrentAccess |= (READ_CONTROL | WRITE_DAC);
> >  @@ -530,13 +530,13 @@
> >              {
> >                  SeUnlockSubjectContext(SubjectSecurityContext);
> >              }
> >  -
> >  +
> >              *GrantedAccess = CurrentAccess;
> >              *AccessStatus = STATUS_SUCCESS;
> >              return TRUE;
> >          }
> >      }
> >  -
> >  +
> >      /* Fail if DACL is absent */
> >      if (Present == FALSE)
> >      {
> >  @@ -544,12 +544,12 @@
> >          {
> >              SeUnlockSubjectContext(SubjectSecurityContext);
> >          }
> >  -
> >  +
> >          *GrantedAccess = 0;
> >          *AccessStatus = STATUS_ACCESS_DENIED;
> >          return FALSE;
> >      }
> >  -
> >  +
> >      /* RULE 4: Grant rights according to the DACL */
> >      CurrentAce = (PACE)(Dacl + 1);
> >      for (i = 0; i < Dacl->AceCount; i++)
> >  @@ -563,13 +563,13 @@
> >                  {
> >                      SeUnlockSubjectContext(SubjectSecurityContext);
> >                  }
> >  -
> >  +
> >                  *GrantedAccess = 0;
> >                  *AccessStatus = STATUS_ACCESS_DENIED;
> >                  return FALSE;
> >              }
> >          }
> >  -
> >  +
> >          else if (CurrentAce->Header.AceType ==  
> >  ACCESS_ALLOWED_ACE_TYPE)
> >          {
> >              if (SepSidInToken(Token, Sid))
> >  @@ -585,17 +585,17 @@
> >          }
> >          CurrentAce = (PACE)((ULONG_PTR)CurrentAce + CurrentAce- 
> > >  Header.AceSize); 
> >      }
> >  -
> >  +
> >      if (SubjectContextLocked == FALSE)
> >      {
> >          SeUnlockSubjectContext(SubjectSecurityContext);
> >      }
> >  -
> >  +
> >      DPRINT("CurrentAccess %08lx\n DesiredAccess %08lx\n",
> >             CurrentAccess, DesiredAccess);
> >  -
> >  +
> >      *GrantedAccess = CurrentAccess & DesiredAccess;
> >  -
> >  +
> >      if (DesiredAccess & MAXIMUM_ALLOWED)
> >      {
> >          *GrantedAccess = CurrentAccess;
> >  @@ -688,32 +688,24 @@
> >      SubjectSecurityContext.ProcessAuditId = NULL;
> >      SeLockSubjectContext(&SubjectSecurityContext);
> >
> >  -    /* FIXME */
> >      /* Now perform the access check */
> >  -    if (SeAccessCheck(SecurityDescriptor,
> >  -                      &SubjectSecurityContext,
> >  -                      TRUE,
> >  -                      DesiredAccess,
> >  -                      0,
> >  -                      &PrivilegeSet, //FIXME
> >  -                      GenericMapping,
> >  -                      PreviousMode,
> >  -                      GrantedAccess,
> >  -                      AccessStatus))
> >  -    {
> >  -        Status = *AccessStatus;
> >  -    }
> >  -    else
> >  -    {
> >  -        Status = STATUS_ACCESS_DENIED;
> >  -    }
> >  +    SeAccessCheck(SecurityDescriptor,
> >  +                  &SubjectSecurityContext,
> >  +                  TRUE,
> >  +                  DesiredAccess,
> >  +                  0,
> >  +                  &PrivilegeSet, //FIXME
> >  +                  GenericMapping,
> >  +                  PreviousMode,
> >  +                  GrantedAccess,
> >  +                  AccessStatus);
> >
> >      /* Unlock subject context and dereference the token */
> >      SeUnlockSubjectContext(&SubjectSecurityContext);
> >      ObDereferenceObject(Token);
> >
> >  -    /* Check succeeded? */
> >  -    return Status;
> >  +    /* Check succeeded */
> >  +    return STATUS_SUCCESS;
> >   }
> >
> >
> >
> >  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Dmitry Gorbachev]

Aleksey Bragin wrote:
>   Also, as in discussion in bug #4169 I see total lack
> of
>  understanding. Johannes fixes how he thinks it's right, Dmitry fixes
>  another place thinking he is right, Stefan blames everyone thinking
>  he is right.
>
>  What anyone of you have done to ensure correct behaviour? Was testing
>  of the kernel exported routine has been done? And then testing
>  advapi's code above the "correct" implementation? Even if the current
>  kernel's implementation is bad, it's not a reason to hack it further.
>
>  Also asking in the ros-dev ML helps if any of you have no idea what
>  they are fixing. There are still some knowledgable people here. 
This regression, which is known for 4 months and about which people
complained on the forum, I thought it was enough reason to return to a
hack when nobody is working on it.

[Aleksey Bragin]

At least a quick question in ros-dev ML would raise attention and  
could prevent some frustration.

WBR,
Aleksey Bragin.

On Jun 26, 2009, at 6:30 PM, Dmitry Gorbachev wrote:

>   This regression, which is known for 4 months and about
> which people
>  complained on the forum, I thought it was enough reason to return to a
>  hack when nobody is working on it.