Skip Menu |
 

This queue is for tickets about the IO-Async CPAN distribution.

Report information
The Basics
Id: 72407
Status: resolved
Priority: 0/
Queue: IO-Async

People
Owner: Nobody in particular
Requestors: john [...] nixnuts.net
Cc:
AdminCc:

Bug Information
Severity: Important
Broken in: (no value)
Fixed in: 0.45



Download (untitled) / with headers
text/plain 969b
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); }
Download (untitled) / with headers
text/plain 388b
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
Download (untitled) / with headers
text/plain 988b
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
Somewhat fixed. May not be bullet-proof but I think it's better than it was. -- Paul Evans
Subject: rt72407.diff
Download rt72407.diff
text/x-diff 2.4k
=== 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() - $!"; } } }
Fixed in 0.45. -- Paul Evans


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.