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

[Dmitry Timoshkov]

Signed-off-by: Dmitry Timoshkov <dmitry(a)baikal.ru>
---
 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 );
-- 
2.29.2

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> wrote:
> Dmitry Timoshkov <dmitry(a)baikal.ru> 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?

-- 
Dmitry.

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> wrote:
> > Dmitry Timoshkov <dmitry(a)baikal.ru> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org