nm-l2tp / NetworkManager-l2tp

L2TP and L2TP/IPsec support for NetworkManager
GNU General Public License v2.0
488 stars 84 forks source link

xfrm policy doesn't match xl2tpd traffic due to hardcoded source port #42

Closed alienth closed 7 years ago

alienth commented 7 years ago

Running on ubuntu 16.04 with strongswan.

The ipsec configuration generated by nm-l2tp includes the following directives:

  leftprotoport=udp/l2tp
  rightprotoport=udp/l2tp

(source)

The resulting ip xfrm policy is as follows:

src VPN_SERVER/32 dst CLIENT/32 proto udp sport 1701 dport 1701 
        dir in priority 2816 
        tmpl src 0.0.0.0 dst 0.0.0.0
                proto esp reqid 2 mode transport
src CLIENT/32 dst VPN_SERVER/32 proto udp sport 1701 dport 1701 
        dir out priority 2816 
        tmpl src 0.0.0.0 dst 0.0.0.0
                proto esp reqid 2 mode transport

As you can see, the policy is only matching traffic with both the source and destination port of 1701. When xl2tpd attempts to run, it utilizes whatever dynamic source port might be available. As such, the traffic does not match the defined xfrm policy, and will not be encrypted.

If a VPN server happens to allow non-IPSec L2TP traffic, this problem could result in L2TP being established without being secured.

alienth commented 7 years ago

Remove the leftprotoport appears to address this, as expected. Resulting xfrm policy:

src VPN_SERVER/32 dst CLIENT/32 proto udp sport 1701
        dir in priority 2816
        tmpl src 0.0.0.0 dst 0.0.0.0
                proto esp reqid 1 mode transport
src CLIENT/32 dst VPN_SERVER/32 proto udp dport 1701
        dir out priority 2816
        tmpl src 0.0.0.0 dst 0.0.0.0
                proto esp reqid 1 mode transport

A tcpdump check confirms that xl2tpd traffic is being properly IPsecd when using this config.

alienth commented 7 years ago

Curious.. I just saw ed00db61036addb21a77c06d2031b5922dc2efab which forces nm-l2tpd to fail if 1701 is unavailable for binding. Yet, the xl2tpd being fired in my case is binding to a random high-numbered port.

Any specific reason 1701 as a source port is a requirement? Would removing the leftprotoport definition be suitable to negate that requirement?

alienth commented 7 years ago

Ah, looks like the PPAd version I'm using doesn't include that code to fail if 1701 is unavailable.

Still curious if modifying the policy would be a better solution.

dkosovic commented 7 years ago

I agree, I was thinking about not setting leftprotoport=udp/l2tp as a less draconian approach some time after I did that commit and especially since I wanted to use `xltpd-control' as the proper fix, in the cases where there was a running xltpd, but ran into issues with some linux distribuitions not shipping the command or the version they shipped with had issues.

I think commit https://github.com/nm-l2tp/network-manager-l2tp/commit/8c35a9a8940259646928fe6657616b4f89788e19 was initially done because some VPN servers will reject the proposal if both source and detention ports aren't 1701. But I imagine it'll only be a minority that would reject the proposal if leftprotoport=udp/l2tp isn't set.

I'll probably do a patch for the exisiting PPA in the next few days.

Still have a number of issues to fix before I release a new version number, but should be in the next couple of weeks.

dkosovic commented 7 years ago

The updated PPA packages now have a patch that does the following :

    if (is_port_free (1701)) {
        write_config_option (ipsec_fd, "  leftprotoport=udp/l2tp\n");
    }
    write_config_option (ipsec_fd, "  rightprotoport=udp/l2tp\n");

The source code will have a similar fix soon.