From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4ZSLyB4w6Hz332d for ; Wed, 2 Apr 2025 10:41:22 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) client-signature RSA-PSS (4096 bits)) (Client CN "mail01.haj.ipfire.org", Issuer "R10" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4ZSLy711nDz30Mg for ; Wed, 2 Apr 2025 10:41:19 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4ZSLy62nBqz36; Wed, 2 Apr 2025 10:41:18 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1743590478; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fAAxpb3pw4mSL44xMgCOSrhuCEsE8fYf+jWLGIARuuo=; b=hGUTskKGBSuMCvU9RMPHArY84rS8T0sF1asQCqCJwwFyDwIDsjQlVpPSqw10FK/V+5lPDw +J+U0JKtOwaDhaAQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1743590478; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=fAAxpb3pw4mSL44xMgCOSrhuCEsE8fYf+jWLGIARuuo=; b=poA8biLrqkkv4aOBWi7O8SOOBQu3wKAucKEyPbYNTPpfnzM/pEiQDCsBUDNMLoFR18WMMM ktDjOqVN6XS1PMqP6mkvJMnhUN49wvBBVQ0r/89yKF2bGBByYTNDFZpnXxHhwVcNmnjv7n COIN3EBZAznzw8ORaXdHj6g1wa95TXsdiLciFskQN78j3wXzbcVBOAfIy88cTBcBf1tIIb olApHu+E6tH3Vo9hRwLd2gU3EWh+BDwY0MEKCaZa6jVnvYnD/5mf0btwsSxzuGI+aRIYQ8 cT5CH6ULmNQXejmeGqX/QgytLw1x1VYpTjAm9nmypAl4gFJDZDrZ2MQX4lYdWA== Message-ID: <12f44533-ba3b-4d91-83a5-c97200cccdd4@ipfire.org> Date: Wed, 2 Apr 2025 12:41:14 +0200 Precedence: list List-Id: List-Subscribe: , List-Unsubscribe: , List-Post: List-Help: Sender: Mail-Followup-To: MIME-Version: 1.0 Subject: Re: [PATCH 2/6] vpnmain.cgi: Fixes bug13737 - revoke any deleted client certificate To: Michael Tremer References: <20250401180802.19784-1-adolf.belka@ipfire.org> <20250401180802.19784-2-adolf.belka@ipfire.org> <1065A659-E2A7-4CB5-9F2C-3E8E40A0DE04@ipfire.org> Content-Language: en-GB Cc: "IPFire: Development-List" From: Adolf Belka In-Reply-To: <1065A659-E2A7-4CB5-9F2C-3E8E40A0DE04@ipfire.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Michael, On 02/04/2025 12:21, Michael Tremer wrote: > > >> On 1 Apr 2025, at 19:07, Adolf Belka 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 >> Signed-off-by: Adolf Belka >> --- >> 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 >> >> > >