[PATCH v5 0/1] MR3381: server: Try isolate wineserver directory.
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 <rockdrilla(a)gmail.com> -- v5: server: Try isolate wineserver directory. https://gitlab.winehq.org/wine/wine/-/merge_requests/3381
From: Konstantin Demin <rockdrilla(a)gmail.com> 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 <rockdrilla(a)gmail.com> --- 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-" ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/3381
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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3381#note_39950
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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3381#note_39951
On Mon Jul 24 14:28:55 2023 +0000, Jinoh Kang wrote: > 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. 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. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/3381#note_39978
On Mon Jul 24 14:33:23 2023 +0000, Jinoh Kang wrote:
Please avoid gratuitous changes. Got it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/3381#note_39979
participants (3)
-
Jinoh Kang (＠iamahuman) -
Konstantin Demin
-
Konstantin Demin (＠rockdrilla)