[PATCH v5 0/1] MR3381: server: Try isolate wineserver directory.

[Konstantin Demin (＠rockdrilla)]

try following locations for wineserver directory:
- `${XDG_RUNTIME_DIR}/wine`
- `/run/user/${uid}/wine`
- `${TMPDIR}/wine` - only if `${TMPDIR}` is owned by user
- `${TMPDIR}/.wine-${uid}`
- `/tmp/.wine-${uid}`

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=39013

Signed-off-by: Konstantin Demin

--
  v5: server: Try isolate wineserver directory.

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

[PATCH v5 1/1] server: Try isolate wineserver directory.

[Konstantin Demin]

From: Konstantin Demin

try following locations for wineserver directory:
- ${XDG_RUNTIME_DIR}/wine
- /run/user/${uid}/wine
- ${TMPDIR}/wine - only if ${TMPDIR} is owned by user
- ${TMPDIR}/.wine-${uid}
- /tmp/.wine-${uid}

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=39013

Signed-off-by: Konstantin Demin 
---
 configure                |   6 ++
 configure.ac             |   1 +
 dlls/ntdll/unix/server.c | 126 +++++++++++++++++++++++++++++++--
 include/config.h.in      |   3 +
 server/request.c         | 149 ++++++++++++++++++++++++++++++++++++---
 5 files changed, 273 insertions(+), 12 deletions(-)

diff --git a/configure b/configure
index 7e0d8d7621d..789100c1558 100755
--- a/configure
+++ b/configure
@@ -20010,6 +20010,12 @@ if test "x$ac_cv_func_sched_yield" = xyes
 then :
   printf "%s\n" "#define HAVE_SCHED_YIELD 1" >>confdefs.h

+fi
+ac_fn_c_check_func "$LINENO" "secure_getenv" "ac_cv_func_secure_getenv"
+if test "x$ac_cv_func_secure_getenv" = xyes
+then :
+  printf "%s\n" "#define HAVE_SECURE_GETENV 1" >>confdefs.h
+
 fi
 ac_fn_c_check_func "$LINENO" "setproctitle" "ac_cv_func_setproctitle"
 if test "x$ac_cv_func_setproctitle" = xyes
diff --git a/configure.ac b/configure.ac
index b32d2ba5aa4..bc1c27a6cff 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2049,6 +2049,7 @@ AC_CHECK_FUNCS(\
    prctl \
    proc_pidinfo \
    sched_yield \
+	secure_getenv \
    setproctitle \
    setprogname \
    sigprocmask \
diff --git a/dlls/ntdll/unix/server.c b/dlls/ntdll/unix/server.c
index e5e234d05ae..0858b5494e5 100644
--- a/dlls/ntdll/unix/server.c
+++ b/dlls/ntdll/unix/server.c
@@ -1238,13 +1238,38 @@ int server_pipe( int fd[2] )
 }

+/***********************************************************************
+ *           try_dir
+ */
+static int try_dir( const char *name, struct stat *st, int per_user )
+{
+    if (lstat( name, st ) == -1) return 1;
+    if (!S_ISDIR( st->st_mode ))
+    {
+        errno = ENOTDIR;
+        return 2;
+    }
+    if (per_user)
+    {
+        if (st->st_uid != getuid()) return 3;
+        if (st->st_mode & 077) return 4;
+        if ((st->st_mode & 0700) != 0700) return 5;
+    }
+
+    return 0;
+}
+
+
 /***********************************************************************
  *           init_server_dir
  */
 static const char *init_server_dir( dev_t dev, ino_t ino )
 {
-    char *p, *dir;
-    size_t len = sizeof("/server-") + 2 * sizeof(dev) + 2 * sizeof(ino) + 2;
+    char *p, *dir, *server_root_dir;
+    struct stat st;
+    size_t len, len2;
+
+    len = sizeof("/server-") + 2 * sizeof(dev) + 2 * sizeof(ino) + 2;

#ifdef __ANDROID__  /* there's no /tmp dir on Android */
     len += strlen( config_dir ) + sizeof("/.wineserver");
@@ -1252,9 +1277,102 @@ static const char *init_server_dir( dev_t dev, ino_t ino )
     strcpy( dir, config_dir );
     strcat( dir, "/.wineserver/server-" );
 #else
+
+    /*
+     * try following locations for wineserver directory:
+     * - ${XDG_RUNTIME_DIR}/wine
+     * - /run/user/${uid}/wine
+     * - ${TMPDIR}/wine - only if ${TMPDIR} is per-user
+     * - ${TMPDIR}/.wine-${uid}
+     * - /tmp/.wine-${uid}
+     */
+
+    dir = NULL;
+
+    /* try "${XDG_RUNTIME_DIR}/wine" */
+
+#ifdef HAVE_SECURE_GETENV
+    server_root_dir = secure_getenv( "XDG_RUNTIME_DIR" );
+#else
+    server_root_dir = getenv( "XDG_RUNTIME_DIR" );
+#endif
+
+    if (!server_root_dir)
+        goto server_dir_at_run;
+
+    if (try_dir( server_root_dir, &st, 1 ))
+        goto server_dir_at_run;
+
+    len += strlen(server_root_dir) + sizeof("/wine") + 2;
+    if (!(dir = malloc( len ))) fatal_error( "out of memory\n" );
+    sprintf( dir, "%s/wine", server_root_dir );
+    goto server_dir_done;
+
+server_dir_at_run:
+
+    /* try "/run/user/${uid}/wine" */
+
+    len2 = len + sizeof("/run/user/") + sizeof("/wine") + 12;
+    if (!(dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+    sprintf( dir, "%s%u", "/run/user/", getuid() );
+    if (try_dir( dir, &st, 1 ))
+    {
+        free( dir );
+        dir = NULL;
+        goto server_dir_at_env_tmpdir;
+    }
+
+    strcat( dir, "/wine" );
+    len = len2;
+    goto server_dir_done;
+
+server_dir_at_env_tmpdir:
+
+    /* try somewhere in ${TMPDIR}/ */
+
+#ifdef HAVE_SECURE_GETENV
+    server_root_dir = secure_getenv( "TMPDIR" );
+#else
+    server_root_dir = getenv( "TMPDIR" );
+#endif
+
+    if (!server_root_dir)
+        goto server_dir_at_tmp;
+
+    if (try_dir( server_root_dir, &st, 0 ))
+        goto server_dir_at_tmp;
+
+    len2 = len + strlen(server_root_dir);
+    if (try_dir( server_root_dir, &st, 1 ))
+    {
+        /* try "${TMPDIR}/.wine-${uid}" because ${TMPDIR} is not per-user */
+
+        len2 += sizeof("/.wine-") + 12;
+        if (!(dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+        sprintf( dir, "%s/.wine-%u", server_root_dir, getuid() );
+    }
+    else
+    {
+        /* try "${TMPDIR}/wine" because ${TMPDIR} is per-user */
+
+        len2 += sizeof("/wine") + 2;
+        if (!(dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+        sprintf( dir, "%s/wine", server_root_dir );
+    }
+
+    len = len2;
+    goto server_dir_done;
+
+server_dir_at_tmp:
+
+    /* try "/tmp/.wine-${uid}" - last resort */
+
     len += sizeof("/tmp/.wine-") + 12;
-    dir = malloc( len );
-    sprintf( dir, "/tmp/.wine-%u/server-", getuid() );
+    if (!(dir = malloc( len ))) fatal_error( "out of memory\n" );
+    sprintf( dir, "/tmp/.wine-%u", getuid() );
+
+server_dir_done:
+    strcat( dir, "/server-" );
 #endif
     p = dir + strlen( dir );
     if (dev != (unsigned long)dev)
diff --git a/include/config.h.in b/include/config.h.in
index 2e806157ebd..43427e152dc 100644
--- a/include/config.h.in
+++ b/include/config.h.in
@@ -366,6 +366,9 @@
 /* Define to 1 if you have the <SDL.h> header file. */
 #undef HAVE_SDL_H

+/* Define to 1 if you have the `secure_getenv' function. */
+#undef HAVE_SECURE_GETENV
+
 /* Define to 1 if you have the `setproctitle' function. */
 #undef HAVE_SETPROCTITLE

diff --git a/server/request.c b/server/request.c
index 7021741c765..24566be3385 100644
--- a/server/request.c
+++ b/server/request.c
@@ -592,30 +592,66 @@ static void socket_cleanup(void)
     if (!do_it_once++) unlink( server_socket_name );
 }

+/* try stat a directory */
+static int try_dir( const char *name, struct stat *st, int per_user )
+{
+    if (lstat( name, st ) == -1) return 1;
+    if (!S_ISDIR( st->st_mode ))
+    {
+        errno = ENOTDIR;
+        return 2;
+    }
+    if (per_user)
+    {
+        if (st->st_uid != getuid()) return 3;
+        if (st->st_mode & 077) return 4;
+        if ((st->st_mode & 0700) != 0700) return 5;
+    }
+
+    return 0;
+}
+
 /* create a directory and check its permissions */
 static void create_dir( const char *name, struct stat *st )
 {
-    if (lstat( name, st ) == -1)
+    int r = try_dir( name, st, 1 );
+    switch( r )
     {
+    case 0: return;
+    case 1:
+        r = 0;
+
         if (errno != ENOENT)
             fatal_error( "lstat %s: %s\n", name, strerror( errno ));
-        if (mkdir( name, 0700 ) == -1 && errno != EEXIST)
+        if ((mkdir( name, 0700 ) == -1) && (errno != EEXIST))
             fatal_error( "mkdir %s: %s\n", name, strerror( errno ));
         if (lstat( name, st ) == -1)
             fatal_error( "lstat %s: %s\n", name, strerror( errno ));
+
+        if (!S_ISDIR( st->st_mode ))
+            r = 2;  /* passthrough to next block */
+        break;
     }
-    if (!S_ISDIR(st->st_mode)) fatal_error( "%s is not a directory\n", name );
-    if (st->st_uid != getuid()) fatal_error( "%s is not owned by you\n", name );
-    if (st->st_mode & 077) fatal_error( "%s must not be accessible by other users\n", name );
+    if (r == 2)
+        fatal_error( "%s is not a directory\n", name );
+
+    if (st->st_uid != getuid())
+        fatal_error( "%s is not owned by you\n", name );
+    if (st->st_mode & 077)
+        fatal_error( "%s must not be accessible by other users\n", name );
+    if ((st->st_mode & 0700) != 0700)
+        fatal_error( "%s must be writeable by you\n", name );
 }

/* create the server directory and chdir to it */
 static char *create_server_dir( int force )
 {
     const char *prefix = getenv( "WINEPREFIX" );
-    char *p, *config_dir;
+    char *p, *config_dir, *server_root_dir;
     struct stat st, st2;
-    size_t len = sizeof("/server-") + 2 * sizeof(st.st_dev) + 2 * sizeof(st.st_ino) + 2;
+    size_t len, len2;
+
+    len = sizeof("/server-") + 2 * sizeof(st.st_dev) + 2 * sizeof(st.st_ino) + 2;

/* open the configuration directory */

@@ -662,13 +698,110 @@ static char *create_server_dir( int force )
     if (!(server_dir = malloc( len ))) fatal_error( "out of memory\n" );
     strcpy( server_dir, config_dir );
     strcat( server_dir, "/.wineserver" );
+    create_dir( server_dir, &st2 );
+#else
+
+    /*
+     * try following locations for wineserver directory:
+     * - ${XDG_RUNTIME_DIR}/wine
+     * - /run/user/${uid}/wine
+     * - ${TMPDIR}/wine - only if ${TMPDIR} is per-user
+     * - ${TMPDIR}/.wine-${uid}
+     * - /tmp/.wine-${uid}
+     */
+
+    server_dir = NULL;
+
+    /* try "${XDG_RUNTIME_DIR}/wine" */
+
+#ifdef HAVE_SECURE_GETENV
+    server_root_dir = secure_getenv( "XDG_RUNTIME_DIR" );
+#else
+    server_root_dir = getenv( "XDG_RUNTIME_DIR" );
+#endif
+
+    if (!server_root_dir)
+        goto server_dir_at_run;
+
+    if (try_dir( server_root_dir, &st2, 1 ))
+        goto server_dir_at_run;
+
+    len += strlen(server_root_dir) + sizeof("/wine") + 2;
+    if (!(server_dir = malloc( len ))) fatal_error( "out of memory\n" );
+    sprintf( server_dir, "%s/wine", server_root_dir );
+    create_dir( server_dir, &st2 );
+    goto server_dir_done;
+
+server_dir_at_run:
+
+    /* try "/run/user/${uid}/wine" */
+
+    len2 = len + sizeof("/run/user/") + sizeof("/wine") + 12;
+    if (!(server_dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+    sprintf( server_dir, "%s%u", "/run/user/", getuid() );
+    if (try_dir( server_dir, &st2, 1 ))
+    {
+        free( server_dir );
+        server_dir = NULL;
+        goto server_dir_at_env_tmpdir;
+    }
+
+    strcat( server_dir, "/wine" );
+    create_dir( server_dir, &st2 );
+    len = len2;
+    goto server_dir_done;
+
+server_dir_at_env_tmpdir:
+
+    /* try somewhere in ${TMPDIR}/ */
+
+#ifdef HAVE_SECURE_GETENV
+    server_root_dir = secure_getenv( "TMPDIR" );
 #else
+    server_root_dir = getenv( "TMPDIR" );
+#endif
+
+    if (!server_root_dir)
+        goto server_dir_at_tmp;
+
+    if (try_dir( server_root_dir, &st2, 0 ))
+        goto server_dir_at_tmp;
+
+    len2 = len + strlen(server_root_dir);
+    if (try_dir( server_root_dir, &st2, 1 ))
+    {
+        /* try "${TMPDIR}/.wine-${uid}" because ${TMPDIR} is not per-user */
+
+        len2 += sizeof("/.wine-") + 12;
+        if (!(server_dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+        sprintf( server_dir, "%s/.wine-%u", server_root_dir, getuid() );
+    }
+    else
+    {
+        /* try "${TMPDIR}/wine" because ${TMPDIR} is per-user */
+
+        len2 += sizeof("/wine") + 2;
+        if (!(server_dir = malloc( len2 ))) fatal_error( "out of memory\n" );
+        sprintf( server_dir, "%s/wine", server_root_dir );
+    }
+
+    create_dir( server_dir, &st2 );
+    len = len2;
+    goto server_dir_done;
+
+server_dir_at_tmp:
+
+    /* try "/tmp/.wine-${uid}" - last resort */
+
     len += sizeof("/tmp/.wine-") + 12;
     if (!(server_dir = malloc( len ))) fatal_error( "out of memory\n" );
     sprintf( server_dir, "/tmp/.wine-%u", getuid() );
-#endif
     create_dir( server_dir, &st2 );

+server_dir_done:
+
+#endif
+
     /* now create the server directory */

strcat( server_dir, "/server-" );

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about dlls/ntdll/unix/server.c:
> -
> - /*
> -
> * try following locations for wineserver directory:
> -
> * - ${XDG_RUNTIME_DIR}/wine
> -
> * - /run/user/${uid}/wine
> -
> * - ${TMPDIR}/wine - only if ${TMPDIR} is per-user
> -
> * - ${TMPDIR}/.wine-${uid}
> -
> * - /tmp/.wine-${uid}
> -
> */
> -
> - dir = NULL;
> -
> - /* try "${XDG_RUNTIME_DIR}/wine" */
> -
>
> +#ifdef HAVE_SECURE_GETENV
> - server_root_dir = secure_getenv( "XDG_RUNTIME_DIR" );

I don't see how `secure_getenv` is useful.

1. Wine is not intended to be run as root, or on behalf of another Unix user.
2. There are plenty of other environment variables that directly influence Wine's behavior, to the point of arbitrary code execution.

[Jinoh Kang (＠iamahuman)]

Jinoh Kang (@iamahuman) commented about server/request.c:
> -
>
> /* create a directory and check its permissions */
>  static void create_dir( const char *name, struct stat *st )
>  {
> - if (lstat( name, st ) == -1)
> - int r = try_dir( name, st, 1 );
> - switch( r )
> {
> - case 0: return;
> - case 1:
> -
>    r = 0;
> -
>    if (errno != ENOENT)
>        fatal_error( "lstat %s: %s\n", name, strerror( errno ));
> -
>    if (mkdir( name, 0700 ) == -1 && errno != EEXIST)
> -
>    if ((mkdir( name, 0700 ) == -1) && (errno != EEXIST))

Please avoid gratuitous changes.

[Konstantin Demin (＠rockdrilla)]

On Mon Jul 24 14:28:55 2023 +0000, Jinoh Kang wrote:
> I don't see how `secure_getenv` is useful.
> - Wine is not intended to be run as root, or on behalf of another Unix user.
> - There are plenty of other environment variables that directly
>
> influence Wine's behavior, to the point of arbitrary code execution.

1. There're other conditions of "secure execution" according to `secure_getenv(3)` - e.g. capabilities and/or LSM.
2. I'm aware of it: simple command `git grep -F getenv ':!**/tests/*' | sed -En 's/^.+getenv\s*(\s*"([^"]+)"\s*.*$/\1/p' | sort -uV | tee /dev/stderr | wc -l` shows at least 52 variables.

If `getenv` is preferred more than `secure_getenv` - let me know. I'll revert changes to configure.ac and related files.

[PATCH v5 0/2] MR3381: server: Try isolate wineserver directory.

[Konstantin Demin (＠rockdrilla)]

On Mon Jul 24 14:33:23 2023 +0000, Jinoh Kang wrote:
> Please avoid gratuitous changes.

Got it.