[PATCH] pt_chown: Clear any signal mask inherited from the parent process.

[Geoffrey Thomas]

On Sun, 17 May 2015, Mike Frysinger wrote:

> On 17 May 2015 16:02, Geoffrey Thomas wrote:
>> If grantpt() is called from a thread that is masking signals (for
>> instance, from a program using signalfd or using a dedicated
>> signal-handling thread), then thas mask will get inherited to pt_chown.
>> This means that signals like SIGINT will not interrupt grantpt(), so if it
>> hangs (e.g., because getgrnam("tty") hangs on a remote name service),
>> Ctrl-C will terminate the parent process but leave grantpt() around. Since
>> it's setuid, it's hard to kill any other way.
>
> recent systems (for years now) should be mounting /dev/pts which means pt_chown
> isn't needed at all.  are you using an old distro ?  or is your glibc
> misconfigured ?  considering /dev/pts has been in all the kernel versions we now
> require, maybe we should drop support for pt_chown on at least Linux ?  make it
> a hard failure if you try to enable it there.

Yeah, fair enough -- I found this from an audit of library sources for 
this style of problem, not from actually seeing it break. But the distros 
I use (Debian and Ubuntu) still ship pt_chown, and claim that removing it 
breaks things: https://bugs.debian.org/717544

It's possible the kernel changes mentioned there are resolved. If so, 
dropping pt_chown upstream is probably a fine plan. But it might be a good 
idea to take this patch for kernels without /dev/pts, though.

> also, the code needs to be audited to make sure that sending arbitrary signals
> can't be abused to make it skip security checks or leave things in a bad state.
> rather than unmask all, it might want to unmask one and make sure that one
> results in its immediate death.

Signal handlers get reset on exec, so the only thing a signal can do is 
immediately kill the process, suspend it, or do nothing, all of which are 
fine. The worst thing you can do is kill it between chown and chmod, but 
if pt_chown exits successfully, the unprivileged user owns the tty anyway 
and can chmod it however they like.

>> +  /* Clear any signal mask from the parent process.  */
>> +  sigemptyset(&sigset);
>> +  sigprocmask(SIG_SETMASK, &sigset, NULL);
>
> GNU style says to put a space before the (
> -mike

Oops. Will fix if you end up wanting to take this patch.

-- 
Geoffrey Thomas
https://ldpreload.com
geofft@ldpreload.com