[PATCH v2 0/2] MR9553: ntdll: Refuse to start if wineserver is in wrong pid namespace

[Alfred Agrell (＠Alcaro)]

To reproduce:
- `unshare -Upf ./wine cmd`
- (in another terminal) `unshare -Upf ./wine not-a-command`
The first terminal will now go completely unresponsive, until you kill -9 the relevant start.exe from the second terminal.

Root cause analysis: In the first Wine, wineserver gets Unix pid 3 in its namespace. (And a different pid in the root namespace, but no relevant process sees that.)

In the second Wine, not-a-command gets Unix pid 3 in its namespace, and sends it to wineserver.

When not-a-command notices it doesn't exist and exits, wineserver checks if pid 3 did indeed exit, and SIGKILLs it if not. But since that pid is from wrong namespace, wineserver ends up killing itself instead.

While this only happens in badly configured sandboxes, such things do exist in the wild. https://github.com/flathub/org.winehq.Wine/issues/41

The real solution would be either fixing the sandbox config, or making Wine use pidfds instead of pids, but former is out of our control, and latter would be a lot of effort and ifdefs (and unlikely to work, there's no process_vm_readv for pidfds).

The second commit only blocks cases where wineserver exists in ntdll's namespace, but not the other way round. It's a much rarer case than having the processes in mutually-inaccessible sibling namespaces, it's a much bigger patch than the ntdll side, and the error is detected at wrong side (meaning it can't print a friendly error). I'm not sure if it's worth keeping.

--
  v2: server: Reject clients from wrong pid namespace
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9553

[PATCH v2 1/2] ntdll: Refuse to start if wineserver is in wrong pid namespace

[Alfred Agrell]

From: Alfred Agrell <floating(a)muncher.se>

---
 dlls/ntdll/unix/server.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
index 258a959de72..1fb02aa4125 100644
--- a/dlls/ntdll/unix/server.c
+++ b/dlls/ntdll/unix/server.c
@@ -1655,6 +1655,10 @@ size_t server_init_process(void)
     /* work around Ubuntu's ptrace breakage */
     if (server_pid != -1) prctl( 0x59616d61 /* PR_SET_PTRACER */, server_pid );
 #endif
+    /* ensure wineserver exists in our pid namespace */
+    if (server_pid == 0)
+        fatal_error( "cannot determine wineserver's process ID.\n"
+                     "Is it running in wrong pid namespace?\n" );
 
     /* ignore SIGPIPE so that we get an EPIPE error instead  */
     sig_act.sa_handler = SIG_IGN;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/9553

[PATCH v2 2/2] server: Reject clients from wrong pid namespace

[Alfred Agrell]

From: Alfred Agrell <floating(a)muncher.se>

---
 server/process.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/server/process.c b/server/process.c
index 52abaa21d75..34c26ce9eb2 100644
--- a/server/process.c
+++ b/server/process.c
@@ -1146,6 +1146,30 @@ int set_process_debug_flag( struct process *process, int flag )
     return write_process_memory( process, process->peb + 2, 1, &data, NULL );
 }
 
+#ifdef __linux__
+int socket_peer_has_pid(int socket_fd)
+{
+    /* ensure the caller exists in our pid namespace */
+    struct ucred ucred;
+    socklen_t len = sizeof(struct ucred);
+    if (getsockopt(socket_fd, SOL_SOCKET, SO_PEERCRED, &ucred, &len) < 0)
+        return 0;
+    if (ucred.uid != getuid())
+        return 0;
+    /* this is the pid of whoever created the socket, i.e. the parent of the newly created process */
+    /* we still trust the unix_pid in init_first_thread */
+    if (ucred.pid == 0)
+        return 0;
+    return 1;
+}
+#else
+int socket_peer_has_pid(int socket_fd)
+{
+    /* pid namespaces aren't a thing outside Linux */
+    return 1;
+}
+#endif
+
 /* create a new process */
 DECL_HANDLER(new_process)
 {
@@ -1188,6 +1212,14 @@ DECL_HANDLER(new_process)
         close( socket_fd );
         return;
     }
+    if (!socket_peer_has_pid( socket_fd ))
+    {
+        set_error( SCHED_E_NAMESPACE );
+        /* caller gave us a bad pid, discard it, don't kill the named pid */
+        current->process->unix_pid = -1;
+        close( socket_fd );
+        return;
+    }
 
     if (req->parent_process)
     {
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/9553

[Zdeněk Kopřivík]

Please note, that while this is probably technically correct, it doesn't fix the issue and it actually makes the situation even worse for the Wine in Flatpak by refusing to run the second app at all.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9553#note_123339

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/process.c:
> close( socket_fd );
>          return;
>      }
> +    if (!socket_peer_has_pid( socket_fd ))
> +    {
> +        set_error( SCHED_E_NAMESPACE );

I don't think Linux PIDs have anything to do with "Windows Task Scheduler subsystem error: The task XML contains an element or attribute from an unexpected namespace."

```suggestion:-0+0
        fprintf( stderr, "wineserver: Rejecting client with unknown PID in peercred\n" );
        set_error( STATUS_UNSUCCESSFUL );
```

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/9553#note_123352