From mboxrd@z Thu Jan 1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 2/2] services.cgi: Fix status/actions on services with
name != addon name
Date: Tue, 04 Oct 2022 13:49:05 +0100
Message-ID: <51F7DC88-7593-4F97-925D-472EEF48B514@ipfire.org>
In-Reply-To: <50e62161be8c54ca80c2cb711be1a2686545b281.camel@sicho.home>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0982853370413612232=="
List-Id: <development.lists.ipfire.org>
--===============0982853370413612232==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Hello,
> On 4 Oct 2022, at 11:33, Robin Roevens <robin.roevens(a)disroot.org> wrote:
>=20
> Hi
>=20
> Michael Tremer schreef op di 04-10-2022 om 10:51 [+0100]:
>> Hello,
>>=20
>> I am going to start with the second patch, because that is going to
>> be shorter :)
>>=20
>>> On 3 Oct 2022, at 16:27, Robin Roevens <robin.roevens(a)disroot.org>
>>> wrote:
>>>=20
>>> * 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.
>>>=20
>>> 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(-)
>>>=20
>>> 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=3D$ENV{QUERY_STRING};
>>> my @param=3Dsplit(/!/, $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]");
>>> }
>>=20
>> Although this has been like this before, I am strongly against
>> passing anything from the request to the setuid binary.
>>=20
>> 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, =E2=80=A6) and the other parameters should only contain ASCII
>> letters, numbers and maybe dash and underscore. Nothing else should
>> be passed through here.
>=20
> 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?=20
I was thinking about the dot as well, and then thought it would not be requir=
ed to include it since we do not have any services like this, and I do not re=
member any of them using a dot on any other distributions.
The regular expression is slightly incorrect though. You want it to be like t=
his:
/^[A-Za-z0-9\-\_]+$/
The ^ and $ indicate that the entire string must match and not only a part. A=
nd 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 s=
tring must only contain those characters.
-Michael
>=20
> Regards
> Robin
>>=20
>>>=20
>>> print <<END
>>> <div align=3D'center'>
>>> <table width=3D'80%' cellspacing=3D'1' class=3D'tbl'>
>>> <tr>
>>> - <th align=3D'center'><b>Addon</b></th>
>>> + <th align=3D'left'><b>Addon $Lang::tr{service}</b></th>
>>> <th align=3D'center'><b>Boot</b></th>
>>> <th align=3D'center'
>>> colspan=3D2><b>$Lang::tr{'action'}</b></th>
>>> <th align=3D'center'><b>$Lang::tr{'status'}</b></th>
>>> @@ -170,33 +170,35 @@ END
>>> foreach my $pak (keys %paklist) {
>>> my %metadata =3D &Pakfire::getmetadata($pak,
>>> "installed");
>>>=20
>>> + 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=3D"bgcolor=3D'$color{'colo
>>> r22'}'";
>>> + } else {
>>> + print "<tr>";
>>> + $col=3D"bgcolor=3D'$color{'colo
>>> r20'}'";
>>> + }
>>>=20
>>> - foreach (@addon_services) {
>>> - $lines++;
>>> - if ($lines % 2){
>>> - print "<tr>";
>>> - $col=3D"bgcolor=3D'$color{'color22'}'";
>>> - }else{
>>> - print "<tr>";
>>> - $col=3D"bgcolor=3D'$color{'color20'}'";
>>> + # Add addon name to displayname of
>>> service if servicename differs from addon
>>> + my $displayname =3D ($pak ne
>>> $service) ? "$service ($pak)" : $service;
>>> + print "<td align=3D'left' $col
>>> width=3D'31%'>$displayname</td> ";
>>> +
>>> + my $status =3D
>>> isautorun($pak,$service,$col);
>>> + print "$status ";
>>> + print "<td align=3D'center' $col
>>> width=3D'8%'><a href=3D'services.cgi?$pak!start!$service'><img
>>> alt=3D'$Lang::tr{'start'}' title=3D'$Lang::tr{'start'}'
>>> src=3D'/images/go-up.png' border=3D'0' /></a></td>";
>>> + print "<td align=3D'center' $col
>>> width=3D'8%'><a href=3D'services.cgi?$pak!stop!$service'><img
>>> alt=3D'$Lang::tr{'stop'}' title=3D'$Lang::tr{'stop'}' src=3D'/images/go-
>>> down.png' border=3D'0' /></a></td> ";
>>> + my $status =3D
>>> isrunningaddon($pak,$service,$col);
>>> + $status =3D~ s/\=1B\[[0-1]\;[0-9]+m//g;
>>> +
>>> + chomp($status);
>>> + print "$status";
>>> + print "</tr>";
>>> + }
>>> }
>>> - print "<td align=3D'left' $col width=3D'31%'>$_</td> ";
>>> - my $status =3D isautorun($_,$col);
>>> - print "$status ";
>>> - print "<td align=3D'center' $col width=3D'8%'><a
>>> href=3D'services.cgi?$_!start'><img alt=3D'$Lang::tr{'start'}'
>>> title=3D'$Lang::tr{'start'}' src=3D'/images/go-up.png' border=3D'0'
>>> /></a></td>";
>>> - print "<td align=3D'center' $col width=3D'8%'><a
>>> href=3D'services.cgi?$_!stop'><img alt=3D'$Lang::tr{'stop'}'
>>> title=3D'$Lang::tr{'stop'}' src=3D'/images/go-down.png' border=3D'0'
>>> /></a></td> ";
>>> - my $status =3D isrunningaddon($_,$col);
>>> - $status =3D~ s/\=1B\[[0-1]\;[0-9]+m//g;
>>> -
>>> - chomp($status);
>>> - print "$status";
>>> - print "</tr>";
>>> }
>>>=20
>>> print "</table></div>\n";
>>> @@ -215,51 +217,24 @@ END
>>> }
>>>=20
>>> sub isautorun (@) {
>>> - my ($cmd, $col) =3D @_;
>>> -
>>> - # Init directory.
>>> - my $initdir =3D "/etc/rc.d/rc3.d/";
>>> -
>>> - my $status =3D "<td align=3D'center' $col></td>";
>>> + my ($pak, $service, $col) =3D @_;
>>> + my @testcmd =3D
>>> &General::system_output("/usr/local/bin/addonctrl", "$pak", "boot-
>>> status", "$service");
>>> + my $testcmd =3D @testcmd[0];
>>> + my $status =3D "<td align=3D'center' $col><img
>>> alt=3D'$Lang::tr{'service boot setting unavailable'}'
>>> title=3D'$Lang::tr{'service boot setting unavailable'}'
>>> src=3D'/images/dialog-warning.png' border=3D'0' width=3D'16' height=3D'16'
>>> /></td>";
>>>=20
>>> - # 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 =3D~ /enabled\ on\ boot/ ) {
>>> # Adjust status.
>>> - $status =3D "<td align=3D'center' $col><a
>>> href=3D'services.cgi?$_!disable'><img alt=3D'$Lang::tr{'deactivate'}'
>>> title=3D'$Lang::tr{'deactivate'}' src=3D'/images/on.gif' border=3D'0'
>>> width=3D'16' height=3D'16' /></a></td>";
>>> - } else {
>>> + $status =3D "<td align=3D'center' $col><a
>>> href=3D'services.cgi?$pak!disable!$service'><img
>>> alt=3D'$Lang::tr{'deactivate'}' title=3D'$Lang::tr{'deactivate'}'
>>> src=3D'/images/on.gif' border=3D'0' width=3D'16' height=3D'16'
>>> /></a></td>";
>>> + } elsif ( $testcmd =3D~ /disabled\ on\ boot/ ) {
>>> # Adjust status.
>>> - $status =3D "<td align=3D'center' $col><a
>>> href=3D'services.cgi?$_!enable'><img alt=3D'$Lang::tr{'activate'}'
>>> title=3D'$Lang::tr{'activate'}' src=3D'/images/off.gif' border=3D'0'
>>> width=3D'16' height=3D'16' /></a></td>";
>>> + $status =3D "<td align=3D'center' $col><a
>>> href=3D'services.cgi?$pak!enable!$service'><img
>>> alt=3D'$Lang::tr{'activate'}' title=3D'$Lang::tr{'activate'}'
>>> src=3D'/images/off.gif' border=3D'0' width=3D'16' height=3D'16'
>>> /></a></td>";
>>> }
>>>=20
>>> # Return the status.
>>> return $status;
>>> }
>>>=20
>>> -sub find_init (@) {
>>> - my ($cmd, $dir) =3D @_;
>>> -
>>> - # Open given init directory.
>>> - opendir (INITDIR, "$dir") || die "Cannot opendir $dir: $!";
>>> -
>>> - # Read-in init files from directory.
>>> - my @inits =3D 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 =3D~ /S\d+\d+$cmd\z/) {
>>> - # Found, return "1" - True.
>>> - return "1";
>>> - }
>>> - }
>>> -
>>> - # Nothing found, return nothing.
>>> - return;
>>> -}
>>> -
>>> sub isrunning (@) {
>>> my ($cmd, $col) =3D @_;
>>> my $status =3D "<td align=3D'center'
>>> bgcolor=3D'${Header::colourred}'><font
>>> color=3D'white'><b>$Lang::tr{'stopped'}</b></font></td><td
>>> colspan=3D'2' $col></td>";
>>> @@ -313,7 +288,7 @@ sub isrunning (@) {
>>> }
>>>=20
>>> sub isrunningaddon (@) {
>>> - my ($cmd, $col) =3D @_;
>>> + my ($pak, $service, $col) =3D @_;
>>>=20
>>> my $status =3D "<td align=3D'center'
>>> bgcolor=3D'${Header::colourred}'><font
>>> color=3D'white'><b>$Lang::tr{'stopped'}</b></font></td><td
>>> colspan=3D'2' $col></td>";
>>> my $pid =3D '';
>>> @@ -321,7 +296,7 @@ sub isrunningaddon (@) {
>>> my $exename;
>>> my @memory;
>>>=20
>>> - my @testcmd =3D
>>> &General::system_output("/usr/local/bin/addonctrl", "$cmd",
>>> "status");
>>> + my @testcmd =3D
>>> &General::system_output("/usr/local/bin/addonctrl", "$pak",
>>> "status", "$service");
>>> my $testcmd =3D @testcmd[0];
>>>=20
>>> if ( $testcmd =3D~ /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' =3D> 'Server String',
>>> 'service' =3D> 'Dienst',
>>> 'service added' =3D> 'Benutzerdefinierter Netzwerkdienst wurde
>>> hinzugef=C3=BCgt',
>>> +'service boot setting unavailable' =3D> 'F=C3=BCr das Initscript dieses
>>> Dienstes wurde kein g=C3=BCltiger Runlevel-Symlink gefunden.',
>>> 'service name' =3D> 'Name des Dienstes:',
>>> 'service removed' =3D> 'Benutzerdefinierter Netzwerkdienst wurde
>>> entfernt',
>>> 'service updated' =3D> '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' =3D> 'Server String',
>>> 'service' =3D> 'Service',
>>> 'service added' =3D> 'Custom network service added',
>>> +'service boot setting unavailable' =3D> 'No valid runlevel symlink
>>> was found for the initscript of this service.',
>>> 'service name' =3D> 'Service name:',
>>> 'service removed' =3D> 'Custom network service removed',
>>> 'service updated' =3D> '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' =3D> 'Server String',
>>> 'service' =3D> 'Dienst',
>>> 'service added' =3D> 'Aangepaste netwerkdienst toegevoegd',
>>> +'service boot setting unavailable' =3D> 'Er werd voor het initscript
>>> van deze service geen geldige runlevel symlink gevonden.',
>>> 'service name' =3D> 'Dienstennaam:',
>>> 'service removed' =3D> 'Aangepaste netwerkdienst verwijderd',
>>> 'service updated' =3D> 'Aangepaste netwerkdienst bijgewerkt',
>>> --=20
>>> 2.37.3
>>>=20
>>>=20
>>> --=20
>>> Dit bericht is gescanned op virussen en andere gevaarlijke
>>> inhoud door MailScanner en lijkt schoon te zijn.
>>>=20
>>=20
>>=20
>=20
> --=20
> Dit bericht is gescanned op virussen en andere gevaarlijke
> inhoud door MailScanner en lijkt schoon te zijn.
--===============0982853370413612232==--