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

  - 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
  

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.

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
> 
>
  

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
>>
>>
> 
>
  

Hello,

> On 10 Mar 2025, at 10:48, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> 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.

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.
> 
> 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.

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

> 
> 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
  

Well I think anytime you create an "enter password" dialog/process, the
user input should be encoded.
I see that this issue with passwords was not the only case. The other I saw
that I think eventually it got fixed was the blue wireless password
dialog/process.
I know HTTP auth has similar flaws, but at least you can limit where the
web GUI is accessible.


On Mon, Mar 10, 2025 at 5:15 AM Michael Tremer <michael.tremer@ipfire.org>
wrote:

> Hello,
>
> > On 10 Mar 2025, at 10:48, Adolf Belka <adolf.belka@ipfire.org> wrote:
> >
> > 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.
>
> 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.
> >
> > 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.
>
> 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
>
> >
> > 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
>
>
>
>