From c62473a098cbe3072a95a20ad240f5ae81e6760a Mon Sep 17 00:00:00 2001 From: Franco Garbini Date: Mon, 15 Jun 2026 19:11:15 -0300 Subject: [PATCH 1/2] Refactor XFCE container startup for POSIX compliance, secure env forwarding, and fail-safe execution Harden XFCE session launcher for DroidSpaces - Add set -eu and set -a/+a for safe, fail-fast env-file sourcing - Always apply DISPLAY/PulseAudio/virgl setup (no longer skipped when the env file exists) - Add safe_chown to skip symlinks when chowning sockets in shared /tmp - Assign explicit user:GID ownership via id -g (POSIX/BusyBox-safe, replaces non-POSIX "chown user:") - Validate that XFCE_USER exists and that its GID resolves - Whitelist forwarded env vars to strict POSIX identifier names - Detect util-linux su -w via anchored --help match; add quote-escaped EXPORTS fallback for BusyBox/Alpine su - Guard XFCE_USER with ${XFCE_USER:-} so set -u does not abort when unset - Warn explicitly when falling back to root --- scripts/xfce-start | 121 ++++++++++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 34 deletions(-) diff --git a/scripts/xfce-start b/scripts/xfce-start index 029b6af..8cf4073 100644 --- a/scripts/xfce-start +++ b/scripts/xfce-start @@ -1,56 +1,109 @@ #!/bin/sh +set -eu ENV_FILE=/run/droidspaces.env CONFIG=/run/droidspaces/container.config -# 1. Parse Initial Environment Variables +# Export variables from the environment file if it exists. if [ -f "$ENV_FILE" ]; then + set -a . "$ENV_FILE" -else - export DISPLAY=:5 - if grep -q 'enable_pulseaudio=1' "$CONFIG" 2>/dev/null; then - export PULSE_SERVER=unix:/tmp/.pulse-socket - fi - if grep -q 'enable_virgl=1' "$CONFIG" 2>/dev/null; then - export GALLIUM_DRIVER=virpipe - fi + set +a +fi + +# Set default local X11 display if undefined. +: "${DISPLAY:=:5}" +export DISPLAY + +# Apply audio and graphics configurations based on container settings. +if grep -q 'enable_pulseaudio=1' "$CONFIG" 2>/dev/null; then + : "${PULSE_SERVER:=unix:/tmp/.pulse-socket}" + export PULSE_SERVER +fi +if grep -q 'enable_virgl=1' "$CONFIG" 2>/dev/null; then + : "${GALLIUM_DRIVER:=virpipe}" + export GALLIUM_DRIVER fi -# 2. Check User and Launch Desktop -if [ -n "$XFCE_USER" ] && [ "$XFCE_USER" != "root" ]; then +# Extract the display number to define X11 socket and lock paths. +DISPLAY_NUM=$(printf '%s' "$DISPLAY" | sed 's/^://; s/\..*$//') +X11_SOCKET="/tmp/.X11-unix/X${DISPLAY_NUM}" +X11_LOCK="/tmp/.X${DISPLAY_NUM}-lock" - # Fetch the target user's numeric UID and prepare XDG runtime dir - USER_UID=$(id -u "$XFCE_USER") +# Helper: Safely change ownership of existing files/sockets without following symlinks. +safe_chown() { + if [ -e "$2" ] && [ ! -L "$2" ]; then + chown "$1" "$2" + fi +} + +# Configure environment and launch desktop for a non-root user. +if [ -n "${XFCE_USER:-}" ] && [ "$XFCE_USER" != "root" ]; then + if ! USER_UID=$(id -u "$XFCE_USER" 2>/dev/null); then + echo "Error: XFCE_USER '$XFCE_USER' does not exist" >&2 + exit 1 + fi + + # Resolve the numeric primary GID for POSIX-compliant ownership assignment. + if ! USER_GID=$(id -g "$XFCE_USER" 2>/dev/null); then + echo "Error: cannot resolve GID for '$XFCE_USER'" >&2 + exit 1 + fi + OWNER="$XFCE_USER:$USER_GID" TARGET_XDG="/run/user/$USER_UID" - # Unconditionally create/heal the runtime directory and force correct permissions + # Initialize and secure the user's XDG runtime directory. mkdir -p "$TARGET_XDG" - chown "$XFCE_USER" "$TARGET_XDG" + chown "$OWNER" "$TARGET_XDG" chmod 700 "$TARGET_XDG" - # Grant ownership of sockets and X11 lock file to the target user - [ -S /tmp/.X11-unix/X5 ] && chown "$XFCE_USER" /tmp/.X11-unix/X5 - [ -f /tmp/.X5-lock ] && chown "$XFCE_USER" /tmp/.X5-lock - [ -S /tmp/.pulse-socket ] && chown "$XFCE_USER" /tmp/.pulse-socket - [ -S /tmp/.virgl_test ] && chown "$XFCE_USER" /tmp/.virgl_test + # Assign socket ownership to the user. + safe_chown "$OWNER" "$X11_SOCKET" + safe_chown "$OWNER" "$X11_LOCK" + safe_chown "$OWNER" /tmp/.pulse-socket + safe_chown "$OWNER" /tmp/.virgl_test + + # Build a comma-separated list of forwardable env vars: valid POSIX identifier + # names, minus session vars that su -l should set itself. - # Build dynamic env whitelist, excluding root-specific or session-poisoning vars - SAFE_ENV=$(env | awk -F= '{print $1}' \ + SAFE_ENV=$(env | sed 's/=.*//' \ + | grep -E '^[a-zA-Z_][a-zA-Z0-9_]*$' \ | grep -vE '^(HOME|USER|LOGNAME|PWD|SHELL|XDG_RUNTIME_DIR|MAIL|SHLVL|_)$' \ | tr '\n' ',' | sed 's/,$//') - # Drop privileges and launch XFCE under a clean login shell - exec su -l -w "$SAFE_ENV" "$XFCE_USER" -c ' - export XDG_RUNTIME_DIR='"$TARGET_XDG"' - exec /usr/bin/startxfce4' -else - # Warn and fall back to root execution - if [ "$XFCE_USER" = "root" ]; then - echo "Warning: XFCE_USER=root, running desktop as root" >&2 + # Launch XFCE using util-linux 'su' if the whitelist flag (-w) is supported. + if su --help 2>&1 | grep -Eq -- '(^|[[:space:]])(-w|--whitelist-environment)([[:space:]]|,|$)'; then + exec su -l -w "$SAFE_ENV" "$XFCE_USER" -c ' + export XDG_RUNTIME_DIR='"$TARGET_XDG"' + exec /usr/bin/startxfce4' fi - mkdir -p /run/user/0 - chmod 700 /run/user/0 - export XDG_RUNTIME_DIR=/run/user/0 - exec /usr/bin/startxfce4 + # Fallback for BusyBox/Alpine 'su' (no -w). Names are already validated as POSIX + # identifiers; the single-quote escaping below keeps values injection-safe -- keep it. + + EXPORTS="export XDG_RUNTIME_DIR='$TARGET_XDG';" + OLD_IFS=$IFS + IFS=, + for var in $SAFE_ENV; do + if [ -z "$var" ]; then + continue + fi + eval "val=\${$var:-}" + esc=$(printf '%s' "$val" | sed "s/'/'\\\\''/g") + EXPORTS="$EXPORTS export $var='$esc';" + done + IFS=$OLD_IFS + exec su -l "$XFCE_USER" -c "$EXPORTS exec /usr/bin/startxfce4" fi + +# Fallback: Configure environment and launch desktop as root. +if [ "${XFCE_USER:-}" = "root" ]; then + echo "Warning: XFCE_USER=root, running desktop as root" >&2 +else + echo "Warning: XFCE_USER is unset or empty, falling back to root" >&2 +fi + +mkdir -p /run/user/0 +chmod 700 /run/user/0 +export XDG_RUNTIME_DIR=/run/user/0 +exec /usr/bin/startxfce4 From 827bab99cd25b258a10b9641d8be3dad91740067 Mon Sep 17 00:00:00 2001 From: Franco Garbini Date: Mon, 15 Jun 2026 20:05:07 -0300 Subject: [PATCH 2/2] Improved and fixed bugs Remove su -l in the whitelist path; -l reset the environment and silently defeated -w. Single-quote XDG_RUNTIME_DIR inside every su -c string to prevent breakage on special characters. Replace eval in the BusyBox fallback with printenv to remove a code-injection vector. Gate PulseAudio/VirGL socket chown behind their config flags so stale sockets aren't re-owned. Split the whitelist launch into explicit branches instead of a fragile ${VAR:+...} expansion that mangled -w args. Guard su --help with || true so a non-zero exit doesn't abort under set -e. safe_chown now handles dangling symlinks and uses chown -h (no link following). Use numeric UID:GID for ownership consistently. Fix DISPLAY parsing so TCP displays (host:5) resolve the socket path correctly. --- scripts/xfce-start | 75 ++++++++++++++++++++++++++-------------------- 1 file changed, 42 insertions(+), 33 deletions(-) diff --git a/scripts/xfce-start b/scripts/xfce-start index 8cf4073..919868a 100644 --- a/scripts/xfce-start +++ b/scripts/xfce-start @@ -4,99 +4,108 @@ set -eu ENV_FILE=/run/droidspaces.env CONFIG=/run/droidspaces/container.config -# Export variables from the environment file if it exists. +# Load environment overrides from the env file if present. if [ -f "$ENV_FILE" ]; then set -a . "$ENV_FILE" set +a fi -# Set default local X11 display if undefined. +# Default the X11 display. : "${DISPLAY:=:5}" export DISPLAY -# Apply audio and graphics configurations based on container settings. +# Enable PulseAudio and VirGL based on container config. +PULSE_ENABLED=0 +VIRGL_ENABLED=0 if grep -q 'enable_pulseaudio=1' "$CONFIG" 2>/dev/null; then + PULSE_ENABLED=1 : "${PULSE_SERVER:=unix:/tmp/.pulse-socket}" export PULSE_SERVER fi if grep -q 'enable_virgl=1' "$CONFIG" 2>/dev/null; then + VIRGL_ENABLED=1 : "${GALLIUM_DRIVER:=virpipe}" export GALLIUM_DRIVER fi -# Extract the display number to define X11 socket and lock paths. -DISPLAY_NUM=$(printf '%s' "$DISPLAY" | sed 's/^://; s/\..*$//') +# Derive the X11 socket and lock paths from the display number, +# ignoring any host prefix and screen suffix. +DISPLAY_NUM=$(printf '%s' "$DISPLAY" | sed 's/^.*://; s/\..*$//') X11_SOCKET="/tmp/.X11-unix/X${DISPLAY_NUM}" X11_LOCK="/tmp/.X${DISPLAY_NUM}-lock" -# Helper: Safely change ownership of existing files/sockets without following symlinks. +# Chown a path if it exists, acting on the link itself rather than its target. safe_chown() { - if [ -e "$2" ] && [ ! -L "$2" ]; then - chown "$1" "$2" + if [ -e "$2" ] || [ -L "$2" ]; then + chown -h "$1" "$2" fi } -# Configure environment and launch desktop for a non-root user. +# Run the desktop as a non-root user when one is configured. if [ -n "${XFCE_USER:-}" ] && [ "$XFCE_USER" != "root" ]; then if ! USER_UID=$(id -u "$XFCE_USER" 2>/dev/null); then echo "Error: XFCE_USER '$XFCE_USER' does not exist" >&2 exit 1 fi - # Resolve the numeric primary GID for POSIX-compliant ownership assignment. if ! USER_GID=$(id -g "$XFCE_USER" 2>/dev/null); then echo "Error: cannot resolve GID for '$XFCE_USER'" >&2 exit 1 fi - OWNER="$XFCE_USER:$USER_GID" + + OWNER="$USER_UID:$USER_GID" TARGET_XDG="/run/user/$USER_UID" - # Initialize and secure the user's XDG runtime directory. + # Create the user's private XDG runtime directory. mkdir -p "$TARGET_XDG" chown "$OWNER" "$TARGET_XDG" chmod 700 "$TARGET_XDG" - # Assign socket ownership to the user. + # Hand the X11 socket and lock to the user. safe_chown "$OWNER" "$X11_SOCKET" safe_chown "$OWNER" "$X11_LOCK" - safe_chown "$OWNER" /tmp/.pulse-socket - safe_chown "$OWNER" /tmp/.virgl_test - # Build a comma-separated list of forwardable env vars: valid POSIX identifier - # names, minus session vars that su -l should set itself. + # Hand over optional service sockets only when enabled. + if [ "$PULSE_ENABLED" -eq 1 ]; then + safe_chown "$OWNER" /tmp/.pulse-socket + fi + if [ "$VIRGL_ENABLED" -eq 1 ]; then + safe_chown "$OWNER" /tmp/.virgl_test + fi - SAFE_ENV=$(env | sed 's/=.*//' \ + # Collect forwardable env var names, excluding system/login variables. + SAFE_ENV=$(env | awk -F= '{print $1}' \ | grep -E '^[a-zA-Z_][a-zA-Z0-9_]*$' \ | grep -vE '^(HOME|USER|LOGNAME|PWD|SHELL|XDG_RUNTIME_DIR|MAIL|SHLVL|_)$' \ | tr '\n' ',' | sed 's/,$//') - # Launch XFCE using util-linux 'su' if the whitelist flag (-w) is supported. - if su --help 2>&1 | grep -Eq -- '(^|[[:space:]])(-w|--whitelist-environment)([[:space:]]|,|$)'; then - exec su -l -w "$SAFE_ENV" "$XFCE_USER" -c ' - export XDG_RUNTIME_DIR='"$TARGET_XDG"' - exec /usr/bin/startxfce4' + # Prefer util-linux su, which can whitelist the environment with -w. + if { su --help 2>&1 || true; } | grep -Eq -- '(^|[[:space:]])(-w|--whitelist-environment)([[:space:]]|,|$)'; then + if [ -n "$SAFE_ENV" ]; then + exec su -w "$SAFE_ENV" "$XFCE_USER" -c \ + "export XDG_RUNTIME_DIR='$TARGET_XDG'; exec /usr/bin/startxfce4" + else + exec su "$XFCE_USER" -c \ + "export XDG_RUNTIME_DIR='$TARGET_XDG'; exec /usr/bin/startxfce4" + fi fi - # Fallback for BusyBox/Alpine 'su' (no -w). Names are already validated as POSIX - # identifiers; the single-quote escaping below keeps values injection-safe -- keep it. - + # BusyBox/Alpine su lacks -w: rebuild the safe env as quoted exports. EXPORTS="export XDG_RUNTIME_DIR='$TARGET_XDG';" OLD_IFS=$IFS IFS=, - for var in $SAFE_ENV; do - if [ -z "$var" ]; then - continue - fi - eval "val=\${$var:-}" + for var in ${SAFE_ENV:-}; do + [ -z "$var" ] && continue + val=$(printenv "$var" || true) esc=$(printf '%s' "$val" | sed "s/'/'\\\\''/g") EXPORTS="$EXPORTS export $var='$esc';" done IFS=$OLD_IFS - exec su -l "$XFCE_USER" -c "$EXPORTS exec /usr/bin/startxfce4" + exec su "$XFCE_USER" -c "$EXPORTS exec /usr/bin/startxfce4" fi -# Fallback: Configure environment and launch desktop as root. +# No usable non-root user: run the desktop as root. if [ "${XFCE_USER:-}" = "root" ]; then echo "Warning: XFCE_USER=root, running desktop as root" >&2 else