| Message ID | 1510393507-15218-1-git-send-email-erik.kapfer@ipfire.org |
|---|---|
| State | Dropped |
| Headers |
Return-Path: <development-bounces@lists.ipfire.org> Received: from mail01.ipfire.org (unknown [172.28.1.200]) by web02.ipfire.org (Postfix) with ESMTP id 6A78F60D81 for <patchwork@ipfire.org>; Sat, 11 Nov 2017 10:46:43 +0100 (CET) Received: from mail01.ipfire.org (localhost [IPv6:::1]) by mail01.ipfire.org (Postfix) with ESMTP id 8CF0934FF; Sat, 11 Nov 2017 10:46:42 +0100 (CET) Received: from localhost.localdomain (p5DC0B6C7.dip0.t-ipconnect.de [93.192.182.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (Client did not present a certificate) by mail01.ipfire.org (Postfix) with ESMTPSA id 242F434FF; Sat, 11 Nov 2017 10:45:28 +0100 (CET) From: Erik Kapfer <erik.kapfer@ipfire.org> To: development@lists.ipfire.org Subject: [PATCH] OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity. Date: Sat, 11 Nov 2017 10:45:07 +0100 Message-Id: <1510393507-15218-1-git-send-email-erik.kapfer@ipfire.org> X-Mailer: git-send-email 2.7.4 MIME-Version: 1.0 Content-Type: text/plain; charset=y Content-Transfer-Encoding: 8bit X-BeenThere: development@lists.ipfire.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: IPFire development talk <development.lists.ipfire.org> List-Unsubscribe: <https://lists.ipfire.org/mailman/options/development>, <mailto:development-request@lists.ipfire.org?subject=unsubscribe> List-Archive: <https://lists.ipfire.org/pipermail/development/> List-Post: <mailto:development@lists.ipfire.org> List-Help: <mailto:development-request@lists.ipfire.org?subject=help> List-Subscribe: <https://lists.ipfire.org/mailman/listinfo/development>, <mailto:development-request@lists.ipfire.org?subject=subscribe> Errors-To: development-bounces@lists.ipfire.org Sender: "Development" <development-bounces@lists.ipfire.org> |
| Series |
OpenVPN: Fix to prevent exceedance of OpenSSLs max. validity.
|
|
Commit Message
Erik Kapfer
Nov. 11, 2017, 8:45 p.m. UTC
- 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(+)
Comments
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
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
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@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 > >
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*$/\./;