From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 2/2] services.cgi: Fix status/actions on services with name != addon name Date: Fri, 07 Oct 2022 11:12:50 +0100 Message-ID: In-Reply-To: <8f1283b2f28150e88a97eeab672879ec48b1eaa0.camel@sicho.home> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0111375191795326331==" List-Id: --===============0111375191795326331== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 4 Oct 2022, at 19:59, Robin Roevens wrote: >=20 > Hi Michael >=20 > Michael Tremer schreef op di 04-10-2022 om 13:49 [+0100]: >> Hello, >>=20 >>> On 4 Oct 2022, at 11:33, Robin Roevens >>> 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 >>>>> >>>>> 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 >>>>> --- >>>>> 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 >>=20 >> 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. >=20 > 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 me= anings (e.g. /etc/init.d/../../bin/bash). We don=E2=80=99t allow using / here, so we should be safe, but it is probably= still a good idea to be as strict as possible. >=20 >>=20 >> The regular expression is slightly incorrect though. You want it to >> be like this: >>=20 >> /^[A-Za-z0-9\-\_]+$/ >>=20 >> 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. >=20 > I have to disagree in the regular expression being incorrect :-).=20 > 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 t= hing 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=E2=80=99t. Disallowing seems to be logic= ally more complex. -Michael >=20 >=20 > Regards > Robin >=20 >>=20 >> -Michael >>=20 >>>=20 >>> Regards >>> Robin >>>>=20 >>>>>=20 >>>>> print <>>>>
>>>>> >>>>> >>>>> - >>>>> + >>>>> >>>>> >>>>> >>>>> @@ -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 ""; >>>>> + $col=3D"bgcolor=3D'$color{'colo >>>>> r22'}'"; >>>>> + } else { >>>>> + print ""; >>>>> + $col=3D"bgcolor=3D'$color{'colo >>>>> r20'}'"; >>>>> + } >>>>>=20 >>>>> - foreach (@addon_services) { >>>>> - $lines++; >>>>> - if ($lines % 2){ >>>>> - print ""; >>>>> - $col=3D"bgcolor=3D'$color{'color22'}'"; >>>>> - }else{ >>>>> - print ""; >>>>> - $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 " "; >>>>> + >>>>> + my $status =3D >>>>> isautorun($pak,$service,$col); >>>>> + print "$status "; >>>>> + print ""; >>>>> + print " "; >>>>> + my $status =3D >>>>> isrunningaddon($pak,$service,$col); >>>>> + $status =3D~ s/\=1B\[[0-1]\;[0-9]+m//g; >>>>> + >>>>> + chomp($status); >>>>> + print "$status"; >>>>> + print ""; >>>>> + } >>>>> } >>>>> - print " "; >>>>> - my $status =3D isautorun($_,$col); >>>>> - print "$status "; >>>>> - print ""; >>>>> - print " "; >>>>> - my $status =3D isrunningaddon($_,$col); >>>>> - $status =3D~ s/\=1B\[[0-1]\;[0-9]+m//g; >>>>> - >>>>> - chomp($status); >>>>> - print "$status"; >>>>> - print ""; >>>>> } >>>>>=20 >>>>> print "
AddonAddon $Lang::tr{service}Boot>>>> colspan=3D2>$Lang::tr{'action'}$Lang::tr{'status'}
>>>> width=3D'31%'>$displayname>>>> width=3D'8%'>>>>> alt=3D'$Lang::tr{'start'}' title=3D'$Lang::tr{'start'}' >>>>> src=3D'/images/go-up.png' border=3D'0' />>>>> width=3D'8%'>>>>> alt=3D'$Lang::tr{'stop'}' title=3D'$Lang::tr{'stop'}' >>>>> src=3D'/images/go- >>>>> down.png' border=3D'0' />
$_>>>> href=3D'services.cgi?$_!start'>3D'$Lang::tr{'start'}'>>>> title=3D'$Lang::tr{'start'}' src=3D'/images/go-up.png' border=3D'0' >>>>> />>>>> href=3D'services.cgi?$_!stop'>3D'$Lang::tr{'stop'}'>>>> title=3D'$Lang::tr{'stop'}' src=3D'/images/go-down.png' border=3D'0' >>>>> />
\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 ""; >>>>> + 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 ">>>> 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' >>>>> />"; >>>>>=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 ">>>> href=3D'services.cgi?$_!disable'>>>>> alt=3D'$Lang::tr{'deactivate'}' >>>>> title=3D'$Lang::tr{'deactivate'}' src=3D'/images/on.gif' border=3D'0' >>>>> width=3D'16' height=3D'16' />"; >>>>> - } else { >>>>> + $status =3D ">>>> href=3D'services.cgi?$pak!disable!$service'>>>>> alt=3D'$Lang::tr{'deactivate'}' title=3D'$Lang::tr{'deactivate'}' >>>>> src=3D'/images/on.gif' border=3D'0' width=3D'16' height=3D'16' >>>>> />"; >>>>> + } elsif ( $testcmd =3D~ /disabled\ on\ boot/ ) { >>>>> # Adjust status. >>>>> - $status =3D ">>>> href=3D'services.cgi?$_!enable'>3D'$Lang::tr{'activate'}'>>>> title=3D'$Lang::tr{'activate'}' src=3D'/images/off.gif' border=3D'0' >>>>> width=3D'16' height=3D'16' />"; >>>>> + $status =3D ">>>> href=3D'services.cgi?$pak!enable!$service'>>>>> alt=3D'$Lang::tr{'activate'}' title=3D'$Lang::tr{'activate'}' >>>>> src=3D'/images/off.gif' border=3D'0' width=3D'16' height=3D'16' >>>>> />"; >>>>> } >>>>>=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 ">>>> bgcolor=3D'${Header::colourred}'>>>>> color=3D'white'>$Lang::tr{'stopped'}>>>> colspan=3D'2' $col>"; >>>>> @@ -313,7 +288,7 @@ sub isrunning (@) { >>>>> } >>>>>=20 >>>>> sub isrunningaddon (@) { >>>>> - my ($cmd, $col) =3D @_; >>>>> + my ($pak, $service, $col) =3D @_; >>>>>=20 >>>>> my $status =3D ">>>> bgcolor=3D'${Header::colourred}'>>>>> color=3D'white'>$Lang::tr{'stopped'}>>>> colspan=3D'2' $col>"; >>>>> 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. >>=20 >>=20 >=20 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. --===============0111375191795326331==--