Use a TUN device and embedded PPP code, instead of using pppd

[DimitriPapadopoulos]

This patch provided by @rain2fog in #801 (comment) works for me. I get a working VPN tunnel without using pppd. I wouldn't mind help with reviewing.

From #801 (comment):

we simulated the PPP negotiation procedure. There are 3 stages in PPP protocal.

• The 1th stage is LCP. client will send Magic-Number and MRU request to fortivpn server, then will ack the server request. server will send the Magic-Number and ack the client request.
• The 2th stage is IPCP. both will will send the IPCP config-request to other. client need to send the IPCP config-request Option (3) with the IP which come from the XML. then both config-request need to be ack.
• The 3th stage is 0x0021. we can read and write IP packet from tun NIC. then sending data by encapsulating or decapsulating the IP packet with header 0x0021(2 Bytes) + (totolsize(2 Bytes) + 0x5050 + pppsize(2 Bytes)).
  we also need send the LCP echo-request to fortinet VPN to keep the TCP connection wouldn't close.

I get complier warnings on Ubuntu 22.04:

In function ‘conf_option_encode’,
    inlined from ‘conf_request’ at src/io.c:415:2:
src/io.c:337:16: warning: ‘request.tail’ may be used uninitialized [-Wmaybe-uninitialized]
  337 |         optlist->tail->type = type;
      |         ~~~~~~~^~~~~~
src/io.c: In function ‘conf_request’:
src/io.c:410:33: note: ‘request’ declared here
  410 |         struct conf_option_list request;
      |                                 ^~~~~~~
In function ‘conf_option_encode’,
    inlined from ‘ipcp_packet’ at src/io.c:984:4:
src/io.c:337:16: warning: ‘request.tail’ may be used uninitialized [-Wmaybe-uninitialized]
  337 |         optlist->tail->type = type;
      |         ~~~~~~~^~~~~~
src/io.c: In function ‘ipcp_packet’:
src/io.c:981:49: note: ‘request’ declared here
  981 |                         struct conf_option_list request;
      |                                                 ^~~~~~~
In function ‘conf_option_encode’,
    inlined from ‘ipcp_packet’ at src/io.c:1038:4:
src/io.c:337:16: warning: ‘request.tail’ may be used uninitialized [-Wmaybe-uninitialized]
  337 |         optlist->tail->type = type;
      |         ~~~~~~~^~~~~~
src/io.c: In function ‘ipcp_packet’:
src/io.c:1035:49: note: ‘request’ declared here
 1035 |                         struct conf_option_list request;
      |                                                 ^~~~~~~

Initially opened as #818.

Fixes #801.

[DimitriPapadopoulos]

I get these warnings while setting routes after connection:

WARN:   Could not get route to gateway (Route not found).
WARN:   Protecting tunnel route has failed. But this can be working except for some cases.
WARN:   Adding route table is incomplete. Please check route table.

I get this error when cleaning up because openfortivpn still attempts to terminate `pppd``:

ERROR:  waitpid: No child processes
INFO:   Terminated pppd.

[relaxlex]

Hi @DimitriPapadopoulos,

thank you for applying the TUN patch from @rain2fog. I have just ran a couple of short tests and this appears to work like a charm. Because I use openfortivpn daily I will use this tun branch build from now on to see how it performs. Because I can create a tun device as a normal user I will be running openfortivpn without sudo.
Only difference I observed between the TUN and pppd version is that the TUN version is seeing a lot of
"Route to gateway exists already." warnings when connecting to the same Forti VPN server.

Cheers!

[relaxlex]

Hi @DimitriPapadopoulos,

thank you for the update, will test further with this rev. g2dec255 version. First glance TUN tunnel still working and Route to gateway exists already warnings are also still there.

Cheers!

[DimitriPapadopoulos]

Yes, I probably won't have time to look into the warnings any time soon. You are welcome to suggest a pull request to fix them.

[DimitriPapadopoulos]

Fixed compiler warnings, just took a couple assert calls.

[caeies]

Thanks for the merge.

@DimitriPapadopoulos : argh, sorry for the spell issues. I miss this test (I only ran run.sh under test/lint/run.sh)

The third restart of the VPN show a small leak, but at least this part is working. I will try to see if I can build something with valgrind to identify the leak. If you have some chance to get this branch running coverity it could be probably good to check for easy to find errors :).

Best regards,

[caeies]

During the WE, the mem leak seems to be very minor (after 9 restarts).

However I saw a strange log order in my output and I will try to dig this in the coming days:

INFO: Establishing the tunnel
INFO: interface created
INFO: setup ip addr to 10.10.10.10
INFO: Got addresses: [10.10.10.10], ns [10.10.10.1, 0.0.0.0]
INFO: Negotiation complete.
INFO: setup ip addr to 10.10.10.10
INFO: Interface pppVPN is UP.
INFO: Setting new routes...
WARN: Removing wrong route to vpn server...
INFO: Adding VPN nameservers...
WARN: Removing wrong route to vpn server...
INFO: Tunnel is up and running.
INFO: Cancelling threads...
INFO: Cleanup, joining threads...
INFO: Setting pppVPN interface down.
INFO: Removing VPN nameservers...

We should only have the one after the Setting new routes, not after the Adding VPN nameservers ... ...

[caeies]

Something is broken on the last code version. it drops the default route even if set-routes = 0 and failed to restore it on tunnel restoration ... maybe something broken with unitialized memory ...

Edit:

Ok I found the issue. Please take care when doing push -f to not drop some commits :/ I don't know where the commit was lost (could be on my side too), but clearly, things get broken at some point :/. Will update my tun branch again in a few minutes.

[DimitriPapadopoulos]

There's one last Coverity Scan issue in the TUN code from rain2fog:

openfortivpn/src/http.c

Lines 873 to 878 in 93b89ca

	if (!gateway)
	log_warn("No gateway address, using interface for routing\n");
	if (tunnel->config->tun) {
	tunnel->ipv4.ip_addr.s_addr = inet_addr(gateway);
	tunnel->ipv4.peer_addr.s_addr = inet_addr("192.0.2.1");

CID 364257 Dereference after null check (FORWARD_NULL)
Passing null pointer gateway to inet_addr, which dereferences it.

@caeies It would be great if you could look into it. I am fixing other issues.

[caeies]

There's one last Coverity Scan issue in the TUN code from rain2fog:

openfortivpn/src/http.c

Lines 873 to 878 in 93b89ca

	if (!gateway)
	log_warn("No gateway address, using interface for routing\n");
	if (tunnel->config->tun) {
	tunnel->ipv4.ip_addr.s_addr = inet_addr(gateway);
	tunnel->ipv4.peer_addr.s_addr = inet_addr("192.0.2.1");

CID 364257 Dereference after null check (FORWARD_NULL) Passing null pointer gateway to inet_addr, which dereferences it.

@caeies It would be great if you could look into it. I am fixing other issues.

Sorry I missed your comment (I was focused on memory leaks). Adding this ASAP.

[caeies]

There's one last Coverity Scan issue in the TUN code from rain2fog:

openfortivpn/src/http.c

Lines 873 to 878 in 93b89ca

	if (!gateway)
	log_warn("No gateway address, using interface for routing\n");
	if (tunnel->config->tun) {
	tunnel->ipv4.ip_addr.s_addr = inet_addr(gateway);
	tunnel->ipv4.peer_addr.s_addr = inet_addr("192.0.2.1");

CID 364257 Dereference after null check (FORWARD_NULL) Passing null pointer gateway to inet_addr, which dereferences it.
@caeies It would be great if you could look into it. I am fixing other issues.

Sorry I missed your comment (I was focused on memory leaks). Adding this ASAP.

Ok done on the PR #1302 I might create a new PR if you want to split some of them.

I still need to work on remaining leaks, but they looks weird, I don't see in the code how it can happen ...