Xpra-org / xpra

Persistent remote applications for X11; screen sharing for X11, MacOS and MSWindows.
https://xpra.org/
GNU General Public License v2.0
1.98k stars 169 forks source link

Password variable --auth=env:name is preserved in Xvfb-for-Xpra-3 process #4252

Closed cherio closed 5 months ago

cherio commented 5 months ago

Describe the bug xpra server clears all its environment (probably for security reasons) but not before it launches Xvfb-for-Xpra-3 process. The variable containing authentication password is clearly visible in the process environment.

To Reproduce Steps to reproduce the behavior:

  1. server command
systemd-run --user -q --unit usr-xpra-service-name \
    --property=ExecStop="/usr/bin/xpra stop :$xpra_disp_num" \
    --property=StandardOutput=append:$log_file \
    --property=StandardError=append:$log_file \
    --property=EnvironmentFile="$xpra_env_file" \
    --property=Environment="xpra_pass=$xpra_pass" \
    --collect \
    /usr/bin/xpra start :$xpra_disp_num --bind-tcp=:$xpra_srv_port --ssl=off \
    --start=/usr/bin/thunar --start=/usr/bin/xfce4-appfinder \
    --pulseaudio=no --speaker=off \
    --mdns=no \
    --dbus-launch=no \
    --auth=env:name=xpra_pass \
    --daemon=no

System Information (please complete the following information):

 - Xpra Server Version:

$ xpra --version xpra v6.1-r35816 (gefa5e2cf65) beta

totaam commented 5 months ago

It's not easy to decide which variables should be sanitized and which ones should not be. The safe approach is to only use a whitelist, but this is bound to break something, somewhere. I would recommend using a different authentication module instead. Otherwise, please submit a PR.

cherio commented 5 months ago

I may be missing something but wouldn't it be safe to assume that the variable whose name referenced as --auth=env:name=VAR should be removed from the environment?

totaam commented 5 months ago

I'm not going to do this sorts of parsing gymnastics, sorry.

cherio commented 5 months ago

I was just responding to

It's not easy to decide which variables should be sanitized

simply pointing to the fact that the variable name that holds authentication password is readily available, as it is explicitly specified. I realize that because I am not familiar with the code, what seems to be obvious and simple on the surface, may not really be that straightforward.

totaam commented 5 months ago

@cherio the vfb code knows nothing about authentication modules, and the authentication modules know nothing about the command line.

Please try e1b5660f6f659d214f073fa086e6efa8764f0f8f