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: 14974 bytes --]

Hello,

> On 4 Oct 2022, at 19:59, Robin Roevens <robin.roevens(a)disroot.org> wrote:
> 
> Hi Michael
> 
> Michael Tremer schreef op di 04-10-2022 om 13:49 [+0100]:
>> 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.
> 
> I don't think I have indeed ever encountered an initscript with a dot
> in it. I was only thinking, as it is a legit and common character for
> filenames (in general) it could cause some confusion as to why the
> webui would not want to work if we ever come across an initscript with
> a dot in its name. But on second thought, the odds are probably very
> very low that we will ever have that on IPFire. So I'l leave the dot
> out.

Thank you.

The dot is a little bit concerning, because in a full path, it has special meanings (e.g. /etc/init.d/../../bin/bash).

We don’t allow using / here, so we should be safe, but it is probably still a good idea to be as strict as possible.

> 
>> 
>> 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.
> 
> I have to disagree in the regular expression being incorrect :-). 
> The ^ right after the [ is not a start of line assertion here, but
> makes it a character class negation which will match as soon as a
> single character is found that is _not_ included in the list. So I was
> looking for a single illegal character in the string, while your regex
> will check if every character in the string is an allowed character.

Hmm, I thought that that could have been the case, but then wrote the whole thing again because that seemed to be easier for me :)

I kind of prefer my version, because it allows certain things and that is it. It feels more strict - probably isn’t. Disallowing seems to be logically more complex.

-Michael

> 
> 
> Regards
> Robin
> 
>> 
>> -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.
>> 
>> 
> 
> -- 
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.