[Pkg-shadow-devel] Bug#505071: Bug#505071: closed ... fixed in shadow 1:4.1.3-1

[Nicolas François]

Hello,

On Sun, Apr 19, 2009 at 09:06:38AM +1000, psz at maths.usyd.edu.au wrote:
> 
> Thanks. New patches (replacing my previous ones) below, including the
> move of endpwent(), and more verbose comments. The patch for login.c
> is essential. The patch for utmp.c is mostly "as you wish"; there is a
> missed endutent() there, am not sure whether correct or how important.

I've added the missing endutent / endutxent.

> Seems to me that su.c needs endgrent() for when SU_WHEEL_ONLY (this is
> not a security issue).
> 
> In newgrp.c also, endpwent() needs to be moved to before the UID/GID
> change (as was in login.c); I do not give patches. - Do these moves
> warrant a DSA?

I don't think there is a need for a DSA.
With a glibc, the user/group databases are opened with the O_CLOEXEC flag.
The file descriptor will not be available to the exec'ed processes.

Regarding login.c, I think it is still fine to "close" the files (with
endspent et al.), in case another implementation is used.

This still have to be checked / done.



Here are some comments on the patches:

>  	amroot = (getuid () == 0);
>  	Prog = Basename (argv[0]);
> +	/*
> +	 * PSz 18 Apr 09 Will this "work" unless we are root?
> +	 * Should we test
> +	if (geteuid() != 0) {
> +		fprintf (stderr, _("%s: Cannot possibly work without effective root\n"), Prog);
> +		exit (1);
> +	}
> +	 * ?
> +	 * Debian /bin/login is not setuid, cannot work without amroot.
> +	 */

Debian used to have a setuid /bin/login. Some people may still want to
have it setuid, so I would prefer to avoid requiring to run login as root.

>  	 * even if your getty is broken, or if something corrupts utmp,
>  	 * but users must "exec login" which will use the existing utmp
>  	 * entry (will not overwrite remote hostname).  --marekm
> +	 * PSz 18 Apr 09 As seen in
> +	 *   http://bugs.debian.org/505071
> +	 *   http://bugs.debian.org/505271
> +	 * we cannot trust utmp: should set our own, new entry.
> +	 * On Debian at least, we will never succeed without amroot
> +	 * (should have exited already?), should never be picky...
>  	 */
>  	checkutmp (!amroot);

My point would be: In case login is setuid, shall we require that it is
called with "exec login". That would be my preference.

Then, how to enforce this? (note the point is not to enforce this is all
cases, but to make sure regular user will not leave a opened session).

It might be better to change the login/utmp logic to:

	if (!amroot)
		checkutmp (); /* Make sure there is a valid enough entry
		               * in utmp */

	getutmp (&utent);      /* get the sanitized existing utmp entry or
	                        * default entry */
#if USE_UTMPX
	getutmpx (&utxent);
#endif

	...

	setutmp (&utent);
#if USE_UTMPX
	setutmpx (&utxent);
#endif

That could remove the need for __linux__ in utmp.c and would remove the
global utent and utxent.

>  	/*
>  	 * hostname & tty are either set to NULL or their correct values,
>  	 * depending on how much we know.
> +	 * PSz 18 Apr 09 PAM_RHOST and PAM_TTY must be kept safe:
> +	 * PAM_RHOST is used e.g. in pam_rhosts_auth. We only ever set
> +	 * hostname from rflg or hflg not from ut_host, so is safe.
> +	 * PAM_TTY is used e.g. in pam_time and pam_group. We set tty
> +	 * from ttyname not from ut_line, so is safe.
>  	 */

I agree on putting a comment here to avoid setting PAM_RHOST differently in
future.

(The other comments were not meant to appear in the code, right?)

> --- libmisc/utmp.c.old	2008-11-23 10:56:10.000000000 +1100
> +++ libmisc/utmp.c	2009-04-19 08:09:22.000000000 +1000
> @@ -68,17 +68,27 @@
>  		snprintf (full_tty, sizeof full_tty, "/dev/%s", tty);
>  		tty = full_tty;
>  	}
> -
> -	if (   (stat (tty, &by_name) != 0)
> -	    || (fstat (STDIN_FILENO, &by_fd) != 0)) {
> -		return false;
> -	}
> -
> -	if (by_name.st_rdev != by_fd.st_rdev) {
> +	/*
> +	 * PSz 19 Apr 09 Might as well just compare to ttyname():
> +	 * code below may be too complex (or unsafe/raceable).
> +	 */
> +	if (strcmp(tty,ttyname(0))) {
>  		return false;
>  	} else {
>  		return true;
>  	}

Let's do it, but I'm still unsure about the consequences of this.
i.e. was it a feature to be able to have a getty using a device not in
/dev

Let's see if somebody notices and complains.

Best Regards,
-- 
Nekral