Re: [screen-devel] screen review

[Steve Grubb]

On Monday, January 17, 2011 08:42:22 am Miroslav Lichvar wrote:
> patch 1: not checking return code from setgid/setuid can lead to
> vulnerabilities, see CVE-2006-2607

Yes and I think under at least one code path and perhaps some compile options, 
a 
failure in setuid would let an arbitrary program pointed to from an 
environmental 
variable run with effective uid root.


> patch 5: tty should be checked if it's not a hardlink and it starts with
> /dev, in login.c from util-linux-ng there are comments:
> 
> /* In case login is suid it was possible to use a hardlink as stdin
>    and exploit races for a local root exploit. (Wojciech Purczynski). */
> /* More precisely, the problem is  ttyn := ttyname(0); ...; chown(ttyn);
>    here ttyname() might return "/tmp/x", a hardlink to a pseudotty. */
> /* All of this is a problem only when login is suid, which it isnt. */
> 
> patches 3, 4, 6 probably fix warnings from a static code analyzer.

Yes, but note that the utmp patch fixed a certain segfault in a couple places:

You see something like this in many places:
if (pututslot(D_loginslot, &D_utmp_logintty, D_loginhost, (struct win *)0)

With NULL being passed for the window, the dereference of wi->w_ptyfd should 
not be 
done without first checking that wi != NULL.

> Not sure about patch 2 (setting PAM_TTY item).

Pam logs audit events. We need the TTY that an authentication request comes 
from for 
cases where there are concurrent sessions. Additionally, pam cannot check that 
authentication should be forbidden on certain terminals unless it has the TTY 
passed 
to it. All around this make pam policy and audit logging better.

-Steve