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 4ZBDnj0hsYz3770 for ; Mon, 10 Mar 2025 11:15:05 +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 4ZBDnd2qslz2xc2 for ; Mon, 10 Mar 2025 11:15:01 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4ZBDnc61vtz2r4; Mon, 10 Mar 2025 11:15:00 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1741605300; 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=UGdC4UQDTLE+JUO089CP3ILW7pP7pyCGKiXGjINW6ZM=; b=uEuDWDv4ZTDGlNfY3NLKx0luhYB4yx1ApCibSke88R209zO3+k7P7eQOEgHGbW5H/u0KMW kJgjiF2hqHEOf6Ag== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1741605300; 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=UGdC4UQDTLE+JUO089CP3ILW7pP7pyCGKiXGjINW6ZM=; b=U+h4nQzeD9xQTc5tpBuqc40z7NI0Yk5mL+tz4S4UyCOGsOS7YQHmYJ2MzG8B3qgcQJTej3 BrLrx1g7UzKNeL2EYrmd34rZgzW/K8ak9sVusnmbDbyIYyCubbOxejDqbED8JWT0rc8dPX h0ZW5FbQQRvm5EUME4cjViTIq1i0ZwvmDOAr/msG5E/ZCF7y7F7dmxCsrp1HmtyYDdC8fw XWvCxRqpYjayZXkJU1PxFukY1Mno9rxsSOvOeu6t7AtQo/rvEgePpKOnqn5aZPWO/k7m3x dKdHJBUql0AKbFA8zbYqbgYciKezHBJwRlASZPeOct+yazrdHgvOU19i4UVFFg== Content-Type: text/plain; charset=utf-8 Precedence: list List-Id: List-Subscribe: , List-Unsubscribe: , List-Post: List-Help: Sender: Mail-Followup-To: Mime-Version: 1.0 Subject: Re: [PATCH 1/2] vpnmain.cgi: Fixes bug12298 - IPSec password cannot use semicolon From: Michael Tremer In-Reply-To: <996d9859-482f-4498-88b3-d40f197d5021@ipfire.org> Date: Mon, 10 Mar 2025 11:15:00 +0000 Cc: "IPFire: Development-List" Content-Transfer-Encoding: quoted-printable Message-Id: <7053804E-F1A1-4F80-935C-1493D49C1E0C@ipfire.org> References: <20250309141209.18633-1-adolf.belka@ipfire.org> <996d9859-482f-4498-88b3-d40f197d5021@ipfire.org> To: Adolf Belka Hello, > On 10 Mar 2025, at 10:48, Adolf Belka wrote: >=20 > Hi Michael, >=20 > On 10/03/2025 10:56, Michael Tremer wrote: >> Hello Adolf, >> Oh this is bad. Not the patch, what we have there before=E2=80=A6 >> I would say I will accept this patch as it is because it slightly = mitigates the problem. However, there is still a chance to run shell = commands using backticks and $(=E2=80=A6). >> Would you be able to rewrite this command to use one of the = &General::system* commands? That way, there will no command injection be = possible any more and we can in theory allow quotes and semicolons = again. >=20 > I had wondered about that and did try at first to use the = &General::system_output command but I couldn't get it to work. Even went = to the stage of creating my own mini program to use system_output but it = didn't like the use of $opt being a string with multiple commands in it = separated by a space. >=20 > I then saw that there were already other parts of $opt that were = quoted and so I thought it was alright to use that approach. It actually might be. The custom system functions will however guarantee = it and I would prefer to have a unified way across the entire code base = than custom solutions here or there, because we might break them over = time. > I now understand that those other entries are also not good to use. > I should have trusted my first instinct that this looked the same as = the other bug fix in vpnmain.cgi where I got &General::system_output = working. >=20 > I will go back and find a way to use system_output to do what is = required here. >=20 > After finding a solution I will also provide patches for all the other = locations in vpnmain.cgi that use a similar approach. Then I can look if = there are similar things in other .cgi files. Yes please. There should not be too many I hope. I think we deliberately = skipped a few places where the command line was static and did not = contain any variables as those should always be safe and we urgently = needed to ship the fixes that were reported to us back then. Feel free to ask any questions in case you get stuck. -Michael >=20 > Regards, > Adolf. >=20 >=20 >> Best, >> -Michael >>> On 9 Mar 2025, at 14:12, Adolf Belka wrote: >>>=20 >>> - The password for the pkcs12 certificate is passed to the open ssl = command via $opt but >>> it is not quoted and so the ; is taken as the end of the command = rather than as part >>> of the password. This also means that a pkcs12 file is not created = and the .pem >>> intermediate file is what is left in the directory. >>> - This patch makes the -passout option quoted in the same way as the = -name and -caname >>> options. >>> - Based on being the same as the name and caname parts in $opt, I = believe that this should >>> not give rise to a vulnerability but I am open to being corrected. >>> - By quoting the -passout then the password must not contain double = quotation marks, ", >>> so a test for the password containing a " has been added. >>> - The message about the use of the double quotation mark has been = added to the english, >>> dutch and german language files. Feel free to correct if what I = have used is not >>> correct. Those are in the other patch of this patch set. >>> - Tested out on my testbed system. I was able to create a pkcs12 = certificate with a >>> password containing a variety of characters, including the = semicolon, and getting >>> a message that the password contains a double quotation mark when = I used that. >>>=20 >>> Fixes: bug12298 >>> Tested-by: Adolf Belka >>> Signed-off-by: Adolf Belka >>> --- >>> html/cgi-bin/vpnmain.cgi | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> mode change 100755 =3D> 100644 html/cgi-bin/vpnmain.cgi >>>=20 >>> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi >>> old mode 100755 >>> new mode 100644 >>> index c9bbbb494..8106ee24e >>> --- a/html/cgi-bin/vpnmain.cgi >>> +++ b/html/cgi-bin/vpnmain.cgi >>> @@ -2149,6 +2149,10 @@ END >>> $errormessage =3D $Lang::tr{'password too short'}; >>> goto VPNCONF_ERROR; >>> } >>> + if ($cgiparams{'CERT_PASS1'} =3D~ /["]/) { >>> + $errormessage =3D $Lang::tr{'password has quotation mark'}; >>> + goto VPNCONF_ERROR; >>> + } >>> if ($cgiparams{'CERT_PASS1'} ne $cgiparams{'CERT_PASS2'}) { >>> $errormessage =3D $Lang::tr{'passwords do not match'}; >>> goto VPNCONF_ERROR; >>> @@ -2226,7 +2230,7 @@ END >>> $opt .=3D " -inkey = ${General::swroot}/certs/$cgiparams{'NAME'}key.pem"; >>> $opt .=3D " -in = ${General::swroot}/certs/$cgiparams{'NAME'}cert.pem"; >>> $opt .=3D " -name \"$cgiparams{'NAME'}\""; >>> - $opt .=3D " -passout pass:$cgiparams{'CERT_PASS1'}"; >>> + $opt .=3D " -passout pass:\"$cgiparams{'CERT_PASS1'}\""; >>> $opt .=3D " -certfile ${General::swroot}/ca/cacert.pem"; >>> $opt .=3D " -caname \"$vpnsettings{'ROOTCERT_ORGANIZATION'} CA\""; >>> $opt .=3D " -out ${General::swroot}/certs/$cgiparams{'NAME'}.p12"; >>> --=20 >>> 2.48.1 >>>=20 >>>=20 >=20 > --=20 > Sent from my laptop