[PATCH] server: Disallow duplicating file handle to elevate access.

[Daniel Lehman]

Signed-off-by: Daniel Lehman <dlehman25(a)gmail.com>
---
 dlls/kernel32/tests/file.c | 7 -------
 server/file.c              | 5 +++++
 server/file.h              | 2 ++
 server/handle.c            | 8 ++++++++
 4 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index 33b17aa327..641fcc1750 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -4679,15 +4679,8 @@ static void test_file_access(void)
                 ok(ret, "DuplicateHandle(%#x => %#x) error %d\n", td[i].access, td[j].access, GetLastError());
             else
             {
-                /* FIXME: Remove once Wine is fixed */
-                todo_wine_if((td[j].access & (GENERIC_READ | GENERIC_WRITE) ||
-                             (!(td[i].access & (GENERIC_WRITE | FILE_WRITE_DATA)) && (td[j].access & FILE_WRITE_DATA)) ||
-                             (!(td[i].access & (GENERIC_READ | FILE_READ_DATA)) && (td[j].access & FILE_READ_DATA)) ||
-                             (!(td[i].access & (GENERIC_WRITE)) && (td[j].access & FILE_APPEND_DATA))))
-                {
                 ok(!ret, "DuplicateHandle(%#x => %#x) should fail\n", td[i].access, td[j].access);
                 ok(GetLastError() == ERROR_ACCESS_DENIED, "expected ERROR_ACCESS_DENIED, got %d\n", GetLastError());
-                }
             }
             if (ret) CloseHandle(hdup);
         }
diff --git a/server/file.c b/server/file.c
index 8d39f303bd..a526467332 100644
--- a/server/file.c
+++ b/server/file.c
@@ -772,3 +772,8 @@ DECL_HANDLER(unlock_file)
         release_object( file );
     }
 }
+
+int is_file_object( const struct object *obj )
+{
+    return obj->ops == &file_ops;
+}
diff --git a/server/file.h b/server/file.h
index 6b67866d3f..82c5eac03b 100644
--- a/server/file.h
+++ b/server/file.h
@@ -224,4 +224,6 @@ static inline int async_queued( struct async_queue *queue )
 #define FILE_MAPPING_WRITE  0x40000000  /* set for writable shared mappings */
 #define FILE_MAPPING_ACCESS 0x20000000  /* set for all mappings */
 
+int is_file_object( const struct object * );
+
 #endif  /* __WINE_SERVER_FILE_H */
diff --git a/server/handle.c b/server/handle.c
index 35ab8607c8..4f915ea3f8 100644
--- a/server/handle.c
+++ b/server/handle.c
@@ -38,6 +38,7 @@
 #include "thread.h"
 #include "security.h"
 #include "request.h"
+#include "file.h"
 
 struct handle_entry
 {
@@ -555,6 +556,13 @@ obj_handle_t duplicate_handle( struct process *src, obj_handle_t src_handle, str
     /* asking for the more access rights than src_access? */
     if (access & ~src_access)
     {
+        if (is_file_object( obj ))
+        {
+            set_error( STATUS_ACCESS_DENIED );
+            release_object( obj );
+            return 0;
+        }
+
         if (options & DUP_HANDLE_MAKE_GLOBAL)
             res = alloc_global_handle( obj, access );
         else
-- 
2.17.1

[Alexandre Julliard]

Daniel Lehman <dlehman25(a)gmail.com> writes:
> @@ -555,6 +556,13 @@ obj_handle_t duplicate_handle( struct process *src, obj_handle_t src_handle, str
>      /* asking for the more access rights than src_access? */
>      if (access & ~src_access)
>      {
> +        if (is_file_object( obj ))
> +        {
> +            set_error( STATUS_ACCESS_DENIED );
> +            release_object( obj );
> +            return 0;
> +        }

You can't special-case it like that. This should be handled as part of
the standard access checks.

-- 
Alexandre Julliard
julliard(a)winehq.org