[PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.

[PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.

[Erik Kapfer]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

- If the OpenSSL maximum of '999999' will be exceeded over the WUI, the entry in
	OpenVPNs database index.txt will be written without a timestamp
	and crashes the database which blocks the creation of new clients.
	To prevent this, a check has been set which restricts the data field
	of 'valid til days' to '6' numerics.

Fixes: #10482
---
 html/cgi-bin/ovpnmain.cgi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
index ceb88c1..8f45f04 100644
--- a/html/cgi-bin/ovpnmain.cgi
+++ b/html/cgi-bin/ovpnmain.cgi
@@ -4039,6 +4039,14 @@ if ($cgiparams{'TYPE'} eq 'net') {
       		goto VPNCONF_ERROR;
 	}
 
+	# Check that OpenSSL maximum of valid days won´t be exceeded
+	if (length($cgiparams{'DAYS_VALID'}) > 6) {
+		$errormessage = $Lang::tr{'invalid input for valid till days'};
+		unlink ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.conf") or die "Removing Configfile fail: $!";
+		rmdir ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing Directory fail: $!";
+		goto VPNCONF_ERROR;
+	}
+
 	if ($cgiparams{'ENABLED'} !~ /^(on|off)$/) {
 	    $errormessage = $Lang::tr{'invalid input'};
 	    goto VPNCONF_ERROR;
@@ -4221,6 +4229,12 @@ if ($cgiparams{'TYPE'} eq 'net') {
 		goto VPNCONF_ERROR;
 	    }
 
+		# Check that OpenSSL maximum of valid days won´t be exceeded
+		if (length($cgiparams{'DAYS_VALID'}) > 6) {
+			$errormessage = $Lang::tr{'invalid input for valid till days'};
+			goto VPNCONF_ERROR;
+		}
+
 	    # Replace empty strings with a .
 	    (my $ou = $cgiparams{'CERT_OU'}) =~ s/^\s*$/\./;
 	    (my $city = $cgiparams{'CERT_CITY'}) =~ s/^\s*$/\./;
-- 
2.7.4

Re: [PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

Hi,

On Sat, 2017-11-11 at 10:45 +0100, Erik Kapfer wrote:
> - If the OpenSSL maximum of '999999' will be exceeded over the WUI, the entry in
> 	OpenVPNs database index.txt will be written without a timestamp
> 	and crashes the database which blocks the creation of new clients.
> 	To prevent this, a check has been set which restricts the data field
> 	of 'valid til days' to '6' numerics.
> 
> Fixes: #10482
> ---
>  html/cgi-bin/ovpnmain.cgi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index ceb88c1..8f45f04 100644
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -4039,6 +4039,14 @@ if ($cgiparams{'TYPE'} eq 'net') {
>        		goto VPNCONF_ERROR;
>  	}
>  
> +	# Check that OpenSSL maximum of valid days won´t be exceeded
> +	if (length($cgiparams{'DAYS_VALID'}) > 6) {
> +		$errormessage = $Lang::tr{'invalid input for valid till days'};
> +		unlink ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.conf") or die "Removing Configfile fail: $!";
> +		rmdir ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing Directory fail: $!";
> +		goto VPNCONF_ERROR;
> +	}
> +

I think it would be better just to check if DAYS_VALID is less then
999999. Checking the length of the string wasn't really obvious for me
what was actually going to be achieved here.

>  	if ($cgiparams{'ENABLED'} !~ /^(on|off)$/) {
>  	    $errormessage = $Lang::tr{'invalid input'};
>  	    goto VPNCONF_ERROR;
> @@ -4221,6 +4229,12 @@ if ($cgiparams{'TYPE'} eq 'net') {
>  		goto VPNCONF_ERROR;
>  	    }
>  
> +		# Check that OpenSSL maximum of valid days won´t be exceeded
> +		if (length($cgiparams{'DAYS_VALID'}) > 6) {
> +			$errormessage = $Lang::tr{'invalid input for valid till days'};
> +			goto VPNCONF_ERROR;
> +		}
> +
>  	    # Replace empty strings with a .
>  	    (my $ou = $cgiparams{'CERT_OU'}) =~ s/^\s*$/\./;
>  	    (my $city = $cgiparams{'CERT_CITY'}) =~ s/^\s*$/\./;

-Michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH] - Added "'TYPE' net" to N2N section since it can only be changed there. - Check for N2N sets days valid maximum to '999999' days. - Check for Roadwarrior sets days valid maximum to '999999' days. - If '999999' days will be exceeded, a warning will be displayed.

[Erik Kapfer]

[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]

---
 html/cgi-bin/ovpnmain.cgi | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
index 8f45f04..9c383f7 100644
--- a/html/cgi-bin/ovpnmain.cgi
+++ b/html/cgi-bin/ovpnmain.cgi
@@ -4040,11 +4040,13 @@ if ($cgiparams{'TYPE'} eq 'net') {
 	}
 
 	# Check that OpenSSL maximum of valid days won´t be exceeded
-	if (length($cgiparams{'DAYS_VALID'}) > 6) {
-		$errormessage = $Lang::tr{'invalid input for valid till days'};
-		unlink ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.conf") or die "Removing Configfile fail: $!";
-		rmdir ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing Directory fail: $!";
-		goto VPNCONF_ERROR;
+	if ($cgiparams{'TYPE'} eq 'net') {
+		if ($cgiparams{'DAYS_VALID'} > '999999') {
+			$errormessage = $Lang::tr{'invalid input for valid till days'};
+			unlink ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.conf") or die "Removing Configfile fail: $!";
+			rmdir ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing Directory fail: $!";
+			goto VPNCONF_ERROR;
+		}
 	}
 
 	if ($cgiparams{'ENABLED'} !~ /^(on|off)$/) {
@@ -4230,7 +4232,7 @@ if ($cgiparams{'TYPE'} eq 'net') {
 	    }
 
 		# Check that OpenSSL maximum of valid days won´t be exceeded
-		if (length($cgiparams{'DAYS_VALID'}) > 6) {
+		if ($cgiparams{'DAYS_VALID'} > '999999') {
 			$errormessage = $Lang::tr{'invalid input for valid till days'};
 			goto VPNCONF_ERROR;
 		}
-- 
2.7.4

Re: [PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.

[ummeegge]

[-- Attachment #1: Type: text/plain, Size: 2696 bytes --]

Hi Michael,
have now seen the format of the sended patch --> https://lists.ipfire.org/pipermail/development/2017-November/003732.html is somehow broken (seems that sendmail and me have some incompatibilities :-| ) whereby in Git --> https://git.ipfire.org/?p=people/ummeegge/ipfire-2.x.git;a=commit;h=3e58db4871f707f6ea79e6f8ca219ee03008fe76 is it OK… Am currently not sure what i did wrong there. 
Michael, if the format is useless let it me know will try then to send it again...


Best,

Erik


Am 12.11.2017 um 13:15 schrieb Michael Tremer:

> Hi,
> 
> On Sat, 2017-11-11 at 10:45 +0100, Erik Kapfer wrote:
>> - If the OpenSSL maximum of '999999' will be exceeded over the WUI, the entry in
>> 	OpenVPNs database index.txt will be written without a timestamp
>> 	and crashes the database which blocks the creation of new clients.
>> 	To prevent this, a check has been set which restricts the data field
>> 	of 'valid til days' to '6' numerics.
>> 
>> Fixes: #10482
>> ---
>> html/cgi-bin/ovpnmain.cgi | 14 ++++++++++++++
>> 1 file changed, 14 insertions(+)
>> 
>> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
>> index ceb88c1..8f45f04 100644
>> --- a/html/cgi-bin/ovpnmain.cgi
>> +++ b/html/cgi-bin/ovpnmain.cgi
>> @@ -4039,6 +4039,14 @@ if ($cgiparams{'TYPE'} eq 'net') {
>>       		goto VPNCONF_ERROR;
>> 	}
>> 
>> +	# Check that OpenSSL maximum of valid days won´t be exceeded
>> +	if (length($cgiparams{'DAYS_VALID'}) > 6) {
>> +		$errormessage = $Lang::tr{'invalid input for valid till days'};
>> +		unlink ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.conf") or die "Removing Configfile fail: $!";
>> +		rmdir ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing Directory fail: $!";
>> +		goto VPNCONF_ERROR;
>> +	}
>> +
> 
> I think it would be better just to check if DAYS_VALID is less then
> 999999. Checking the length of the string wasn't really obvious for me
> what was actually going to be achieved here.
> 
>> 	if ($cgiparams{'ENABLED'} !~ /^(on|off)$/) {
>> 	    $errormessage = $Lang::tr{'invalid input'};
>> 	    goto VPNCONF_ERROR;
>> @@ -4221,6 +4229,12 @@ if ($cgiparams{'TYPE'} eq 'net') {
>> 		goto VPNCONF_ERROR;
>> 	    }
>> 
>> +		# Check that OpenSSL maximum of valid days won´t be exceeded
>> +		if (length($cgiparams{'DAYS_VALID'}) > 6) {
>> +			$errormessage = $Lang::tr{'invalid input for valid till days'};
>> +			goto VPNCONF_ERROR;
>> +		}
>> +
>> 	    # Replace empty strings with a .
>> 	    (my $ou = $cgiparams{'CERT_OU'}) =~ s/^\s*$/\./;
>> 	    (my $city = $cgiparams{'CERT_CITY'}) =~ s/^\s*$/\./;
> 
> -Michael

Re: [PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]

Hi,

yes, it would have been better to send a patch against next instead of a patch
for the other patch. That makes it a lot harder to see what has actually
changed.

I suppose you can squash both commits together to one and then just send it by
using "git send-email -1 --to development(a)lists.ipfire.org".

https://stackoverflow.com/questions/2563632/how-can-i-merge-two-commits-into-one

Best,
-Michael

On Tue, 2017-11-14 at 14:20 +0100, ummeegge wrote:
> Hi Michael,
> have now seen the format of the sended patch --> https://lists.ipfire.org/pipe
> rmail/development/2017-November/003732.html is somehow broken (seems that
> sendmail and me have some incompatibilities :-| ) whereby in Git --> https://g
> it.ipfire.org/?p=people/ummeegge/ipfire-
> 2.x.git;a=commit;h=3e58db4871f707f6ea79e6f8ca219ee03008fe76 is it OK… Am
> currently not sure what i did wrong there. 
> Michael, if the format is useless let it me know will try then to send it
> again...
> 
> 
> Best,
> 
> Erik
> 
> 
> Am 12.11.2017 um 13:15 schrieb Michael Tremer:
> 
> > Hi,
> > 
> > On Sat, 2017-11-11 at 10:45 +0100, Erik Kapfer wrote:
> > > - If the OpenSSL maximum of '999999' will be exceeded over the WUI, the
> > > entry in
> > > 	OpenVPNs database index.txt will be written without a timestamp
> > > 	and crashes the database which blocks the creation of new clients.
> > > 	To prevent this, a check has been set which restricts the data field
> > > 	of 'valid til days' to '6' numerics.
> > > 
> > > Fixes: #10482
> > > ---
> > > html/cgi-bin/ovpnmain.cgi | 14 ++++++++++++++
> > > 1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> > > index ceb88c1..8f45f04 100644
> > > --- a/html/cgi-bin/ovpnmain.cgi
> > > +++ b/html/cgi-bin/ovpnmain.cgi
> > > @@ -4039,6 +4039,14 @@ if ($cgiparams{'TYPE'} eq 'net') {
> > >       		goto VPNCONF_ERROR;
> > > 	}
> > > 
> > > +	# Check that OpenSSL maximum of valid days won´t be exceeded
> > > +	if (length($cgiparams{'DAYS_VALID'}) > 6) {
> > > +		$errormessage = $Lang::tr{'invalid input for valid till
> > > days'};
> > > +		unlink
> > > ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}/$cgiparams{'NAME'}.co
> > > nf") or die "Removing Configfile fail: $!";
> > > +		rmdir
> > > ("${General::swroot}/ovpn/n2nconf/$cgiparams{'NAME'}") || die "Removing
> > > Directory fail: $!";
> > > +		goto VPNCONF_ERROR;
> > > +	}
> > > +
> > 
> > I think it would be better just to check if DAYS_VALID is less then
> > 999999. Checking the length of the string wasn't really obvious for me
> > what was actually going to be achieved here.
> > 
> > > 	if ($cgiparams{'ENABLED'} !~ /^(on|off)$/) {
> > > 	    $errormessage = $Lang::tr{'invalid input'};
> > > 	    goto VPNCONF_ERROR;
> > > @@ -4221,6 +4229,12 @@ if ($cgiparams{'TYPE'} eq 'net') {
> > > 		goto VPNCONF_ERROR;
> > > 	    }
> > > 
> > > +		# Check that OpenSSL maximum of valid days won´t be
> > > exceeded
> > > +		if (length($cgiparams{'DAYS_VALID'}) > 6) {
> > > +			$errormessage = $Lang::tr{'invalid input for
> > > valid till days'};
> > > +			goto VPNCONF_ERROR;
> > > +		}
> > > +
> > > 	    # Replace empty strings with a .
> > > 	    (my $ou = $cgiparams{'CERT_OU'}) =~ s/^\s*$/\./;
> > > 	    (my $city = $cgiparams{'CERT_CITY'}) =~ s/^\s*$/\./;
> > 
> > -Michael
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]