openssh bugs - Aug 2003 - [Bug 252] Patch for use of /etc/default/login

http://bugzilla.mindrot.org/show_bug.cgi?id=252

dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #212 is|0                           |1
           obsolete|                            |



------- Additional Comments From dtucker at zip.com.au  2003-08-22 14:14 -------
Created an attachment (id=367)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=367&action=view)
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252

djm at mindrot.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
OtherBugsDependingO|                            |627
              nThis|                            |





------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From djm at mindrot.org  2003-09-01 16:41 -------
(From update of attachment 367)
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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From dtucker at zip.com.au  2003-09-04 14:17 -------
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?



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From Robert.Dahlem at siemens.com  2003-09-04 18:14
-------
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. 



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252

dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #367 is|0                           |1
           obsolete|                            |



------- Additional Comments From dtucker at zip.com.au  2003-09-05 10:21 -------
Created an attachment (id=378)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=378&action=view)
Rework based on comments.

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



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From dtucker at zip.com.au  2003-09-13 10:54 -------
What's the verdict on patch #378?  Should it make 3.7?



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From djm at mindrot.org  2003-09-15 09:29 -------
(From update of attachment 378)
>+	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




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252

dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #378 is|0                           |1
           obsolete|                            |



------- Additional Comments From dtucker at zip.com.au  2003-09-15 15:49 -------
Created an attachment (id=397)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=397&action=view)
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?



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From djm at mindrot.org  2003-09-15 16:18 -------
(From update of attachment 397)
>+	/* 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.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From dtucker at zip.com.au  2003-09-15 16:33 -------
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).



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From dtucker at zip.com.au  2003-09-15 19:10 -------
Created an attachment (id=398)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=398&action=view)
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From djm at mindrot.org  2003-09-16 08:22 -------
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".



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252





------- Additional Comments From dtucker at zip.com.au  2003-09-16 11:07 -------
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.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

http://bugzilla.mindrot.org/show_bug.cgi?id=252

dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED



------- Additional Comments From dtucker at zip.com.au  2003-09-16 11:53 -------
Patch applied, thanks.



------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.