[Pkg-shadow-devel] Bug#505071: Bug#505071: Bug#505071: login tty mis-determination (see bug#332198)

[Nicolas François]

Hi,

Thanks for your answer.
The culprit is now confirmed.

On Mon, Nov 10, 2008 at 08:51:53AM +1100, psz at maths.usyd.edu.au wrote:
> Dear Nicolas (Nekral?),
> 
> > First of all, this issue was already discussed, and the main problem was
> > that we were not able to reproduce it.
> 
> Yes, I am aware of bug #332198.
> 
> > Are you currently able to reproduce it?
> 
> Have not yet attempted to actively reproduce, have observed one
> occurrence of "spontaneous" bad behaviour.
> 
> > That would help us a lot, since this would allow testing instrumentation
> > of login to find the root cause.
> > Would you agree testing some patches?
> 
> Yes, would be happy to test.
> 
> >> I found in my logs (I think first occurrence of such mis-behaviour):
> >> 
> >> Nov  8 05:50:09 rome in.telnetd[21060]: connect from psz at bari.maths.usyd.edu.au (129.78.69.145) 
> >> Nov  8 05:50:12 rome login[21062]: (pam_unix) session opened for user root by (uid=0) 
> >> Nov  8 05:50:12 rome login[21062]: can't stat(`/dev/smb/39'): errno 2  
> >> Nov  8 05:50:12 rome login[21062]: unable to determine TTY name, got /dev/smb/39  
> >> 
> >> Surely that Samba device is wrong for a telnet session...
> >
> > You logged in with telnet, right?
> > Do you know the version of telnet you are using?
> > Do you know if telnet creates a utmp entry before calling login?
> 
> Yes, with telnet; version 0.17-34 (debian etch); surely it cannot
> possibly create utmp (telnet runs on bari, telnetd on rome).

Well, I meant telnetd should have inserted a utmp entry, on teh server
side.

> > What might be happening is that telnet do not create a utmp entry, and an
> > old one from samba is reused (in checkutmp). This should be very rare also
> > because the bug would occur only if the same pid is reused. 
> 
> Yes, I agree that this is a re-use of an old "unclosed" utmp entry.
> (Samba is in the habit of leaving such unclosed entries.) My logs show
> (much earlier than the above-quoted lines):
> 
> Nov  7 00:52:02 rome samba[21062]: Connect IPC_ for smbguest from p706f (p706f.pc.maths.usyd.edu.au, 129.78.223.215) 
> 
> and I did not notice other utmp uses for the same PID in between.
> 
> ---
> 
> Seems to me that the picking of utent in checkutmp by PID (and type?)
> only is naive, should pick by line (or id) also, in fact pick by the
> is_my_tty checks.

I agree with you that the utmp handling in shadow is not clean, and might
have a security implication.

I fear I won't have time to work on it in the next 2/3 weeks.

I think checking for the line might be good if the line is known, as well
as the user if possible.

> File src/login.c has line 87
>   extern struct utmp utent;
> whereas file libmisc/utmp.c has line 48
>   struct utmp utent;
> without extern: is that correct?

I think that's the expected behavior, however, I would prefer to avoid
such hidden communication between the modules.

> ---
> 
> Other comments. Am worried that relying on utmp correctness is a
> security risk: conceptually because group utmp would become
> root-equivalent, and practically because of shenanigans with utmp
> writing e.g. bugs #329156 #330907.
> 
> In file libmisc/chowntty.c :
> - line 51: should the call
>     (stat (tty, &by_name))
>   be changed to lstat? Avoid being fooled by symlinks.
> - line 66: is the check
>     (by_name.st_rdev != by_fd.st_rdev)
>   sufficient: can it be fooled with symlinks or hardlinks?
> - lines 122,123: should chown(tty,...) and chmod(tty,...) be changed to
>   fchown(0,...) and fchmod(0,...)? Avoid being fooled by symlinks and
>   races.
> 
> Seems to me that as things stand, writing a suitable utmp entry, would
> trick login into chowning an arbitrary file. Should I attempt to write
> an exploit/demo?

That would be nice to check if it would be possible to chown /etc/shadow
by cheating utmp.

A fake demo would be nice.
(by "fake demo", I mean that you do not have to find a way to guess the
PID, but can recompile a new login which use an hardcoded utmp entry in
checkutmp; that would be sufficient since we already know the utmp entry
selection is wrong and can be cheated)

I hope is_my_tty protects it, but I did not checked at all the complete
path.

Cheers,
-- 
Nekral