[PATCH v2 2/2] server: Grant implicit DACL_SECURITY_INFORMATION permission to an object owner.

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov 
---
 dlls/advapi32/tests/security.c |  4 ----
 server/handle.c                | 11 +++++++++++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/dlls/advapi32/tests/security.c b/dlls/advapi32/tests/security.c
index ff180ae4da4..73e19bd69f6 100644
--- a/dlls/advapi32/tests/security.c
+++ b/dlls/advapi32/tests/security.c
@@ -7963,15 +7963,11 @@ static void test_regkey_security(void)
     sd = NULL;
     dacl = NULL;
     ret = GetSecurityInfo(hkey, SE_REGISTRY_KEY, DACL_SECURITY_INFORMATION, NULL, NULL, &dacl, NULL, &sd);
-todo_wine
     ok(ret == ERROR_SUCCESS, "got %u\n", ret);
-todo_wine
     ok(sd != NULL, "sd should not be NULL\n");
-todo_wine
     ok(dacl != NULL, "dacl should not be NULL\n");

ret = SetSecurityInfo(hkey, SE_REGISTRY_KEY, DACL_SECURITY_INFORMATION, NULL, NULL, dacl, NULL);
-todo_wine
     ok(ret == ERROR_SUCCESS, "got %u\n", ret);

LocalFree(sd);
diff --git a/server/handle.c b/server/handle.c
index a6fcb871e2d..7a11e30017e 100644
--- a/server/handle.c
+++ b/server/handle.c
@@ -482,6 +482,17 @@ struct object *get_handle_obj( struct process *process, obj_handle_t handle,
             set_error( STATUS_OBJECT_TYPE_MISMATCH );  /* not the right type */
             return NULL;
         }
+        if (access & (READ_CONTROL | WRITE_DAC))
+        {
+            const struct security_descriptor *sd = obj->ops->get_sd( obj );
+            if (sd)
+            {
+                struct token *token = current->token ? current->token : current->process->token;
+                const SID *owner = sd_get_owner( sd );
+                if (token_sid_present( token, owner, FALSE ))
+                    access &= ~(READ_CONTROL | WRITE_DAC);
+            }
+        }
         if ((entry->access & access) != access)
         {
             set_error( STATUS_ACCESS_DENIED );

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> @@ -482,6 +482,17 @@ struct object *get_handle_obj( struct process *process, obj_handle_t handle,
>              set_error( STATUS_OBJECT_TYPE_MISMATCH );  /* not the right type */
>              return NULL;
>          }
> -
>    if (access & (READ_CONTROL | WRITE_DAC))
> -
>    {
> -
>        const struct security_descriptor *sd = obj->ops->get_sd( obj );
> -
>        if (sd)
> -
>        {
> -
>            struct token *token = current->token ? current->token : current->process->token;
> -
>            const SID *owner = sd_get_owner( sd );
> -
>            if (token_sid_present( token, owner, FALSE ))
> -
>                access &= ~(READ_CONTROL | WRITE_DAC);
> -
>        }
> -
>    }

That seems very ad-hoc. It would need more convincing tests.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> Dmitry Timoshkov  writes:
> > @@ -482,6 +482,17 @@ struct object *get_handle_obj( struct process *process, obj_handle_t handle,
> >              set_error( STATUS_OBJECT_TYPE_MISMATCH );  /* not the right type */
> >              return NULL;
> >          }
> > -
> >    if (access & (READ_CONTROL | WRITE_DAC))
> > -
> >    {
> > -
> >        const struct security_descriptor *sd = obj->ops->get_sd( obj );
> > -
> >        if (sd)
> > -
> >        {
> > -
> >            struct token *token = current->token ? current->token : current->process->token;
> > -
> >            const SID *owner = sd_get_owner( sd );
> > -
> >            if (token_sid_present( token, owner, FALSE ))
> > -
> >                access &= ~(READ_CONTROL | WRITE_DAC);
> > -
> >        }
> > -
> >    }
>
> That seems very ad-hoc. It would need more convincing tests.

What kind of test would be more convincing for you? Even if the tests
in 1/2 are not enough it's still a good start, and why not accept them
while anticipating more tests?

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > Dmitry Timoshkov  writes:
> > > @@ -482,6 +482,17 @@ struct object *get_handle_obj( struct process *process, obj_handle_t handle,
> > >              set_error( STATUS_OBJECT_TYPE_MISMATCH );  /* not the right type */
> > >              return NULL;
> > >          }
> > > -
> > >    if (access & (READ_CONTROL | WRITE_DAC))
> > > -
> > >    {
> > > -
> > >        const struct security_descriptor *sd = obj->ops->get_sd( obj );
> > > -
> > >        if (sd)
> > > -
> > >        {
> > > -
> > >            struct token *token = current->token ? current->token : current->process->token;
> > > -
> > >            const SID *owner = sd_get_owner( sd );
> > > -
> > >            if (token_sid_present( token, owner, FALSE ))
> > > -
> > >                access &= ~(READ_CONTROL | WRITE_DAC);
> > > -
> > >        }
> > > -
> > >    }
> >
> > That seems very ad-hoc. It would need more convincing tests.
>
> What kind of test would be more convincing for you? Even if the tests
> in 1/2 are not enough it's still a good start, and why not accept them
> while anticipating more tests?

I'd like to see tests for various object types, various types of
security descriptors and tokens, and other security bits. Adding
something like that in get_handle_obj() is going to affect everything,
so it needs extensive tests.