Mailing List Archive

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

djm@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@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@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@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@zip.com.au changed:

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



------- Additional Comments From dtucker@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@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@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@zip.com.au changed:

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



------- Additional Comments From dtucker@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@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@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@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@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@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@zip.com.au changed:

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



------- Additional Comments From dtucker@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.