Patching the default PATH of `su`

Patching the default PATH of `su`

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

In the man page of su(1), it says this:

------
The current environment is passed to the new shell. The value of $PATH is reset to
/bin:/usr/bin for normal users, or /sbin:/bin:/usr/sbin:/usr/bin for the superuser.
This may be changed with the ENV_PATH and ENV_SUPATH definitions in /etc/login.defs.
------

This means that `su leo` or `sudo su` give a broken environment on
GuixSD. You have to use `su --login` instead.

Should we use our own values for ENV_PATH and ENV_SUPATH so that this
works out of the box?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Patching the default PATH of `su`

[Ludovic Courtès]

Hello,

Leo Famulari <leo@famulari.name> skribis:

> In the man page of su(1), it says this:
>
> ------
> The current environment is passed to the new shell. The value of $PATH is reset to
> /bin:/usr/bin for normal users, or /sbin:/bin:/usr/sbin:/usr/bin for the superuser.
> This may be changed with the ENV_PATH and ENV_SUPATH definitions in /etc/login.defs.
> ------
>
> This means that `su leo` or `sudo su` give a broken environment on
> GuixSD. You have to use `su --login` instead.
>
> Should we use our own values for ENV_PATH and ENV_SUPATH so that this
> works out of the box?

Probably, yes.  It would be good to check how this affects
mingetty/login, sshd, etc.

Note that libc also has its own default PATH value in <paths.h>:

  /* Default search path. */
  #define	_PATH_DEFPATH	"/usr/bin:/bin"
  /* All standard utilities path. */
  #define	_PATH_STDPATH \
          "/usr/bin:/bin:/usr/sbin:/sbin"

Does ‘su’ rely on this?  In a future rebuild cycle we could change these
values, but /run/current-system/bin wouldn’t work on foreign distros, so
it’s not clear there’s much to gain.

Thanks,
Ludo’.

Re: Patching the default PATH of `su`

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

On Fri, Apr 06, 2018 at 10:01:57AM +0200, Ludovic Courtès wrote:
> Probably, yes.  It would be good to check how this affects
> mingetty/login, sshd, etc.

Okay. I can test the change.

> Note that libc also has its own default PATH value in <paths.h>:
> 
>   /* Default search path. */
>   #define	_PATH_DEFPATH	"/usr/bin:/bin"
>   /* All standard utilities path. */
>   #define	_PATH_STDPATH \
>           "/usr/bin:/bin:/usr/sbin:/sbin"
> 
> Does ‘su’ rely on this?  In a future rebuild cycle we could change these
> values, but /run/current-system/bin wouldn’t work on foreign distros, so
> it’s not clear there’s much to gain.

I don't think `su` uses this, but I'll find out. As you say, it wouldn't
help much on foreign distros. I think the situation with `su` is
different, since it seems inconvenient to use our `su` on a foreign
distro, because it needs to be setuid.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#31112] Patching the default PATH of `su`

[Leo Famulari]

[-- Attachment #1.1: Type: text/plain, Size: 2295 bytes --]

On Fri, Apr 06, 2018 at 10:01:57AM +0200, Ludovic Courtès wrote:
> Probably, yes.  It would be good to check how this affects
> mingetty/login, sshd, etc.

I've attached a patch that sets the variables in /etc/login.defs.

The resulting PATH only includes "system" packages, which are found in
/run/current-system and /run/setuid-programs.

It would be better if it also included the user-specific programs in
~/.guix-profile, but I don't think this is possible without patching
`su` to look up usernames.

Shell variables are not expanded by `su`, so using $HOME doesn't work.
And I'm not sure how to use /var/guix/profiles/per-user without making
`su` look up usernames.

Nevertheless, I think it's an improvement, although maybe it's less
confusing for the PATH to be totally wrong than merely missing the
user's packages. WDYT?

With the attached patch, I tested mingetty and agetty's login, as well
as OpenSSH sshd login, and everything seemed to work — this changes
should have no effect in those cases because it affects non-login shells
only.

> Note that libc also has its own default PATH value in <paths.h>:
> 
>   /* Default search path. */
>   #define	_PATH_DEFPATH	"/usr/bin:/bin"
>   /* All standard utilities path. */
>   #define	_PATH_STDPATH \
>           "/usr/bin:/bin:/usr/sbin:/sbin"
> 
> Does ‘su’ rely on this?  In a future rebuild cycle we could change these
> values, but /run/current-system/bin wouldn’t work on foreign distros, so
> it’s not clear there’s much to gain.

AFAICT, `su` doesn't use this. The relevant code is below; it hard-codes
the fall-back PATHs rather than refer to libc.

------
	/*
	 * Create the PATH environmental variable and export it.
	 */

	cp = getdef_str ((info->pw_uid == 0) ? "ENV_SUPATH" : "ENV_PATH");

	if (NULL == cp) {
		/* not specified, use a minimal default */
		addenv ((info->pw_uid == 0) ? "PATH=/sbin:/bin:/usr/sbin:/usr/bin" : "PATH=/bin:/usr/bin", NULL);
	} else if (strchr (cp, '=')) {
		/* specified as name=value (PATH=...) */
		addenv (cp, NULL);
	} else {
		/* only value specified without "PATH=" */
		addenv ("PATH", cp);
}
------

https://github.com/shadow-maint/shadow/blob/15be89f89d553c06d52453721ee8e9a8433cfdfd/libmisc/setupenv.c#L263

[-- Attachment #1.2: 0001-system-Provide-a-fall-back-PATH-for-non-login-shells.patch --]
[-- Type: text/plain, Size: 1540 bytes --]

From dc5098edd98c85ad45d2e22ca1824a9445dcc36d Mon Sep 17 00:00:00 2001
From: Leo Famulari <leo@famulari.name>
Date: Sun, 8 Apr 2018 14:30:05 -0400
Subject: [PATCH] system: Provide a fall-back PATH for non-login shells started
 with su(1).

* gnu/system.scm (operating-system-etc-service): Provide values for
ENV_PATH and ENV_SUPATH in '/etc/login.defs'.
---
 gnu/system.scm | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/gnu/system.scm b/gnu/system.scm
index 592a0ea58..1cf00aafc 100644
--- a/gnu/system.scm
+++ b/gnu/system.scm
@@ -571,7 +571,16 @@ This is the GNU system.  Welcome.\n")
 (define* (operating-system-etc-service os)
   "Return a <service> that builds containing the static part of the /etc
 directory."
-  (let ((login.defs (plain-file "login.defs" "# Empty for now.\n"))
+  (let ((login.defs
+          (plain-file "login.defs"
+                      (string-append
+                        "# Default paths for non-login shells started by su(1).\n"
+                        "ENV_PATH    /run/setuid-programs:"
+                        "/run/current-system/profile/bin:"
+                        "/run/current-system/profile/sbin\n"
+                        "ENV_SUPATH  /run/setuid-programs:"
+                        "/run/current-system/profile/bin:"
+                        "/run/current-system/profile/sbin\n")))
 
         (issue      (plain-file "issue" (operating-system-issue os)))
         (nsswitch   (plain-file "nsswitch.conf"
-- 
2.17.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[bug#31112] Patching the default PATH of `su`

[Ludovic Courtès]

Hey Leo,

Leo Famulari <leo@famulari.name> skribis:

> I've attached a patch that sets the variables in /etc/login.defs.
>
> The resulting PATH only includes "system" packages, which are found in
> /run/current-system and /run/setuid-programs.
>
> It would be better if it also included the user-specific programs in
> ~/.guix-profile, but I don't think this is possible without patching
> `su` to look up usernames.
>
> Shell variables are not expanded by `su`, so using $HOME doesn't work.
> And I'm not sure how to use /var/guix/profiles/per-user without making
> `su` look up usernames.
>
> Nevertheless, I think it's an improvement, although maybe it's less
> confusing for the PATH to be totally wrong than merely missing the
> user's packages. WDYT?

I think the patch is an improvement.

I don’t see how we could add the user ~/.guix-profile as well,
especially since there’s no central place that parses ‘login.defs’
(Shadow has its own parser in lib/getdef.c).

BTW, for a similar reason, OpenSSH’s ‘scp’ doesn’t work when the remote
machine runs GuixSD, even if OpenSSH is in the user’s profile.

> With the attached patch, I tested mingetty and agetty's login, as well
> as OpenSSH sshd login, and everything seemed to work — this changes
> should have no effect in those cases because it affects non-login shells
> only.

Great.

> AFAICT, `su` doesn't use this. The relevant code is below; it hard-codes
> the fall-back PATHs rather than refer to libc.

OK.

> From dc5098edd98c85ad45d2e22ca1824a9445dcc36d Mon Sep 17 00:00:00 2001
> From: Leo Famulari <leo@famulari.name>
> Date: Sun, 8 Apr 2018 14:30:05 -0400
> Subject: [PATCH] system: Provide a fall-back PATH for non-login shells started
>  with su(1).
>
> * gnu/system.scm (operating-system-etc-service): Provide values for
> ENV_PATH and ENV_SUPATH in '/etc/login.defs'.

So LGTM, thank you!

Ludo’.

bug#31112: Patching the default PATH of `su`

[Leo Famulari]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Mon, Apr 09, 2018 at 10:47:30PM +0200, Ludovic Courtès wrote:
> Leo Famulari <leo@famulari.name> skribis:
> > From dc5098edd98c85ad45d2e22ca1824a9445dcc36d Mon Sep 17 00:00:00 2001
> > From: Leo Famulari <leo@famulari.name>
> > Date: Sun, 8 Apr 2018 14:30:05 -0400
> > Subject: [PATCH] system: Provide a fall-back PATH for non-login shells started
> >  with su(1).
> >
> > * gnu/system.scm (operating-system-etc-service): Provide values for
> > ENV_PATH and ENV_SUPATH in '/etc/login.defs'.
> 
> So LGTM, thank you!

Alright, pushed as e453da132a3482540d2166b23554ef693b2c0c0d

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]