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

[Paul Szabo]

Dear Nicolas,

> I've added the missing endutent / endutxent.

Thanks.

>> 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.

Once you change UID, the user may interact with the process; the file
descriptor must be closed before the user gets a chance. Anyway you must
not rely on O_CLOEXEC that you did not set yourself. (Admittedly, the
window of opportunity between setuid and endspent is small, and it may
be hard to interact with a process that just gave up privileges so is
not yet "fully" user. But, races are easy to win.)

There may be no need for DSA, because in fact login never had a file
descriptor for /etc/shadow. Other (non-Debian) machines may have a
different libc and not so lucky.

> 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.

The above would check EUID, would allow that (if uncommented).

> 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.

Sorry but I do not know. (I guess that would be OK, with plenty of
checking/sanitizing after getutmp(), not assuming that it would return
the just-checked entry, but double-checking to make sure.)

>>  	/*
>>  	 * 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.

Thanks.

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

I do not know. I like comments, for when (others?) reviewing the code.

>> --- 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

Thanks.

>> Now testing, seems that just before the endspent() etc calls, login has
>> a file descriptor open on /etc/passwd but does not have one for
>> /etc/shadow. Seems there is no security issue. (Is this weird behaviour
>> in libc?)
>
> There are no call to setspent or getspent in shadow, so I'm not really
> surprised.

But... I do not see setpwent() either within /bin/login sources (though
is in the binary), and /etc/passwd is open; the man pages suggest that
both /etc/passwd and /etc/shadow should be open "internally" until
endXXent, even without explicit setXXent.

>> Since I do not know how getspent() or endspent() work, I now wonder
>> whether chunks of /etc/shadow (other than the line for right user) could
>> be found in process memory, before or after endspent(). Have so far
>> failed to read /proc/self/mem in my test program, and wonder if that
>> feature works in my kernel...
>
> Only getspnam would have to be checked.
> The problem probably depends on the libc.

Yes. Would be nice for getspnam() to scrub memory areas; surely
endspent() should. (Tried looking in libc sources, and got confused...)
Since login uses these, it should make sure they do the "right thing"
(maybe by doing the memory scrubbing itself).

Cheers, Paul

Paul Szabo   psz at maths.usyd.edu.au   http://www.maths.usyd.edu.au/u/psz/
School of Mathematics and Statistics   University of Sydney    Australia