Re: [PATCH 2/2] services.cgi: Fix status/actions on services with name != addon name

[Robin Roevens <robin.roevens@disroot.org>]

[-- Attachment #1: Type: text/plain, Size: 14994 bytes --]

Hi

Michael Tremer schreef op di 04-10-2022 om 10:51 [+0100]:
> Hello,
> 
> I am going to start with the second patch, because that is going to
> be shorter :)
> 
> > On 3 Oct 2022, at 16:27, Robin Roevens <robin.roevens(a)disroot.org>
> > wrote:
> > 
> > * addonctrl's new functionality to control explicit addon services
> > was
> >  implemented.
> > * Change 'Addon' column header to 'Addon Service' to be clear that
> >  it's not addons but services listed here.
> > * Services not matching the name of the addon now display the addon
> >  name between parentheses, so the user knows where the service
> > comes
> >  from.
> > * When no valid runlevel symlink is found by addonctrl for a
> > service,
> >  the 'enable on boot' checkbox is replaced by a small exclamation
> > point
> >  with alt-text "No valid runlevel symlink was found for the
> > initscript of
> >  this service." to inform user why a service can't be enabled.
> > * Added German and Dutch translation for above message.
> > 
> > Fixes: Bug#12935
> > Signed-off-by: Robin Roevens <robin.roevens(a)disroot.org>
> > ---
> > html/cgi-bin/services.cgi | 103 +++++++++++++++--------------------
> > ---
> > langs/de/cgi-bin/de.pl    |   1 +
> > langs/en/cgi-bin/en.pl    |   1 +
> > langs/nl/cgi-bin/nl.pl    |   1 +
> > 4 files changed, 42 insertions(+), 64 deletions(-)
> > 
> > diff --git a/html/cgi-bin/services.cgi b/html/cgi-bin/services.cgi
> > index 29926ecc3..e91da884b 100644
> > --- a/html/cgi-bin/services.cgi
> > +++ b/html/cgi-bin/services.cgi
> > @@ -142,14 +142,14 @@ END
> >         my $paramstr=$ENV{QUERY_STRING};
> >         my @param=split(/!/, $paramstr);
> >         if ($param[1] ne ''){
> > -               &General::system("/usr/local/bin/addonctrl",
> > "$param[0]", "$param[1]");
> > +               &General::system("/usr/local/bin/addonctrl",
> > "$param[0]", "$param[1]", "$param[2]");
> >         }
> 
> Although this has been like this before, I am strongly against
> passing anything from the request to the setuid binary.
> 
> This will open us up for any shell command injections and so on. I
> would prefer to check the second argument to be at least on some kind
> of list as there are only a few keywords allowed (start, stop,
> restart, …) and the other parameters should only contain ASCII
> letters, numbers and maybe dash and underscore. Nothing else should
> be passed through here.

Good point. I was thinking this regexp: /[^a-zA-Z_0-9\-\.]/ or should I
not include a '.'? I don't think there currently are any addons or
initscripts that contain a '.', but it could be in the future? 

Regards
Robin
> 
> > 
> >         print <<END
> > <div align='center'>
> > <table width='80%' cellspacing='1' class='tbl'>
> > <tr>
> > -       <th align='center'><b>Addon</b></th>
> > +       <th align='left'><b>Addon $Lang::tr{service}</b></th>
> >         <th align='center'><b>Boot</b></th>
> >         <th align='center'
> > colspan=2><b>$Lang::tr{'action'}</b></th>
> >         <th align='center'><b>$Lang::tr{'status'}</b></th>
> > @@ -170,33 +170,35 @@ END
> >         foreach my $pak (keys %paklist) {
> >                 my %metadata = &Pakfire::getmetadata($pak,
> > "installed");
> >                         
> > +               my $service;
> > +
> >                 if ("$metadata{'Services'}") {
> > -                       foreach my $service (split(/ /,
> > "$metadata{'Services'}")) {
> > -                               push(@addon_services, $service);
> > -                       }
> > -               }
> > -       }
> > +                       foreach $service (split(/ /,
> > "$metadata{'Services'}")) {
> > +                               $lines++;
> > +                               if ($lines % 2) {
> > +                                       print "<tr>";
> > +                                       $col="bgcolor='$color{'colo
> > r22'}'";
> > +                               } else {
> > +                                       print "<tr>";
> > +                                       $col="bgcolor='$color{'colo
> > r20'}'";
> > +                               }
> > 
> > -       foreach (@addon_services) {
> > -               $lines++;
> > -               if ($lines % 2){
> > -                       print "<tr>";
> > -                       $col="bgcolor='$color{'color22'}'";
> > -               }else{
> > -                       print "<tr>";
> > -                       $col="bgcolor='$color{'color20'}'";
> > +                               # Add addon name to displayname of
> > service if servicename differs from addon
> > +                               my $displayname = ($pak ne
> > $service) ? "$service ($pak)" : $service;
> > +                               print "<td align='left' $col
> > width='31%'>$displayname</td> ";
> > +
> > +                               my $status =
> > isautorun($pak,$service,$col);
> > +                               print "$status ";
> > +                               print "<td align='center' $col
> > width='8%'><a href='services.cgi?$pak!start!$service'><img
> > alt='$Lang::tr{'start'}' title='$Lang::tr{'start'}'
> > src='/images/go-up.png' border='0' /></a></td>";
> > +                               print "<td align='center' $col
> > width='8%'><a href='services.cgi?$pak!stop!$service'><img
> > alt='$Lang::tr{'stop'}' title='$Lang::tr{'stop'}' src='/images/go-
> > down.png' border='0' /></a></td> ";
> > +                               my $status =
> > isrunningaddon($pak,$service,$col);
> > +                               $status =~ s/\^[\[[0-1]\;[0-9]+m//g;
> > +
> > +                               chomp($status);
> > +                               print "$status";
> > +                               print "</tr>";
> > +                       }
> >                 }
> > -               print "<td align='left' $col width='31%'>$_</td> ";
> > -               my $status = isautorun($_,$col);
> > -               print "$status ";
> > -               print "<td align='center' $col width='8%'><a
> > href='services.cgi?$_!start'><img alt='$Lang::tr{'start'}'
> > title='$Lang::tr{'start'}' src='/images/go-up.png' border='0'
> > /></a></td>";
> > -               print "<td align='center' $col width='8%'><a
> > href='services.cgi?$_!stop'><img alt='$Lang::tr{'stop'}'
> > title='$Lang::tr{'stop'}' src='/images/go-down.png' border='0'
> > /></a></td> ";
> > -               my $status = isrunningaddon($_,$col);
> > -               $status =~ s/\^[\[[0-1]\;[0-9]+m//g;
> > -
> > -               chomp($status);
> > -               print "$status";
> > -               print "</tr>";
> >         }
> > 
> >         print "</table></div>\n";
> > @@ -215,51 +217,24 @@ END
> > }
> > 
> > sub isautorun (@) {
> > -       my ($cmd, $col) = @_;
> > -
> > -       # Init directory.
> > -       my $initdir = "/etc/rc.d/rc3.d/";
> > -
> > -       my $status = "<td align='center' $col></td>";
> > +       my ($pak, $service, $col) = @_;
> > +       my @testcmd =
> > &General::system_output("/usr/local/bin/addonctrl", "$pak", "boot-
> > status", "$service");
> > +       my $testcmd = @testcmd[0];
> > +       my $status = "<td align='center' $col><img
> > alt='$Lang::tr{'service boot setting unavailable'}'
> > title='$Lang::tr{'service boot setting unavailable'}'
> > src='/images/dialog-warning.png' border='0' width='16' height='16'
> > /></td>";
> > 
> > -       # Check if autorun for the given cmd is enabled.
> > -       if ( &find_init("$cmd", "$initdir") ) {
> > +       # Check if autorun for the given service is enabled.
> > +       if ( $testcmd =~ /enabled\ on\ boot/ ) {
> >                 # Adjust status.
> > -               $status = "<td align='center' $col><a
> > href='services.cgi?$_!disable'><img alt='$Lang::tr{'deactivate'}'
> > title='$Lang::tr{'deactivate'}' src='/images/on.gif' border='0'
> > width='16' height='16' /></a></td>";
> > -       } else {
> > +               $status = "<td align='center' $col><a
> > href='services.cgi?$pak!disable!$service'><img
> > alt='$Lang::tr{'deactivate'}' title='$Lang::tr{'deactivate'}'
> > src='/images/on.gif' border='0' width='16' height='16'
> > /></a></td>";
> > +       } elsif ( $testcmd =~ /disabled\ on\ boot/ ) {
> >                 # Adjust status.
> > -               $status = "<td align='center' $col><a
> > href='services.cgi?$_!enable'><img alt='$Lang::tr{'activate'}'
> > title='$Lang::tr{'activate'}' src='/images/off.gif' border='0'
> > width='16' height='16' /></a></td>";
> > +               $status = "<td align='center' $col><a
> > href='services.cgi?$pak!enable!$service'><img
> > alt='$Lang::tr{'activate'}' title='$Lang::tr{'activate'}'
> > src='/images/off.gif' border='0' width='16' height='16'
> > /></a></td>";
> >         }
> > 
> >         # Return the status.
> >         return $status;
> > }
> > 
> > -sub find_init (@) {
> > -       my ($cmd, $dir) = @_;
> > -
> > -       # Open given init directory.
> > -       opendir (INITDIR, "$dir") || die "Cannot opendir $dir: $!";
> > -
> > -       # Read-in init files from directory.
> > -       my @inits = readdir(INITDIR);
> > -
> > -       # Close directory handle.
> > -       closedir(INITDIR);
> > -
> > -       # Loop through the directory.
> > -       foreach my $init (@inits) {
> > -               # Check if the current processed file belongs to
> > the given command.
> > -               if ($init =~ /S\d+\d+$cmd\z/) {
> > -                       # Found, return "1" - True.
> > -                       return "1";
> > -               }
> > -        }
> > -
> > -       # Nothing found, return nothing.
> > -       return;
> > -}
> > -
> > sub isrunning (@) {
> >         my ($cmd, $col) = @_;
> >         my $status = "<td align='center'
> > bgcolor='${Header::colourred}'><font
> > color='white'><b>$Lang::tr{'stopped'}</b></font></td><td
> > colspan='2' $col></td>";
> > @@ -313,7 +288,7 @@ sub isrunning (@) {
> > }
> > 
> > sub isrunningaddon (@) {
> > -       my ($cmd, $col) = @_;
> > +       my ($pak, $service, $col) = @_;
> > 
> >         my $status = "<td align='center'
> > bgcolor='${Header::colourred}'><font
> > color='white'><b>$Lang::tr{'stopped'}</b></font></td><td
> > colspan='2' $col></td>";
> >         my $pid = '';
> > @@ -321,7 +296,7 @@ sub isrunningaddon (@) {
> >         my $exename;
> >         my @memory;
> > 
> > -       my @testcmd =
> > &General::system_output("/usr/local/bin/addonctrl", "$cmd",
> > "status");
> > +       my @testcmd =
> > &General::system_output("/usr/local/bin/addonctrl", "$pak",
> > "status", "$service");
> >         my $testcmd = @testcmd[0];
> > 
> >         if ( $testcmd =~ /is\ running/ && $testcmd !~ /is\ not\
> > running/){
> > diff --git a/langs/de/cgi-bin/de.pl b/langs/de/cgi-bin/de.pl
> > index 798abcffc..db7d117b0 100644
> > --- a/langs/de/cgi-bin/de.pl
> > +++ b/langs/de/cgi-bin/de.pl
> > @@ -2251,6 +2251,7 @@
> > 'server string' => 'Server String',
> > 'service' => 'Dienst',
> > 'service added' => 'Benutzerdefinierter Netzwerkdienst wurde
> > hinzugefügt',
> > +'service boot setting unavailable' => 'Für das Initscript dieses
> > Dienstes wurde kein gültiger Runlevel-Symlink gefunden.',
> > 'service name' => 'Name des Dienstes:',
> > 'service removed' => 'Benutzerdefinierter Netzwerkdienst wurde
> > entfernt',
> > 'service updated' => 'Benutzerdefinierter Netzwerkdienst wurde
> > aktualisiert',
> > diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
> > index f770e7cd9..60dca5be4 100644
> > --- a/langs/en/cgi-bin/en.pl
> > +++ b/langs/en/cgi-bin/en.pl
> > @@ -2306,6 +2306,7 @@
> > 'server string' => 'Server String',
> > 'service' => 'Service',
> > 'service added' => 'Custom network service added',
> > +'service boot setting unavailable' => 'No valid runlevel symlink
> > was found for the initscript of this service.',
> > 'service name' => 'Service name:',
> > 'service removed' => 'Custom network service removed',
> > 'service updated' => 'Custom network service updated',
> > diff --git a/langs/nl/cgi-bin/nl.pl b/langs/nl/cgi-bin/nl.pl
> > index 49dabec99..4fd6955cc 100644
> > --- a/langs/nl/cgi-bin/nl.pl
> > +++ b/langs/nl/cgi-bin/nl.pl
> > @@ -1899,6 +1899,7 @@
> > 'server string' => 'Server String',
> > 'service' => 'Dienst',
> > 'service added' => 'Aangepaste netwerkdienst toegevoegd',
> > +'service boot setting unavailable' => 'Er werd voor het initscript
> > van deze service geen geldige runlevel symlink gevonden.',
> > 'service name' => 'Dienstennaam:',
> > 'service removed' => 'Aangepaste netwerkdienst verwijderd',
> > 'service updated' => 'Aangepaste netwerkdienst bijgewerkt',
> > -- 
> > 2.37.3
> > 
> > 
> > -- 
> > Dit bericht is gescanned op virussen en andere gevaarlijke
> > inhoud door MailScanner en lijkt schoon te zijn.
> > 
> 
> 

-- 
Dit bericht is gescanned op virussen en andere gevaarlijke
inhoud door MailScanner en lijkt schoon te zijn.