diff mbox series

[2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate

Message ID 20250401180802.19784-2-adolf.belka@ipfire.org
State Staged
Commit 41c7cc325e1e2f922de803842d0625e564f6771e
Headers
Series [1/6] vpnmain.cgi: Fixes bug13737 - remove unneeded &cleanssldatabase calls |

Commit Message

Adolf Belka April 1, 2025, 6:07 p.m. UTC 
  - As the serial number is incremented now for each new cert that is created, then when a
   client cert is deleted from the ipsec list in the wui then that cert must be revoked
   otherwise it will still be listed in the .index file as a valid certificate and then
   the certificate name and DN could never be used again.
- Running the revoke command when deleting a client cert leaves the details in the .index
   file but the same name can then be re-used and will get a new serial number etc.

Fixes: bug13737
Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

Comments

Michael Tremer April 2, 2025, 10:21 a.m. UTC | #1 
> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> - As the serial number is incremented now for each new cert that is created, then when a
>   client cert is deleted from the ipsec list in the wui then that cert must be revoked
>   otherwise it will still be listed in the .index file as a valid certificate and then
>   the certificate name and DN could never be used again.
> - Running the revoke command when deleting a client cert leaves the details in the .index
>   file but the same name can then be re-used and will get a new serial number etc.
> 
> Fixes: bug13737
> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
> ---
> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
> 1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
> index 85119a81d..1c9f9243b 100644
> --- a/html/cgi-bin/vpnmain.cgi
> +++ b/html/cgi-bin/vpnmain.cgi
> @@ -1595,17 +1595,25 @@ END
> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
> 
> - if ($confighash{$cgiparams{'KEY'}}) {
> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
> - delete $confighash{$cgiparams{'KEY'}};
> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
> - &writeipsecfiles();
> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
> - } else {
> - $errormessage = $Lang::tr{'invalid key'};
> - }
> - &General::firewall_reload();
> +        if ($confighash{$cgiparams{'KEY'}}) {
> +                # Revoke the removed certificate
> +                if (!$errormessage) {
> +                        &General::log("charon", "Revoking the removed client cert...");
> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
> +                        $errormessage = &callssl($opt);

This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.

Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.

> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
> +                        delete $confighash{$cgiparams{'KEY'}};
> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
> +                        &writeipsecfiles();
> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
> +                } else {
> +                        goto VPNCONF_ERROR;
> +                }
> +        } else {
> +                $errormessage = $Lang::tr{'invalid key'};
> +        }
> +        &General::firewall_reload();
> ###
> ### Choose between adding a host-net or net-net connection
> ###
> -- 
> 2.49.0
> 
>
Adolf Belka April 2, 2025, 10:41 a.m. UTC | #2 
Hi Michael,

On 02/04/2025 12:21, Michael Tremer wrote:
> 
> 
>> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>
>> - As the serial number is incremented now for each new cert that is created, then when a
>>    client cert is deleted from the ipsec list in the wui then that cert must be revoked
>>    otherwise it will still be listed in the .index file as a valid certificate and then
>>    the certificate name and DN could never be used again.
>> - Running the revoke command when deleting a client cert leaves the details in the .index
>>    file but the same name can then be re-used and will get a new serial number etc.
>>
>> Fixes: bug13737
>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>> ---
>> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
>> index 85119a81d..1c9f9243b 100644
>> --- a/html/cgi-bin/vpnmain.cgi
>> +++ b/html/cgi-bin/vpnmain.cgi
>> @@ -1595,17 +1595,25 @@ END
>> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
>> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
>>
>> - if ($confighash{$cgiparams{'KEY'}}) {
>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>> - delete $confighash{$cgiparams{'KEY'}};
>> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>> - &writeipsecfiles();
>> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>> - } else {
>> - $errormessage = $Lang::tr{'invalid key'};
>> - }
>> - &General::firewall_reload();
>> +        if ($confighash{$cgiparams{'KEY'}}) {
>> +                # Revoke the removed certificate
>> +                if (!$errormessage) {
>> +                        &General::log("charon", "Revoking the removed client cert...");
>> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
>> +                        $errormessage = &callssl($opt);
> 
> This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.
> 
> Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.

I will look at doing that. However it might have to wait till I am back from visiting my family. I might be able to do it while visiting my son but can't be sure I will get the time.

Will put it on my list.

Regards,

Adolf.

> 
>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>> +                        delete $confighash{$cgiparams{'KEY'}};
>> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>> +                        &writeipsecfiles();
>> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>> +                } else {
>> +                        goto VPNCONF_ERROR;
>> +                }
>> +        } else {
>> +                $errormessage = $Lang::tr{'invalid key'};
>> +        }
>> +        &General::firewall_reload();
>> ###
>> ### Choose between adding a host-net or net-net connection
>> ###
>> -- 
>> 2.49.0
>>
>>
> 
>
Michael Tremer April 2, 2025, 1:52 p.m. UTC | #3 
Hello Adolf,

> On 2 Apr 2025, at 11:41, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 02/04/2025 12:21, Michael Tremer wrote:
>>> On 1 Apr 2025, at 19:07, Adolf Belka <adolf.belka@ipfire.org> wrote:
>>> 
>>> - As the serial number is incremented now for each new cert that is created, then when a
>>>   client cert is deleted from the ipsec list in the wui then that cert must be revoked
>>>   otherwise it will still be listed in the .index file as a valid certificate and then
>>>   the certificate name and DN could never be used again.
>>> - Running the revoke command when deleting a client cert leaves the details in the .index
>>>   file but the same name can then be re-used and will get a new serial number etc.
>>> 
>>> Fixes: bug13737
>>> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>> ---
>>> html/cgi-bin/vpnmain.cgi | 30 +++++++++++++++++++-----------
>>> 1 file changed, 19 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
>>> index 85119a81d..1c9f9243b 100644
>>> --- a/html/cgi-bin/vpnmain.cgi
>>> +++ b/html/cgi-bin/vpnmain.cgi
>>> @@ -1595,17 +1595,25 @@ END
>>> &General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
>>> &General::readhasharray("${General::swroot}/vpn/config", \%confighash);
>>> 
>>> - if ($confighash{$cgiparams{'KEY'}}) {
>>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>>> - unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>>> - delete $confighash{$cgiparams{'KEY'}};
>>> - &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>>> - &writeipsecfiles();
>>> - &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>>> - } else {
>>> - $errormessage = $Lang::tr{'invalid key'};
>>> - }
>>> - &General::firewall_reload();
>>> +        if ($confighash{$cgiparams{'KEY'}}) {
>>> +                # Revoke the removed certificate
>>> +                if (!$errormessage) {
>>> +                        &General::log("charon", "Revoking the removed client cert...");
>>> +                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
>>> +                        $errormessage = &callssl($opt);
>> This is another chance to perform some shell command execution because the $cgiparams{'KEY’} input is potentially unchecked.
>> Can we change the validate the key before? It should be a number. In the if statement above, we are only checking if it is actually set.
> 
> I will look at doing that. However it might have to wait till I am back from visiting my family. I might be able to do it while visiting my son but can't be sure I will get the time.

No rush, this is not that urgent, but of course we should not forget about it…

-Michael

> 
> Will put it on my list.
> 
> Regards,
> 
> Adolf.
> 
>>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
>>> +                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
>>> +                        delete $confighash{$cgiparams{'KEY'}};
>>> +                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
>>> +                        &writeipsecfiles();
>>> +                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
>>> +                } else {
>>> +                        goto VPNCONF_ERROR;
>>> +                }
>>> +        } else {
>>> +                $errormessage = $Lang::tr{'invalid key'};
>>> +        }
>>> +        &General::firewall_reload();
>>> ###
>>> ### Choose between adding a host-net or net-net connection
>>> ###
>>> -- 
>>> 2.49.0
diff mbox series

Patch

diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
index 85119a81d..1c9f9243b 100644
--- a/html/cgi-bin/vpnmain.cgi
+++ b/html/cgi-bin/vpnmain.cgi
@@ -1595,17 +1595,25 @@  END
 	&General::readhash("${General::swroot}/vpn/settings", \%vpnsettings);
 	&General::readhasharray("${General::swroot}/vpn/config", \%confighash);
 
-	if ($confighash{$cgiparams{'KEY'}}) {
-		unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
-		unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
-		delete $confighash{$cgiparams{'KEY'}};
-		&General::writehasharray("${General::swroot}/vpn/config", \%confighash);
-		&writeipsecfiles();
-		&General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
-	} else {
-		$errormessage = $Lang::tr{'invalid key'};
-	}
-	&General::firewall_reload();
+        if ($confighash{$cgiparams{'KEY'}}) {
+                # Revoke the removed certificate
+                if (!$errormessage) {
+                        &General::log("charon", "Revoking the removed client cert...");
+                        my $opt = " ca -revoke ${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem";
+                        $errormessage = &callssl($opt);
+                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1]cert.pem");
+                        unlink ("${General::swroot}/certs/$confighash{$cgiparams{'KEY'}}[1].p12");
+                        delete $confighash{$cgiparams{'KEY'}};
+                        &General::writehasharray("${General::swroot}/vpn/config", \%confighash);
+                        &writeipsecfiles();
+                        &General::system('/usr/local/bin/ipsecctrl', 'D', $cgiparams{'KEY'}) if (&vpnenabled);
+                } else {
+                        goto VPNCONF_ERROR;
+                }
+        } else {
+                $errormessage = $Lang::tr{'invalid key'};
+        }
+        &General::firewall_reload();
 ###
 ### Choose between adding a host-net or net-net connection
 ###