Bug 252

Created attachment 98 [details]
description? ... the proposed patch itself :-)

*** Bug 475 has been marked as a duplicate of this bug. ***

Created attachment 212 [details]
Updated patch for 3.5p1

Created attachment 367 [details]
Update to -current, integrate into configure a little more.

Handles /etc/default/login the same as /etc/login.conf.

Note: this will probably stop scp from working until /usr/local/bin is added to
/etc/default/login since it is no longer added to the default path.

Comment on attachment 367 [details]
Update to -current, integrate into configure a little more.

Some comments:

I think these warnings:

>+	[ if test ! -z "$external_path_file" ; then
>+	AC_MSG_WARN([Make sure the path to scp is in $external_path_file])

should be added to here:

>@@ -2558,8 +2568,8 @@ echo "                   Askpass program
> echo "                      Manual pages: $F"
> echo "                          PID file: $G"
> echo "  Privilege separation chroot path: $H"
>-if test "$USES_LOGIN_CONF" = "yes" ; then
>-echo "   At runtime, sshd will use the path defined in /etc/login.conf"
>+if test ! -z "$external_path_file"; then
>+echo "   At runtime, sshd will use the path defined in $external_path_file"

so users actually get a chance to read them :)

>+static char *
>+child_get_env(char **envp, const char *name)
>+{
>+	u_int i, namelen;
>+
>+	namelen = strlen(name);
>+	for (i = 0; envp[i]; i++) {

KNF says "envp[i] != NULL"

>+	edf_envsize = 10;
>+	edf_env = xmalloc(edf_envsize * sizeof(char *));

Nit: I think that:
	edf_env = xmalloc(edf_envsize * sizeof(*edf_env));

is a generally safer way of allocating arrays.

>+		/*
>+		 * Paranoia check: set at least a standard path
>+		 * if none is set yet.
>+		 */

Nit: This isn't a paranoia check, most platforms don't use /e/d/l

>+		if (child_get_env(env, "PATH") == NULL) {
>+#ifdef SUPERUSER_PATH
>+			child_set_env(&env, &envsize, "PATH", 
>+			    s->pw->pw_uid == 0 ?
>+				SUPERUSER_PATH : _PATH_STDPATH);
>+#else 
>+			child_set_env(&env, &envsize, "PATH", _PATH_STDPATH);
>+#endif /* SUPERUSER_PATH */
>+		}

Maybe it would be better to hack defines.h to set SUPERUSER_PATH to
_PATH_STDPATH in cases where SUPERUSER_PATH isn't already set. That would allow
us to eliminate this #ifdef block entirely.

Robert, regarding this comment: "I rewrote some of the old code to gather at 
least PATH and UMASK."

Is the patch you posted written entirely by yourself?

Darren, child_get_env() is very similar to the one in sshd.c from ssh-1.2.31
read_etc_default_login() and the other patches were written by me. 

Created attachment 378 [details]
Rework based on comments.

Updated based on comments (both here and in email).  Also deleted and rewrote
child_get_env.

What's the verdict on patch #378?  Should it make 3.7?

Comment on attachment 378 [details]
Rework based on comments.


>+	if (*envp == NULL && *envsizep == 0) {
>+		*envp = xmalloc(sizeof(char *));
>+		*envp[0] = NULL;
>+		*envsizep = 1;
>+	}
>+

An expanatory comment here would be good.

>+char *
>+child_get_env(char **env, const char *name)
>+{
>+	int i;
>+	size_t len;
>+
>+	len = strlen(name);
>+	for (i=0; env[i] != NULL; i++)
>+		if (env[i][len] == '=' && strncmp(name, env[i], len) == 0)
>+			return(env[i] + len + 1);

I think the order of this test should be reversed. env[i][len] may be past the
end of the environment string if name is long (e.g. name = "blahblahblah",
env[i] = "a=b").

Otherwise OK

Created attachment 397 [details]
Fix bugs found in testing

Last-minute testing found a couple of (my) nasty bugs:
* if PATH wasn't defined in /etc/default/login, no path was set due to an
unintentionally inverted test in a configure test.
* on platforms without /e/d/l, child_get_env was being called, even though it
was #ifdef'ed out.

I'm tempted to re-enable the part in configure that adds /usr/local/bin (for
scp) to USER_PATH on platforms with /e/d/l.  I think --with-user-path should
still be able to be set at build time, but be overridden if PATH (or SUPATH) is
set in /e/d/l.

Comments?

Comment on attachment 397 [details]
Fix bugs found in testing

>+	/* If we're passed an uninitialized list, allocate a single null
>+	 * entry before continuing */

Nit - this comment is not KNF. 

As to whether or not we should automatically add $SCP_PATH to the $PATH set by
the server - I don't think we should mess with a path explicitly set in a
configuration file.

Oops.  Comment fixed.

I'm not proposing changing the PATH if it's set in the config file, only to the
compiled-in PATH (which is either the default or as specified --with-user-path).

Put another way: I propose maintaining the existing behaviour unless overridden
by PATH specified in /etc/default/login (ie, rule of least surprise).

Created attachment 398 [details]
Preserve existing add-path-to-scp behaviour

Existing compile-time behaviour is:
Use path specified by --with-default-path
else use default path, adding scp path if necessary.

This patch (hopefully!) keeps exactly the same behaviour, except that the path
from /etc/default/login will be used at run time if it's set.

Does this latest patch add the path to scp to an explicitly specified $PATH
(--with-default-path)?

If it doesn't, OK :)

Also, do we need the prototypes in session.h? The don't seem to be used outside
the file. I suggest making the functions "static".

With patch id=398, if the user specifies the path (either --with-default path or
in /etc/default/login) they get exactly what they specify, regardless of where
scp is.

(In comment #14 I assumed that scp's path was also added to --with-default-path,
however that was not correct.)

I'll "static" those functions and commit it.

Patch applied, thanks.

Mass change of RESOLVED bugs to CLOSED