From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <development+bounces-14-archive=lists.ipfire.org@lists.ipfire.org>
Received: from mail02.haj.ipfire.org (localhost [127.0.0.1])
by mail02.haj.ipfire.org (Postfix) with ESMTP id 4ZBDnj0hsYz3770
for <archive@lists.ipfire.org>; 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 <development@lists.ipfire.org>; 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: <development.lists.ipfire.org>
List-Subscribe: <https://lists.ipfire.org/>,
<mailto:development+subscribe@lists.ipfire.org?subject=subscribe>
List-Unsubscribe: <https://lists.ipfire.org/>,
<mailto:development+unsubscribe@lists.ipfire.org?subject=unsubscribe>
List-Post: <mailto:development@lists.ipfire.org>
List-Help: <mailto:development+help@lists.ipfire.org?subject=help>
Sender: <development@lists.ipfire.org>
Mail-Followup-To: <development@lists.ipfire.org>
Mime-Version: 1.0
Subject: Re: [PATCH 1/2] vpnmain.cgi: Fixes bug12298 - IPSec password cannot
use semicolon
From: Michael Tremer <michael.tremer@ipfire.org>
In-Reply-To: <996d9859-482f-4498-88b3-d40f197d5021@ipfire.org>
Date: Mon, 10 Mar 2025 11:15:00 +0000
Cc: "IPFire: Development-List" <development@lists.ipfire.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <7053804E-F1A1-4F80-935C-1493D49C1E0C@ipfire.org>
References: <20250309141209.18633-1-adolf.belka@ipfire.org>
<E4F7CA3C-DE69-47F6-BC10-F94A2D12B4D9@ipfire.org>
<996d9859-482f-4498-88b3-d40f197d5021@ipfire.org>
To: Adolf Belka <adolf.belka@ipfire.org>
Hello,
> On 10 Mar 2025, at 10:48, Adolf Belka <adolf.belka@ipfire.org> 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 <adolf.belka@ipfire.org> 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 <adolf.belka@ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
>>> ---
>>> 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