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 4ZBDCQ29pfz3772 for ; Mon, 10 Mar 2025 10:48:50 +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 4ZBDCL441bz35ZM for ; Mon, 10 Mar 2025 10:48:46 +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 4ZBDCK6LLTz2QH; Mon, 10 Mar 2025 10:48:45 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1741603726; 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=W5+jHiILFP+DWPTHV+EujbM6KpdmEf7rAyxdZGswfdY=; b=HCHB17PK6a8oS4nLmAL//pRdNVi/IlOE4EbjFGhU3IgFjWggIAi5zH/D2y93myRs/M0LTr UZhN9vEwANJgYdAQ== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1741603726; 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=W5+jHiILFP+DWPTHV+EujbM6KpdmEf7rAyxdZGswfdY=; b=h/CxeO57J5btob5jwoKQ/bg1kkz/hmJozhDua1Rnl3IjUg5DM76EIwWe3UsYmSD7BeSQXI 1DcHFEL7+uRm1d+lC/luspDweYHiJaCCxPgN7/1GsAB2b+C5OVAq7q6+Ly1Q16QZ4Z04Dz xd2Ffnpy9/3kmkDRfpqjn/as9cqlPmNrsBzvssWbuCEnZmoWwvoIVTwfx0VuFij4DJ0gav tsaV2yE7wRH7SOJ4D/AVTVx2WSwc3WJkA1tHJpaMoOgATxo8fzMSKCQ3BuQLuqbdumVXpf 5nsgqgOJ6b5RmI81Sm3LTjq4ra2BDDE+ElbMUSsv4I3s8uhnofmVbJ8RYpE36Q== Message-ID: <996d9859-482f-4498-88b3-d40f197d5021@ipfire.org> Date: Mon, 10 Mar 2025 11:48:41 +0100 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 To: Michael Tremer References: <20250309141209.18633-1-adolf.belka@ipfire.org> Content-Language: en-GB Cc: "IPFire: Development-List" From: Adolf Belka In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Michael, On 10/03/2025 10:56, Michael Tremer wrote: > Hello Adolf, > > Oh this is bad. Not the patch, what we have there before… > > 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 $(…). > > 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. 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. 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. 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. I will go back and find a way to use system_output to do what is required here. 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. Regards, Adolf. > > Best, > -Michael > >> On 9 Mar 2025, at 14:12, Adolf Belka wrote: >> >> - 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. >> >> 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 => 100644 html/cgi-bin/vpnmain.cgi >> >> 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 = $Lang::tr{'password too short'}; >> goto VPNCONF_ERROR; >> } >> + if ($cgiparams{'CERT_PASS1'} =~ /["]/) { >> + $errormessage = $Lang::tr{'password has quotation mark'}; >> + goto VPNCONF_ERROR; >> + } >> if ($cgiparams{'CERT_PASS1'} ne $cgiparams{'CERT_PASS2'}) { >> $errormessage = $Lang::tr{'passwords do not match'}; >> goto VPNCONF_ERROR; >> @@ -2226,7 +2230,7 @@ END >> $opt .= " -inkey ${General::swroot}/certs/$cgiparams{'NAME'}key.pem"; >> $opt .= " -in ${General::swroot}/certs/$cgiparams{'NAME'}cert.pem"; >> $opt .= " -name \"$cgiparams{'NAME'}\""; >> - $opt .= " -passout pass:$cgiparams{'CERT_PASS1'}"; >> + $opt .= " -passout pass:\"$cgiparams{'CERT_PASS1'}\""; >> $opt .= " -certfile ${General::swroot}/ca/cacert.pem"; >> $opt .= " -caname \"$vpnsettings{'ROOTCERT_ORGANIZATION'} CA\""; >> $opt .= " -out ${General::swroot}/certs/$cgiparams{'NAME'}.p12"; >> -- >> 2.48.1 >> >> > > -- Sent from my laptop