Created attachment 6514 patch file xflock4 clobbers the PATH environment variable with a hardcoded value. /bin and /usr/bin may be common locations to find binaries on FHS distros, but it is not always so. I am a maintainer for the GNU GuixSD project, which does not conform to the FHS, and we do not have /usr/bin or anything in /bin except /bin/sh. So, I think the sanest thing to do in this script is not touch PATH at all. It should be properly configured before the xflock4 process is launched. I noticed this bug on 4.12.0. The attached patch is against the current master branch. Thanks!
I think the original idea of setting PATH to a limited 'trusted' list of subdirs was to avoid potential attackers/malwares to drop malicious replacements for xlock/etc in user-writable directories potentially in the user's PATH...
So isn't the solution then that system administer changes PATH so that it does not contain user-writeable directories? Well, in terminal a regular user can change PATH though. I think it would be safer to check in xflock4 that the command is not user-writeable and is owned by root. (I have a shell function for that.) If the command told by an xfconf variable is used for locking, it can be changed by regular user to run some command that might not lock anyway, but supposedly not as harmful command.
(In reply to Jarno Suni from comment #2) > I think it would be safer to check in xflock4 that the command is not > user-writeable and is owned by root. (I have a shell function for that.) Actually this is tricky. The command could be wrapped by e.g. "time ", "dash -c " etc. so how do you find the final wrapped command?
How could you know that the command is not in a removeable drive then?
I think xflock4 could use "command -vp command_name" to get the secure path of a locker command command_name. Would that work in GNU GuixSD, too?
Oh, unfortunately `command -vp` does not work by dash even in Linux, but works by bash. (https://bugs.launchpad.net/ubuntu/+source/dash/+bug/1539932)
`command -pv` or even `command -v` is not required in POSIX 2004 http://stackoverflow.com/a/34572831/4414935 but I think we can use `command -p getconf PATH` to get a reasonable PATH for the script.
Since 4.13, xflock4 query xfconf to know which binary to run, no need to have a list of path in xflock4 directly: https://git.xfce.org/xfce/xfce4-session/tree/scripts/xflock4#n27
But then that means whatever you set in xfconf needs to have the full path to the binary, otherwise xflock might not find it in $PATH :) since forever i carry this patch in xfce4-session OpenBSD port: https://cgit.rhaalovely.net/xfce4/tree/xfce4-session/patches/patch-scripts_xflock4?h=next
So, what can we do ? Add a bigger list of "trusted" paths ? Use the command -p trick (which does not include /usr/local on linux, but works on OpenBSD) ? $ command -p getconf PATH FreeBSD: /usr/bin:/bin:/usr/sbin:/sbin OpenBSD: /usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/local/bin Various Linuxes: /bin:/usr/bin
I wanted to use "command -p getconf PATH" in xflock4 to set the path instead of an hardcoded value. However on FreeBSD this command does not add /usr/local/bin, which is where xflock4 is located… (this is patched in port tree). Or maybe simply use an hardcoded " /usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/local/bin" to try to be compatible with all OS,and let packagers patch if needed.
(In reply to Skunnyk from comment #11) > I wanted to use "command -p getconf PATH" in xflock4 to set the path instead > of an hardcoded value. > However on FreeBSD this command does not add /usr/local/bin, which is where > xflock4 is located… (this is patched in port tree). > > Or maybe simply use an hardcoded " > /usr/bin:/bin:/usr/sbin:/sbin:/usr/X11R6/bin:/usr/local/bin" to try to be > compatible with all OS,and let packagers patch if needed. Yes, on FreeBSD we expand PATH variable. I think it is proper way to do it.
(In reply to Landry Breuil from comment #1) > I think the original idea of setting PATH to a limited 'trusted' list of > subdirs was to avoid potential attackers/malwares to drop malicious > replacements for xlock/etc in user-writable directories potentially in the > user's PATH... Now that there is the xfconf option, is that somehow protected from potential attackers/malwares? If not, playing with the PATH does not help much. Besides it also matters how xflock4 is called. For example I have used a custom xflock4 in /usr/local/bin which directory is checked before /usr/bin in Ubuntu Linux (due to PATH). It might be better not to touch PATH at all to avoid creating false feeling of security.
David Thompson referenced this bugreport in commit fd46109d056237410cad6901272c86f6d55a293c xflock4: Do not override PATH with hardcoded value. https://git.xfce.org/xfce/xfce4-session/commit?id=fd46109d056237410cad6901272c86f6d55a293c
*** Bug 8830 has been marked as a duplicate of this bug. ***