[PATCH 0/1] MR5343: server: Fix use-after-free on root directory ("\") deletion.
If a client can crash a wineserver via requests only, it's a bug. The wineserver crashes when a privileged client deletes every object in the NT object namespace (which frees `root_directory`) and then attempts to lookup or create any object name. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343
From: Jinoh Kang <jinoh.kang.kr(a)gmail.com> If a client can crash a wineserver via requests only, it's a bug. The wineserver crashes when a privileged client deletes every object in the NT object namespace (which frees `root_directory`) and then attempts to lookup or create any object name. --- server/directory.c | 7 ++++++- server/object.c | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/server/directory.c b/server/directory.c index 23d7eb0a2b7..1c57a29ae71 100644 --- a/server/directory.c +++ b/server/directory.c @@ -494,13 +494,18 @@ void init_directories( struct fd *intl_fd ) release_object( named_pipe_device ); release_object( mailslot_device ); release_object( null_device ); - release_object( root_directory ); release_object( dir_driver ); release_object( dir_device ); release_object( dir_objtype ); release_object( dir_kernel ); release_object( dir_nls ); release_object( dir_global ); + + /* + * Leave at least one extra ref (in addition to permanent ref) for + * root_directory. This protects against use-after-free when the client + * makes it temporary and empties it on an unlikely occasion. + */ } /* create a directory object */ diff --git a/server/object.c b/server/object.c index 89e541ffb6b..11689e6cbb3 100644 --- a/server/object.c +++ b/server/object.c @@ -79,6 +79,8 @@ void dump_objects(void) void close_objects(void) { + struct object *rootdir = get_root_directory(); + /* release the permanent objects */ for (;;) { @@ -95,6 +97,11 @@ void close_objects(void) if (!found) break; } + /* release the initial and final reference */ + assert( rootdir->refcount >= 2 ); + release_object( rootdir ); + release_object( rootdir ); + dump_objects(); /* dump any remaining objects */ } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5343
I wonder if anything can really work after a client has deleted every object inside root kernel directory? Is it achievable on Windows? Or may legitimately happen in Wine due to some reasons? Maybe something is wrong elsewhere and some objects should be not removable even by privileged users? I. e., avoiding use after free in a case when everything is broken already beyond recovery maybe not wrong per se but also not all that practical. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343#note_65219
On Mon Mar 18 23:41:24 2024 +0000, Paul Gofman wrote:
I wonder if anything can really work after a client has deleted every object inside root kernel directory? Is it achievable on Windows? Or may legitimately happen in Wine due to some reasons? Maybe something is wrong elsewhere and some objects should be not removable even by privileged users? I. e., avoiding use after free in a case when everything is broken already beyond recovery maybe not wrong per se but also not all that practical. `root_directory` is a global pointer. Removing all references it will free the directory object, and the pointer will become dangling.
The only way to achieve this is to delete every named object, which is yes, not that practical. Does this mean that we can introduce another global pointer that works in a similar manner? !3103 introduces a global pointer to a permanent shared mapping, and deleting it will lead to the same kind of bug: not only use-after-free but also rendering win32u unusuable. I submitted this MR mainly so that I can learn how serious is serious about the "wineserver crash = bug" rule. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343#note_65275
On Mon Mar 18 23:43:42 2024 +0000, Jinoh Kang wrote:
`root_directory` is a global pointer. Removing all references to it will free the directory object, and the pointer will become dangling. The only way to achieve this is to delete every named object, which is, yes, not that practical. Does this mean that we can introduce another global pointer that works in a similar manner? !3103 introduces a global pointer to a permanent shared mapping, and deleting it will lead to the same kind of bug: not only use-after-free but also rendering win32u unusuable. I submitted this MR mainly so that I can learn how serious upstream is about the "wineserver crash = bug" rule. I guess wineserver crash is serious, but probably different ways of fixing it are possible. If it is not achievable without breaking everything before due to some other bugs, maybe fixing something earlier in that sequence is more valuable? Maybe it is just me, but fixing some select consequences of the total breakage looks like a pointless exercise to me.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343#note_65277
On Mon Mar 18 23:48:34 2024 +0000, Paul Gofman wrote:
I guess wineserver crash is serious, but probably different ways of fixing it are possible. If it is not achievable without breaking everything before due to some other bugs, maybe fixing something earlier in that sequence is more valuable? Maybe it is just me, but fixing some select consequences of the total breakage looks like a pointless exercise to me. It wouldn't surprise me if some global objects can't be deleted, but at the same time, relying on that is... maybe not fragile per se, but obscure. This patch seems simple and declarative.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343#note_65281
On Tue Mar 19 00:33:51 2024 +0000, Zebediah Figura wrote:
It wouldn't surprise me if some global objects can't be deleted, but at the same time, relying on that is... maybe not fragile per se, but obscure. This patch seems simple and declarative. Being able to make system objects temporary is not a bug; in fact I have been able to successfully mark `\` as temporary (and then back to permanent), and Windows allows you to actually delete other system objects this way.
However, a handful of system objects cannot be deleted this way because they still retain refcount (presumably via global references) even after it's made temporary. An example is the root directory. Wineserver doesn't have such behavior, which makes it possible to delete everything. The main intention of this patch is to prevent future regressions in this aspect, rather than fixing a known bug. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343#note_65286
This merge request was approved by Elizabeth Figura. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5343
participants (5)
-
Elizabeth Figura (＠zfigura) -
Jinoh Kang
-
Jinoh Kang (＠iamahuman)
-
Paul Gofman (＠gofman)
-
Zebediah Figura (＠zfigura)