Bug #72407 for IO-Async: set{uid,gid,groups} failure detection doesn't

Sun Nov 13 19:37:32 2011 john [...] nixnuts.net - Ticket created

The error handling in IO::Async::ChildManager for a failure to setgid or setgroups is defective elsif( $key eq "setgid" ) { $) = $value or die "Cannot setgid('$value') - $!"; } This code will never die on a failure to change the GID since $) is returning a string with "<gid> <supplemental groups>" elsif( $key eq "setgroups" ) { ... my $groups = join( " ", grep { $_ != $gid } @$value ); $) = "$gid $gid $groups" or die "Cannot setgroups('$groups') - $!"; } Same problem here. Both of these boil down to "$) or die" which will never hit the die since $) is a string that always evaluates true. The setuid version of this looks questionable as well. elsif( $key eq "setuid" ) { $> = $value or die "Cannot setuid('$value') - $!"; } This should probably be rewritten to verify the EUID is the one the user actually requested (instead of allowing any non-root EUID) elsif( $key eq "setuid" ) { $> = $value; die unless ($> == $value); }

Mon Nov 14 13:59:27 2011 leonerd-cpan [...] leonerd.org.uk - Subject changed from (no value) to 'set{uid,gid,groups} failure detection doesn't'

Mon Nov 14 14:00:09 2011 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Sun Nov 13 19:37:32 2011, lightsey wrote: Show quoted text

> This should probably be rewritten to verify the EUID is the one the user > actually requested (instead of allowing any non-root EUID) > > elsif( $key eq "setuid" ) { > $> = $value; > die unless ($> == $value); > }

Ahh yes; that all sounds quite plausible. I'll have a go at fixing that up now. Should be in 0.45. -- Paul Evans

Mon Nov 14 14:00:10 2011 The RT System itself - Status changed from 'new' to 'open'

Sat Nov 19 09:16:34 2011 leonerd-cpan [...] leonerd.org.uk - Correspondence added

On Mon Nov 14 14:00:09 2011, PEVANS wrote: Show quoted text

> On Sun Nov 13 19:37:32 2011, lightsey wrote: >

> > This should probably be rewritten to verify the EUID is the one the

> user

> > actually requested (instead of allowing any non-root EUID)

Actually it turns out this is significantly harder than I first imagined, for the following reasons: 1) Core perl does not give any error indication of the success or failure of a setuid/setgid/setgroups operation. 2) Checking with getuid/getgid/getgroups will corrupt the value of errno, so the previous failure will have been lost. This needs preserving. 3) Comparing getgroups() result with the wanted value is nontrivial, as operating systems in practice have variable semantics with respect to group ID uniqueness and sorting. It will require a set comparison operation that ignores duplicates and ordering. I may have a stab at solving this by writing a CPAN module to provide error-safe setuid/setgid/setgroups functions. -- Paul Evans

Sat Nov 19 11:58:40 2011 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Somewhat fixed. May not be bullet-proof but I think it's better than it was. -- Paul Evans

Subject:

rt72407.diff

=== modified file 'lib/IO/Async/ChildManager.pm' --- lib/IO/Async/ChildManager.pm 2011-10-15 12:49:35 +0000 +++ lib/IO/Async/ChildManager.pm 2011-11-19 16:55:17 +0000 @@ -113,6 +113,54 @@ =cut +# Writing to variables of $> and $) have tricky ways to obtain error results +sub setuid +{ + my ( $uid ) = @_; + + $> = $uid; my $saved_errno = $!; + $> == $uid and return 1; + + $! = $saved_errno; + return undef; +} + +sub setgid +{ + my ( $gid ) = @_; + + $) = $gid; my $saved_errno = $!; + $) == $gid and return 1; + + $! = $saved_errno; + return undef; +} + +sub setgroups +{ + my @groups = @_; + + my $gid = $)+0; + # Put the primary GID as the first group in the supplementary list, because + # some operating systems ignore this position, expecting it to indeed be + # the primary GID. + # See + # https://rt.cpan.org/Ticket/Display.html?id=65127 + @groups = grep { $_ != $gid } @groups; + + $) = "$gid $gid " . join " ", @groups; my $saved_errno = $!; + + # No easy way to detect success or failure. Just check that we have all and + # only the right groups + my %gotgroups = map { $_ => 1 } split ' ', "$)"; + + $! = $saved_errno; + $gotgroups{$_}-- or return undef for @groups; + keys %gotgroups or return undef; + + return 1; +} + # Internal constructor sub new { @@ -607,20 +655,13 @@ chdir( $value ) or die "Cannot chdir('$value') - $!"; } elsif( $key eq "setuid" ) { - $> = $value or die "Cannot setuid('$value') - $!"; + setuid( $value ) or die "Cannot setuid('$value') - $!"; } elsif( $key eq "setgid" ) { - $) = $value or die "Cannot setgid('$value') - $!"; + setgid( $value ) or die "Cannot setgid('$value') - $!"; } elsif( $key eq "setgroups" ) { - my $gid = $)+0; - # Put the primary GID as the first group in the supplementary - # list, because some operating systems ignore this position, - # expecting it to indeed be the primary GID. - # See - # https://rt.cpan.org/Ticket/Display.html?id=65127 - my $groups = join( " ", grep { $_ != $gid } @$value ); - $) = "$gid $gid $groups" or die "Cannot setgroups('$groups') - $!"; + setgroups( @$value ) or die "Cannot setgroups() - $!"; } } }

Sat Nov 19 11:58:41 2011 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'patched'

Sun Nov 27 16:14:05 2011 leonerd-cpan [...] leonerd.org.uk - Correspondence added

Fixed in 0.45. -- Paul Evans

Sun Nov 27 16:14:05 2011 The RT System itself - Status changed from 'patched' to 'open'

Sun Nov 27 16:14:05 2011 leonerd-cpan [...] leonerd.org.uk - Status changed from 'open' to 'resolved'

Sun Nov 27 16:16:11 2011 leonerd-cpan [...] leonerd.org.uk - Fixed in 0.45 added