Lost OpenVPN connection latest snap

[chpalmer]

Shared key.

I have not updated my second test box from the Monday snapshot yet.  If you would like I can do that and see if the same occurs.

[chpalmer]

Possibly?-

https://redmine.pfsense.org/projects/pfsense/repository/revisions/e4eed78198162119268fb9d11aeb73d9767930f6

[heper]

same happened to my pfsense @ home

@chpalmer seems unlikely that a change to a frontend page would result in a broken vpn-config …. haven't found anything else as of yet .

[phil.davis]

2.3.8 is the openvpn version - what is that about?

I was running a 10 Dec build that was gitsync'd up to date - all fine. I just upgraded to the latest build and then gitsync'd again to the current PHP code (the latest build seems to have the broken index.php thing in it). My 3 OpenVPN client site-to-site shared-key links up to 3 offices are working fine and the dashboard status shows them up. So I don't see any problem.

My previous version:

2.3-ALPHA (amd64) 
built on Thu Dec 10 15:16:10 CST 2015 
FreeBSD 10.2-STABLE 

OpenVPN 2.3.8 amd64-portbld-freebsd10.2 [SSL (OpenSSL)] [LZO] [MH] [IPv6] built on Aug 21 2015
library versions: OpenSSL 1.0.1q-freebsd 3 Dec 2015, LZO 2.09
Originally developed by James Yonan
Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sales@openvpn.net>Compile time defines: enable_crypto=yes enable_crypto_ofb_cfb=yes enable_debug=yes enable_def_auth=yes enable_dlopen=unknown enable_dlopen_self=unknown enable_dlopen_self_static=unknown enable_fast_install=needless enable_fragment=yes enable_http_proxy=yes enable_iproute2=no enable_libtool_lock=yes enable_lzo=yes enable_lzo_stub=no enable_management=yes enable_multi=yes enable_multihome=yes enable_pam_dlopen=no enable_password_save=yes enable_pedantic=no enable_pf=yes enable_pkcs11=no enable_plugin_auth_pam=yes enable_plugin_down_root=yes enable_plugins=yes enable_port_share=yes enable_selinux=no enable_server=yes enable_shared=yes enable_shared_with_static_runtimes=no enable_small=no enable_socks=yes enable_ssl=yes enable_static=yes enable_strict=no enable_strict_options=no enable_systemd=no enable_win32_dll=yes enable_x509_alt_username=no with_crypto_library=openssl with_gnu_ld=yes with_mem_check=no with_plugindir='$(libdir)/openvpn/plugins' with_sysroot=no</sales@openvpn.net> 

And my current version:

2.3-ALPHA (amd64) 
built on Thu Dec 17 02:00:21 CST 2015 
FreeBSD 10.2-STABLE 

OpenVPN 2.3.8 amd64-portbld-freebsd10.2 [SSL (OpenSSL)] [LZO] [MH] [IPv6] built on Aug 21 2015
library versions: OpenSSL 1.0.1q-freebsd 3 Dec 2015, LZO 2.09
Originally developed by James Yonan
Copyright (C) 2002-2010 OpenVPN Technologies, Inc. <sales@openvpn.net>Compile time defines: enable_crypto=yes enable_crypto_ofb_cfb=yes enable_debug=yes enable_def_auth=yes enable_dlopen=unknown enable_dlopen_self=unknown enable_dlopen_self_static=unknown enable_fast_install=needless enable_fragment=yes enable_http_proxy=yes enable_iproute2=no enable_libtool_lock=yes enable_lzo=yes enable_lzo_stub=no enable_management=yes enable_multi=yes enable_multihome=yes enable_pam_dlopen=no enable_password_save=yes enable_pedantic=no enable_pf=yes enable_pkcs11=no enable_plugin_auth_pam=yes enable_plugin_down_root=yes enable_plugins=yes enable_port_share=yes enable_selinux=no enable_server=yes enable_shared=yes enable_shared_with_static_runtimes=no enable_small=no enable_socks=yes enable_ssl=yes enable_static=yes enable_strict=no enable_strict_options=no enable_systemd=no enable_win32_dll=yes enable_x509_alt_username=no with_crypto_library=openssl with_gnu_ld=yes with_mem_check=no with_plugindir='$(libdir)/openvpn/plugins' with_sysroot=no</sales@openvpn.net> 

[heper]

/var/etc/openvpn/client1.conf after upgrade yesterday

dev ovpnc1
verb 1
dev-type tun
tun-ipv6
dev-node /dev/tun1
writepid /var/run/openvpn_client1.pid
#user nobody
#group nobody
script-security 3
daemon
keepalive 10 60
ping-timer-rem
persist-tun
persist-key
proto udp
cipher AES-256-CBC
auth SHA1
up /usr/local/sbin/ovpn-linkup
down /usr/local/sbin/ovpn-linkdown
local 78.xx.xx.170
lport 0
management /var/etc/openvpn/client1.sock unix
remote 81.yy.yy.190 1234
ifconfig
secret /var/etc/openvpn/client1.secret
comp-lzo adaptive

just got home, trying to figure out why that is

[heper]

for whatever reason openvpn_get_interface_ip() is returning empty strings (see L929->L938 in openvpn.inc)

debugging from console, make it clear that $ip1 & $ip2 are empty:

pfSense shell: require_once('util.inc');
pfSense shell: $baselong = ip2long32('192.168.222.0') & ip2long('255.255.255.248');
pfSense shell: $ip1 = long2ip32($baselong + 1);
pfSense shell: $ip2 = long2ip32($baselong + 2);
pfSense shell: echo $baselong;
pfSense shell: echo $ip1;
pfSense shell: echo $ip2;
pfSense shell: exec 
-1062674944pfSense shell: 

problem might be in long2ip32() located in utils.inc

the quest continues ;)

[heper]

found the culprit, too stupid to fix it. reverting to the old code fixes the issue.
https://github.com/pfsense/pfsense/commit/ce9dc19873669c8bc3c420e7c57dde3d48d8e64a#diff-d45d58361f1c16219dd36fc4951f76dbR433

not sure why we got hit by this today since the commit is from 15 days ago, and i update every 2-3 days.

anyways, someone please correct that commit.

I'm running 32bit @ home, perhaps that the reason the issue hasn't hit everyone?

[Steve_B]

I have manually reverted PR 2152 for now.

[stilez]

@heper:

for whatever reason openvpn_get_interface_ip() is returning empty strings (see L929->L938 in openvpn.inc)

debugging from console, make it clear that $ip1 & $ip2 are empty:

pfSense shell: require_once('util.inc');
pfSense shell: $baselong = ip2long32('192.168.222.0') & ip2long('255.255.255.248');
pfSense shell: $ip1 = long2ip32($baselong + 1);
pfSense shell: $ip2 = long2ip32($baselong + 2);
pfSense shell: echo $baselong;
pfSense shell: echo $ip1;
pfSense shell: echo $ip2;
pfSense shell: exec 
-1062674944pfSense shell: 

```problem might be in long2ip32() located in utils.inc

Thanks for the test snippet above.  I wrote the commit that's highlighted in this thread.

There is a chance that your test snippet may have had an error in the second line:- "$baselong = ip2long32('192.168.222.0') & ip2long('255.255.255.248')").

Surely the second call in your test code should also have been ip2long32() and not one call of ip2long32() and another call of ip2long()?  Can you recheck. I used your test snippet in the GUI command prompt of 2.3, to compare the same input data you used on the pre-commit and post-commit code. When I corrected this line, they gave identical and correct answers.  So it might be a false alert caused by bad debug code (i.e., the error may be elsewhere).

Here is the corrected test snippet I used to check your finding. As you can see, when your debug code was changed to call ip2long32() on that line, the result is correct, suggesting the issue is probably elsewhere and this is a false alert on that function…..

// included in case any dependencies, though there shouldn't be any
require_once('util.inc');

// version A, the pre-commit code
function ip2long32_a($ip) {
	return (ip2long($ip) & 0xFFFFFFFF); 
}

//version B, the newly committed code
function ip2long32_b($ip) {
	$a = ip2long($ip);
	return ($a === False ? '' : $a & 0xFFFFFFFF);
}

echo "test HEPER's snippet and input data using ip2long32() version A\n";
$baselong = ip2long32_a('192.168.222.0') & ip2long_a('255.255.255.248');
$ip1 = long2ip32($baselong + 1);
$ip2 = long2ip32($baselong + 2);
echo "baselong: [{$baselong}]\n";
echo "ip1: [{$ip1}]\n";
echo "ip2: [{$ip2}]\n";

echo "\ntest HEPER's snippet and input data using ip2long32() version B\n";
$baselong = ip2long32_b('192.168.222.0') & ip2long_b('255.255.255.248');
$ip1 = long2ip32($baselong + 1);
$ip2 = long2ip32($baselong + 2);
echo "baselong: [{$baselong}]\n";
echo "ip1: [{$ip1}]\n";
echo "ip2: [{$ip2}]\n";

Result: 

test HEPER's snippet and input data using ip2long32() version A
baselong: [3232292352]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

test HEPER's snippet and input data using ip2long32() version B
baselong: [3232292352]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

For info, all the modded code does, is test whether ip2long() returned successfully, before assuming success and passing the return value down the line (it may not succeed in some circumstances). Everything else, including the calculation method used, is identical.

[heper]

@stilez
the problem doesn't exist in ip2long32() … the problem exists in long2ip32()

see:

• https://github.com/pfsense/pfsense/blob/master/src/etc/inc/openvpn.inc#L932 <– call to openvpn_get_interface_ip() that returned empty

• https://github.com/pfsense/pfsense/blob/master/src/etc/inc/openvpn.inc#L1706-L1711 <–- function that i recreated in my console debugging

$baselong was/is correct.
$ip1 & ip2  on lines 1708-1709 of openvpn.inc were emty after your commit and thus the function, returned empty. this caused the openvpn client conf to become invalid.

i redid your script to show you the differences in long2ip32 (i ran it in /usr/local/www through browser):

// included in case any dependencies, though there shouldn't be any
require_once('util.inc');
require_once('openvpn.inc');

function long2ip32_a($ip) {
        return long2ip($ip & 0xFFFFFFFF);
//      return ((is_int($ip) && ($ip >> 32) == 0) ? long2ip($ip & 0xFFFFFFFF) : '');
 }
function long2ip32_b($ip) {
//        return long2ip($ip & 0xFFFFFFFF);
      return ((is_int($ip) && ($ip >> 32) == 0) ? long2ip($ip & 0xFFFFFFFF) : '');
 }

echo "test HEPER's snippet and input data using long2ip32() version A
";
$baselong = ip2long32('192.168.222.0') & ip2long('255.255.255.248');
$ip1 = long2ip32_a($baselong + 1);
$ip2 = long2ip32_a($baselong + 2);
echo "baselong: [{$baselong}]
";
echo "ip1: [{$ip1}]
";
echo "ip2: [{$ip2}]
";

echo "
test HEPER's snippet and input data using long2ip32() version B
";
$baselong = ip2long32('192.168.222.0') & ip2long('255.255.255.248');
$ip1 = long2ip32_b($baselong + 1);
$ip2 = long2ip32_b($baselong + 2);
echo "baselong: [{$baselong}]
";
echo "ip1: [{$ip1}]
";
echo "ip2: [{$ip2}]
";
?>

output:

test HEPER's snippet and input data using long2ip32() version A
baselong: [-1062674944]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

test HEPER's snippet and input data using long2ip32() version B
baselong: [-1062674944]
ip1: []
ip2: []

I notice in your testing, you are running on 64bit(=positive longs).  I'm experiencing this on 32bit and perhaps, thats where your version of long2ip32() fails & original works ?

[stilez]

Interesting. I run the exact snippet you gave (zero modification) in the GUI diagnostic -> command prompt shell, and got this:

test HEPER's snippet and input data using long2ip32() version A
baselong: [3232292352]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

test HEPER's snippet and input data using long2ip32() version B
baselong: [3232292352]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

Hard to see how it's an x32/x64 issue, but I'm inclined to say it must be.
As I don't have x32 around here, can you check out for me what you get with the following modification:

function long2ip32_b($ip) {
      echo "DEBUG:";
      $data = array ($ip, is_int($ip), $ip >> 32, (($ip >> 32) == 0), (is_int($ip) && ($ip >> 32) == 0));
      var_dump($data);
      return ((is_int($ip) && ($ip >> 32) == 0) ? long2ip($ip & 0xFFFFFFFF) : '');
 }

On x64 I get:

[ 0] => int(3232292354)
[1] => bool(true)
[2] => int(0)
[3] => bool(true)
[4] => bool(true)

[heper]

DEBUGarray(5) {
[ 0] => int(-1062674943)
[1] => bool(true)
[2] => int(-1062674943)
[3] => bool(false)
[4] => bool(false) }

DEBUG:array(5) {
[ 0] => int(-1062674942)
[1] => bool(true)
[2] => int(-1062674942)
[3] => bool(false)
[4] => bool(false) }

[stilez]

So the shift right operator works differently in x32 and x64. >>32 should shift right by 32 bits and should clear any 32 bit value, and therefore confirm if the input has any data in bits >=33 for validity.  But on x32 it's not shifting at all. That's…. unexpected, but easily fixed. That'll be it.

Update:
http://php.net/manual/en/language.operators.bitwise.php small print confirms:  "Shifting integers by values greater than or equal to the system long integer width results in undefined behavior. In other words, don't shift more than 31 bits on a 32-bit system, and don't shift more than 63 bits on a 64-bit system".
How unsatisfactory. But easily fixed. Replace shift >>32 by an equivalent test. Might need you to verify the result of a few possible variants, on x32.

I'm just cautious about x32 systems that might nonetheless handle integers >32 bits. In that case you wouldn't be able to rely on assuming (it's an integer) -> (it's a valid x32 value),, so the function would still need a test whether the input is within 2^32 limits. The easiest test is that if you shift out the rightmost 32 bits, do you have zeros left. But if that won't work on x32 then alternatives are easy.

Other tests would probably need to check MAXINT or system bitness instead (ie "SYSTEM INT is 32 bits wide OR $ip >>32 == 0"), because both logical AND, and directly testing the value of the integer, might share the same issues as >>32 does on x32.

What happens if the B version is replaced by this, which isn't so elegant but is functionally equivalent? Does it work for you then?

function long2ip32_b($ip) {
      // replace >> 32 by >> 16 twice, due to >>32 issues on x32 platforms
      echo "DEBUG:";
      $data = array ($ip, is_int($ip), ($ip >> 16) >> 16, ((($ip >> 16) >> 16) == 0), (is_int($ip) && (($ip >> 16) >> 16) == 0));
      var_dump($data);
      return ((is_int($ip) && (($ip >> 16) >> 16) == 0) ? long2ip($ip & 0xFFFFFFFF) : '');
 }

[heper]

DEBUG:array(5) {
[ 0] => int(-1062674943)
[1] => bool(true)
[2] => int(-1)
[3] => bool(false)
[4] => bool(false) }

DEBUG:array(5) {
[ 0] => int(-1062674942)
[1] => bool(true)
[2] => int(-1)
[3] => bool(false)
[4] => bool(false) }

baselong: [-1062674944]
ip1: []
ip2: []

so no, doesn't work

[stilez]

Intriguing. I wonder how on earth, a right bit shift ends up with a 1-filled value (-1 binary), when every other shift operation zero fills. Never mind. MAX_INT should be robust and bitwise NOT should automatically adapt to the system integer size. Can you try again with these two versions:

function long2ip32_b($ip) {
      // ">> 32" only works when INT SIZE > 4 bytes, but if <=4 bytes it's automatically within IPv4 limits anyway
      echo "DEBUG:";
      $data = array ($ip, is_int($ip), PHP_INT_SIZE, ($ip >> 32), (($ip >> 32) == 0), (is_int($ip) && (PHP_INT_SIZE <= 4 || ($ip >> 32) == 0)));
      var_dump($data);
      return ((is_int($ip) && (PHP_INT_SIZE <= 4 || ($ip >> 32) == 0)) ? long2ip($ip & 0xFFFFFFFF) : '');
 }

and:

function long2ip32_b($ip) {
      echo "DEBUG:";
      $data = array ($ip, is_int($ip), ($ip & ~0xFFFFFFFF), (($ip & ~0xFFFFFFFF) == 0), (is_int($ip) && ($ip & ~0xFFFFFFFF) == 0));
      var_dump($data);
      return ((is_int($ip) && ($ip & ~0xFFFFFFFF) == 0) ? long2ip($ip & 0xFFFFFFFF) : '');
 }

One, probably both, should have a chance of working

[heper]

both seem to work (called them B & C)

test HEPER's snippet and input data using long2ip32() version A
baselong: [-1062674944]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

test HEPER's snippet and input data using long2ip32() version B
DEBUG:
array(6) { [0] => int(-1062674943) [1] => bool(true) [2] => int(4) [3] => int(-1062674943) [4] => bool(false) [5] => bool(true) }
DEBUG:
array(6) { [0] => int(-1062674942) [1] => bool(true) [2] => int(4) [3] => int(-1062674942) [4] => bool(false) [5] => bool(true) } baselong: [-1062674944]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

test HEPER's snippet and input data using long2ip32() version C
DEBUG:
array(5) { [0] => int(-1062674943) [1] => bool(true) [2] => int(0) [3] => bool(true) [4] => bool(true) }
DEBUG:
array(5) { [0] => int(-1062674942) [1] => bool(true) [2] => int(0) [3] => bool(true) [4] => bool(true) } baselong: [-1062674944]
ip1: [192.168.222.1]
ip2: [192.168.222.2]

[stilez]

PR#2317 submitted with the above fix that seems to now work properly on x32 and x64:  https://github.com/pfsense/pfsense/pull/2317

Thanks for the heads up and help bugfixing it!

[heper]

thats what we are here for ;)

[Steve_B]

PR has been merged. Thanks for the detective work. Seems PHP always has a new quirk up its sleeve :)