Re: [PATCH 1/2] vpnmain.cgi: Fixes bug12298 - IPSec password cannot use semicolon

[Adolf Belka <adolf.belka@ipfire.org>]

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 <adolf.belka@ipfire.org> 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 <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 => 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