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

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hello,

> On 4 Oct 2022, at 11:33, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> 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? 

I was thinking about the dot as well, and then thought it would not be required to include it since we do not have any services like this, and I do not remember any of them using a dot on any other distributions.

The regular expression is slightly incorrect though. You want it to be like this:

/^[A-Za-z0-9\-\_]+$/

The ^ and $ indicate that the entire string must match and not only a part. And the + after the brackets with the list of allowed characters says that we need to have at least one character, but could be more. However, the entire string must only contain those characters.

-Michael

> 
> 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.