remove support for user/group switching, because it never worked corr…

…ectly, which means that this security feature has become an attack vector itself
mojolicious · Apr 26, 2015 · 1ddab06 · 1ddab06
1 parent 1f1acd2
commit 1ddab06
Show file tree

Hide file tree

Showing 13 changed files with 57 additions and 91 deletions.
diff --git a/Changes b/Changes
@@ -1,5 +1,14 @@
 
 6.10  2015-04-26
+  - Removed support for user/group switching, because it never worked
+    correctly, which means that this security feature has become an attack
+    vector itself.
+  - Removed group and user attributes from Mojo::Server.
+  - Removed setuidgid method from Mojo::Server.
+  - Removed group and user settings from Hypnotoad.
+  - Removed -g/--group and -u/--user options from daemon and prefork commands.
+  - Improved next_tick callbacks to run in the same order in which they were
+    registered.
 
 6.09  2015-04-25
   - Improved HTML Living Standard compliance of Mojo::Parameters. (riche, sri)

diff --git a/lib/Mojo/IOLoop.pm b/lib/Mojo/IOLoop.pm
@@ -476,8 +476,8 @@ Check if event loop is running.
   my $undef = Mojo::IOLoop->next_tick(sub {...});
   my $undef = $loop->next_tick(sub {...});
 
-Invoke callback as soon as possible, but not before returning, always returns
-C<undef>.
+Invoke callback as soon as possible, but not before returning or other
+callbacks that have been registered with this method, always returns C<undef>.
 
   # Perform operation on next reactor tick
   Mojo::IOLoop->next_tick(sub {

diff --git a/lib/Mojo/Reactor.pm b/lib/Mojo/Reactor.pm
@@ -14,7 +14,11 @@ sub detect {
 sub io         { croak 'Method "io" not implemented by subclass' }
 sub is_running { croak 'Method "is_running" not implemented by subclass' }
 
-sub next_tick { shift->timer(0 => @_) and return undef }
+sub next_tick {
+  my ($self, $cb) = @_;
+  $self->timer(0 => \&_next) if push(@{$self->{next_tick}}, $cb) == 1;
+  return undef;
+}
 
 sub one_tick  { croak 'Method "one_tick" not implemented by subclass' }
 sub recurring { croak 'Method "recurring" not implemented by subclass' }
@@ -25,6 +29,11 @@ sub stop      { croak 'Method "stop" not implemented by subclass' }
 sub timer     { croak 'Method "timer" not implemented by subclass' }
 sub watch     { croak 'Method "watch" not implemented by subclass' }
 
+sub _next {
+  my $self = shift;
+  while (my $cb = shift @{$self->{next_tick}}) { $self->$cb }
+}
+
 1;
 
 =encoding utf8
@@ -122,8 +131,8 @@ Check if reactor is running. Meant to be overloaded in a subclass.
 
   my $undef = $reactor->next_tick(sub {...});
 
-Invoke callback as soon as possible, but not before returning, always returns
-C<undef>.
+Invoke callback as soon as possible, but not before returning or other
+callbacks that have been registered with this method, always returns C<undef>.
 
 =head2 one_tick
 

diff --git a/lib/Mojo/Reactor/Poll.pm b/lib/Mojo/Reactor/Poll.pm
@@ -84,7 +84,7 @@ sub remove {
   return !!delete $self->{io}{fileno $remove};
 }
 
-sub reset { delete @{shift()}{qw(io timers)} }
+sub reset { delete @{shift()}{qw(io next_tick timers)} }
 
 sub start {
   my $self = shift;

diff --git a/lib/Mojo/Server.pm b/lib/Mojo/Server.pm
@@ -8,8 +8,7 @@ use Mojo::Util 'md5_sum';
 use POSIX;
 use Scalar::Util 'blessed';
 
-has app => sub { shift->build_app('Mojo::HelloWorld') };
-has [qw(group user)];
+has app           => sub { shift->build_app('Mojo::HelloWorld') };
 has reverse_proxy => sub { $ENV{MOJO_REVERSE_PROXY} };
 
 sub build_app {
@@ -71,29 +70,6 @@ sub new {
 
 sub run { croak 'Method "run" not implemented by subclass' }
 
-sub setuidgid {
-  my $self = shift;
-
-  # Group (make sure secondary groups are reassigned too)
-  if (my $group = $self->group) {
-    $self->_error(qq{Group "$group" does not exist})
-      unless defined(my $gid = getgrnam $group);
-    $self->_error(qq{Can't switch to group "$group": $!})
-      unless ($( = $) = "$gid $gid") && $) eq "$gid $gid" && $( eq "$gid $gid";
-  }
-
-  # User
-  return $self unless my $user = $self->user;
-  $self->_error(qq{User "$user" does not exist})
-    unless defined(my $uid = getpwnam $user);
-  $self->_error(qq{Can't switch to user "$user": $!})
-    unless POSIX::setuid($uid);
-
-  return $self;
-}
-
-sub _error { $_[0]->app->log->error($_[1]) and croak $_[1] }
-
 1;
 
 =encoding utf8
@@ -158,13 +134,6 @@ L<Mojo::Server> implements the following attributes.
 
 Application this server handles, defaults to a L<Mojo::HelloWorld> object.
 
-=head2 group
-
-  my $group = $server->group;
-  $server   = $server->group('users');
-
-Group for server process.
-
 =head2 reverse_proxy
 
   my $bool = $server->reverse_proxy;
@@ -173,13 +142,6 @@ Group for server process.
 This server operates behind a reverse proxy, defaults to the value of the
 C<MOJO_REVERSE_PROXY> environment variable.
 
-=head2 user
-
-  my $user = $server->user;
-  $server  = $server->user('web');
-
-User for the server process.
-
 =head1 METHODS
 
 L<Mojo::Server> inherits all methods from L<Mojo::EventEmitter> and implements
@@ -226,12 +188,6 @@ with default request handling.
 
 Run server. Meant to be overloaded in a subclass.
 
-=head2 setuidgid
-
-  $server = $server->setuidgid;
-
-Set L</"user"> and L</"group"> for process.
-
 =head1 SEE ALSO
 
 L<Mojolicious>, L<Mojolicious::Guides>, L<http://mojolicio.us>.

diff --git a/lib/Mojo/Server/Daemon.pm b/lib/Mojo/Server/Daemon.pm
@@ -30,7 +30,7 @@ sub run {
   my $loop = $self->ioloop;
   my $int = $loop->recurring(1 => sub { });
   local $SIG{INT} = local $SIG{TERM} = sub { $loop->stop };
-  $self->start->setuidgid->ioloop->start;
+  $self->start->ioloop->start;
   $loop->remove($int);
 }
 

diff --git a/lib/Mojo/Server/Hypnotoad.pm b/lib/Mojo/Server/Hypnotoad.pm
@@ -27,9 +27,9 @@ sub configure {
   $prefork->max_clients($c->{clients})   if $c->{clients};
   $prefork->max_requests($c->{requests}) if $c->{requests};
   defined $c->{$_} and $prefork->$_($c->{$_})
-    for qw(accepts backlog graceful_timeout group heartbeat_interval),
+    for qw(accepts backlog graceful_timeout heartbeat_interval),
     qw(heartbeat_timeout inactivity_timeout listen multi_accept pid_file),
-    qw(user workers);
+    qw(workers);
 }
 
 sub run {
@@ -272,13 +272,6 @@ Maximum amount of time in seconds stopping a worker gracefully may take before
 being forced, defaults to the value of
 L<Mojo::Server::Prefork/"graceful_timeout">.
 
-=head2 group
-
-  group => 'staff'
-
-Group name for worker processes, defaults to the value of
-L<Mojo::Server/"group">.
-
 =head2 heartbeat_interval
 
   heartbeat_interval => 3
@@ -346,12 +339,6 @@ L<Mojo::Server::Daemon/"max_requests">.
 Maximum amount of time in seconds a zero downtime software upgrade may take
 before getting canceled, defaults to C<60>.
 
-=head2 user
-
-  user => 'sri'
-
-Username for worker processes, defaults to the value of L<Mojo::Server/"user">.
-
 =head2 workers
 
   workers => 10

diff --git a/lib/Mojo/Server/Prefork.pm b/lib/Mojo/Server/Prefork.pm
@@ -145,11 +145,8 @@ sub _spawn {
   return $self->emit(spawn => $pid)->{pool}{$pid} = {time => steady_time}
     if $pid;
 
-  # Change user/group
-  $self->cleanup(0)->setuidgid;
-
   # Heartbeat messages
-  my $loop     = $self->ioloop;
+  my $loop     = $self->cleanup(0)->ioloop;
   my $finished = 0;
   $loop->on(finish => sub { $finished = 1 });
   weaken $self;

diff --git a/lib/Mojolicious/Command/daemon.pm b/lib/Mojolicious/Command/daemon.pm
@@ -14,12 +14,10 @@ sub run {
   GetOptionsFromArray \@args,
     'b|backlog=i'            => sub { $daemon->backlog($_[1]) },
     'c|clients=i'            => sub { $daemon->max_clients($_[1]) },
-    'g|group=s'              => sub { $daemon->group($_[1]) },
     'i|inactivity-timeout=i' => sub { $daemon->inactivity_timeout($_[1]) },
     'l|listen=s'   => \my @listen,
     'p|proxy'      => sub { $daemon->reverse_proxy(1) },
-    'r|requests=i' => sub { $daemon->max_requests($_[1]) },
-    'u|user=s'     => sub { $daemon->user($_[1]) };
+    'r|requests=i' => sub { $daemon->max_requests($_[1]) };
 
   $daemon->listen(\@listen) if @listen;
   $daemon->run;
@@ -47,7 +45,6 @@ Mojolicious::Command::daemon - Daemon command
                                          SOMAXCONN
     -c, --clients <number>               Maximum number of concurrent
                                          connections, defaults to 1000
-    -g, --group <name>                   Group name for process
     -i, --inactivity-timeout <seconds>   Inactivity timeout, defaults to the
                                          value of MOJO_INACTIVITY_TIMEOUT or 15
     -l, --listen <location>              One or more locations you want to
@@ -58,7 +55,6 @@ Mojolicious::Command::daemon - Daemon command
                                          MOJO_REVERSE_PROXY
     -r, --requests <number>              Maximum number of requests per
                                          keep-alive connection, defaults to 25
-    -u, --user <name>                    Username for process
 
 =head1 DESCRIPTION
 

diff --git a/lib/Mojolicious/Command/prefork.pm b/lib/Mojolicious/Command/prefork.pm
@@ -17,7 +17,6 @@ sub run {
     'b|backlog=i'            => sub { $prefork->backlog($_[1]) },
     'c|clients=i'            => sub { $prefork->max_clients($_[1]) },
     'G|graceful-timeout=i'   => sub { $prefork->graceful_timeout($_[1]) },
-    'g|group=s'              => sub { $prefork->group($_[1]) },
     'I|heartbeat-interval=i' => sub { $prefork->heartbeat_interval($_[1]) },
     'H|heartbeat-timeout=i'  => sub { $prefork->heartbeat_timeout($_[1]) },
     'i|inactivity-timeout=i' => sub { $prefork->inactivity_timeout($_[1]) },
@@ -26,7 +25,6 @@ sub run {
     'P|pid-file=s'     => sub { $prefork->pid_file($_[1]) },
     'p|proxy'          => sub { $prefork->reverse_proxy(1) },
     'r|requests=i'     => sub { $prefork->max_requests($_[1]) },
-    'u|user=s'         => sub { $prefork->user($_[1]) },
     'w|workers=i'      => sub { $prefork->workers($_[1]) };
 
   $prefork->listen(\@listen) if @listen;
@@ -58,7 +56,6 @@ Mojolicious::Command::prefork - Prefork command
     -c, --clients <number>               Maximum number of concurrent
                                          connections, defaults to 1000
     -G, --graceful-timeout <seconds>     Graceful timeout, defaults to 20.
-    -g, --group <name>                   Group name for process
     -I, --heartbeat-interval <seconds>   Heartbeat interval, defaults to 5
     -H, --heartbeat-timeout <seconds>    Heartbeat timeout, defaults to 20
     -i, --inactivity-timeout <seconds>   Inactivity timeout, defaults to the
@@ -75,7 +72,6 @@ Mojolicious::Command::prefork - Prefork command
                                          MOJO_REVERSE_PROXY
     -r, --requests <number>              Maximum number of requests per
                                          keep-alive connection, defaults to 25
-    -u, --user <name>                    Username for process
     -w, --workers <number>               Number of workers, defaults to 4
 
 =head1 DESCRIPTION

diff --git a/t/mojo/hypnotoad.t b/t/mojo/hypnotoad.t
@@ -24,7 +24,6 @@ use Mojo::Util qw(slurp spurt);
     backlog            => 43,
     clients            => 1,
     graceful_timeout   => 23,
-    group              => 'testers',
     heartbeat_interval => 7,
     heartbeat_timeout  => 9,
     inactivity_timeout => 5,
@@ -34,28 +33,25 @@ use Mojo::Util qw(slurp spurt);
     proxy              => 1,
     requests           => 3,
     upgrade_timeout    => 45,
-    user               => 'tester',
     workers            => 7
   };
   is $hypnotoad->upgrade_timeout, 60, 'right default';
   $hypnotoad->configure('test');
   is_deeply $hypnotoad->prefork->listen, ['http://*:8080'], 'right value';
   $hypnotoad->configure('myserver');
-  is $hypnotoad->prefork->accepts,            13,        'right value';
-  is $hypnotoad->prefork->backlog,            43,        'right value';
-  is $hypnotoad->prefork->graceful_timeout,   23,        'right value';
-  is $hypnotoad->prefork->group,              'testers', 'right value';
-  is $hypnotoad->prefork->heartbeat_interval, 7,         'right value';
-  is $hypnotoad->prefork->heartbeat_timeout,  9,         'right value';
-  is $hypnotoad->prefork->inactivity_timeout, 5,         'right value';
+  is $hypnotoad->prefork->accepts,            13, 'right value';
+  is $hypnotoad->prefork->backlog,            43, 'right value';
+  is $hypnotoad->prefork->graceful_timeout,   23, 'right value';
+  is $hypnotoad->prefork->heartbeat_interval, 7,  'right value';
+  is $hypnotoad->prefork->heartbeat_timeout,  9,  'right value';
+  is $hypnotoad->prefork->inactivity_timeout, 5,  'right value';
   is_deeply $hypnotoad->prefork->listen, ['http://*:8081'], 'right value';
   is $hypnotoad->prefork->max_clients,  1,              'right value';
   is $hypnotoad->prefork->max_requests, 3,              'right value';
   is $hypnotoad->prefork->multi_accept, 16,             'right value';
   is $hypnotoad->prefork->pid_file,     '/foo/bar.pid', 'right value';
   ok $hypnotoad->prefork->reverse_proxy, 'reverse proxy enabled';
-  is $hypnotoad->prefork->user,          'tester', 'right value';
-  is $hypnotoad->prefork->workers,       7, 'right value';
+  is $hypnotoad->prefork->workers, 7, 'right value';
   is $hypnotoad->upgrade_timeout, 45, 'right value';
 }
 

diff --git a/t/mojo/reactor_ev.t b/t/mojo/reactor_ev.t
@@ -148,6 +148,16 @@ ok !$recurring, 'recurring was not triggered again';
 my $reactor2 = Mojo::Reactor::EV->new;
 is ref $reactor2, 'Mojo::Reactor::Poll', 'right object';
 
+# Ordered next_tick
+my $result = [];
+$reactor->next_tick(sub { push @$result, 1 });
+$reactor->next_tick(sub { push @$result, 2 });
+$reactor->next_tick(sub { push @$result, 3 });
+$reactor->next_tick(sub { push @$result, 4 });
+$reactor->next_tick(sub { push @$result, 5 });
+$reactor->start;
+is_deeply $result, [1, 2, 3, 4, 5], 'right result';
+
 # Reset while watchers are active
 $writable = undef;
 $reactor->io($_ => sub { ++$writable and shift->reset })->watch($_, 0, 1)

diff --git a/t/mojo/reactor_poll.t b/t/mojo/reactor_poll.t
@@ -145,6 +145,16 @@ ok !$recurring, 'recurring was not triggered again';
 my $reactor2 = Mojo::Reactor::Poll->new;
 is ref $reactor2, 'Mojo::Reactor::Poll', 'right object';
 
+# Ordered next_tick
+my $result = [];
+$reactor->next_tick(sub { push @$result, 1 });
+$reactor->next_tick(sub { push @$result, 2 });
+$reactor->next_tick(sub { push @$result, 3 });
+$reactor->next_tick(sub { push @$result, 4 });
+$reactor->next_tick(sub { push @$result, 5 });
+$reactor->start;
+is_deeply $result, [1, 2, 3, 4, 5], 'right result';
+
 # Reset while watchers are active
 $writable = undef;
 $reactor->io($_ => sub { ++$writable and shift->reset })->watch($_, 0, 1)