pptp kernel-mode plugin

[anphsw]

This proposal suggests integrating the accel-pptp code (originally from https://accel-pptp.sourceforge.net/), which has been successfully used in various SOHO routers for many years, into the main pppd branch.
The module supports both client and server modes and uses the pptp module that has been included in the kernel since version 2.6.36.
This integration would eliminate the need for out-of-tree builds and ensure better long-term maintenance and compatibility.

[neheb]

https://github.com/openwrt/openwrt/commits/main/package/network/services/ppp/patches/500-add-pptp-plugin.patch

it appears this has been in OpenWrt since 2012.

[Neustradamus]

@anphsw: A long time ago, we have seen to add patches in upstream :)

• @xosevp 2020-07-11: new ppp release ? #111 (comment)
• me 2020-11-09: new ppp release ? #111 (comment)
• me 2020-11-09: new ppp release ? #111 (comment)
• @jow- 2021-01-04: new ppp release ? #111 (comment)
• me 2021-01-10: new ppp release ? #111 (comment)
• me 2021-01-16: new ppp release ? #111 (comment)
• me 2021-01-28: new ppp release ? #111 (comment)
• me 2021-02-28: new ppp release ? #111 (comment)
• @hauke 2021-02-28: new ppp release ? #111 (comment)
• ...

@LGA1150, @systemcrash, @rsalvaterra, @DragonBluep, @icegood, @PolynomialDivision, @Ansuel, @hauke, @rittere, @dedeckeh, @jow-, @ynezz, @sch-m, @nbd168: Your participation here is welcome :)

[xosevp]

https://github.com/openwrt/openwrt/commits/main/package/network/services/ppp/patches/500-add-pptp-plugin.patch

it appears this has been in OpenWrt since 2012.

The OpenWrt patch is much better than the original code from ACCEL-PPTP. It modernizes and cleans up the codebase by unifying logging with the native pppd API, refactoring internal inter-process communication, and improving parameter handling: ACCEL-PPTP.vs.openwrt.diff.txt

Furthermore, it has been more extensively tested as it is included in prplOS, a standard distribution for residential routers: https://gitlab.com/prpl-foundation/prplos/prplos/-/tree/latest-24.10/package/network/services/ppp/patches

[anphsw]

@xosevp
It seems that patch you mentioned:

• missing server mode fixes (socket leak and dead connections, already applied in this pull)
• changing callmgr* call symantics (why?)
• changes logging way for some situations (maybe it's good)
• adds support for changing pptp_window parameter (is it really used in production?)
• using direct access to ppp_devname instead ppp_set_pppdevnam() (seems like outdated way)

[Neustradamus]

@paulusmack, @jkroonza, @enaess, @jjkeijser: What do you think?

[jkroonza]

https://poptop.sourceforge.net/
http://pptpclient.sourceforge.net/

I've always just used these two. And I'm fairly certain it ends up using kernel mode ... but I'm not 100% sure.

If external dependencies for pptp (client) can be 100% eliminated that would be tremendous.

I'm assuming pptp server would still require some form of external daemon to listen on port 1723 to spawn pppd as appropriate?

That said - generally try to steer clear of pptp wherever possible now.

[anphsw]

@jkroonza

And I'm fairly certain it ends up using kernel mode ... but I'm not 100% sure

AFAIK it's purely userspace.

If external dependencies for pptp (client) can be 100% eliminated that would be tremendous

plugin pptp.so
pptp_server x.x.x.x

That's how it used in soho routers nowdays. can be also used with ppp 'peers' file etc.

I'm assuming pptp server would still require some form of external daemon to listen on port 1723 to spawn pppd as appropriate?

Yes, it can be controlled by poptop server side with slight modifications. Or theoretically you can launch it manually:

plugin pptp.so
pptp_client x.x.x.x
pptp_sock x

[jkroonza]

Okay, so one would still require some pptpd server. Is there an existing implementation that works out of the box or are there patches that needs to be upstreamed into poptop?

In principle I like this, I've not inspected code in detail.

I do find it odd that we've got dirutil.* here - I wasn't expecting that a pptp implementation here would need that?

Definitely seeing client-side connect stuff on a quick scan, and it looks like passing pptp_sock as an int means another process for example can just accept pptp port 1723 connections and then fork to pppd to actually run the control code? That implies a server is potentially almost as simple as (obviously need some error and SIGCHLD handling, as well fixing some obviously glaring syntax issues in the below):

int main() {
    int fd = socket(...);
    int fd2;
    pid_t p;
    bind(fd, 0.0.0.0:1723);
    while (true) {
      fd2 = accept(fd);
      p = fork();
      if (p == 0) {
         close(fd);
         execve(... "pptp_sock", "fd2");
         exit(1);
      }
   }
}

Man page only seem to document pptp_server, but pptp_sock option. This should be fixed. There are also some options that seem to duplicate existing options (pptp_phone vs remotenumber).

There is another undocumented pptp_client option too.

[anphsw]

@jkroonza

Is there an existing implementation that works out of the box or are there patches that needs to be upstreamed into poptop?

Yes, there are original (but outdated) patched version of poptop from https://sourceforge.net/projects/accel-pptp/files/accel-pptp/ and (fixed to build with modern compilers) https://github.com/anphsw/accel-pptpd/

I do find it odd that we've got dirutil.* here - I wasn't expecting that a pptp implementation here would need that?

Original code using dirnamex and make_valid_path to create call manager sockets. Is there local ppp code which doing same thing?

That implies a server is potentially almost as simple as (obviously need some error and SIGCHLD handling, as well fixing some obviously glaring syntax issues in the below)

I think such code should work OK if we don't need various server-side stuff like dynamic ip allocation.

There are also some options that seem to duplicate existing options (pptp_phone vs remotenumber)

It's not duplicate, it's the number that we pass to our peer in "control message type 7" pptp packet (pptp_msg.h). Not sure if it was really used.

Man page only seem to document pptp_server, but pptp_sock option.

Do we need just mention them as pptp server usage and not intended to be used in configuration files?

[paulusmack]

I want to defer thinking about this for now. At the moment my focus is on potential security vulnerabilities, and this is a lot of additional code that I would need to check carefully for vulnerabilities. Also, it seems like PPTP itself is not very secure, or at least that the Microsoft implementation of it (which I would expect to be what most people would use this with) has severe security deficiencies. So I want to put this aside until after I have dealt with the current crop of existing potential vulnerabilities.

[jkroonza]

@paulusmack the pptp deficiencies in PPTP relates (as far as I understand) primarily to authentication using MS-CHAP(v2) which generates MPPE keys, which to a degree can be circumvented by using alternative auth methods (srp and eap methods, where supported, I'm sure many clients won't support this), this does not improve the crypto side but for many scenarios where dealing with older cases there's no avoidance of pptp. Yes, RC4 (even with a 128-bit RNG) is far from ideal. Use PPP/L2TP/IPSec where possible. Not in a position to disclose details unfortunately. We are closing the loops on that, but suspect pptp will still be us unfortunately for a couple more years unfortunately.

I think you're right though, let's get a bug&security fix release out the door first, so let's leave this one aside for the moment.

[jkroonza]

@jkroonza

Is there an existing implementation that works out of the box or are there patches that needs to be upstreamed into poptop?

Yes, there are original (but outdated) patched version of poptop from https://sourceforge.net/projects/accel-pptp/files/accel-pptp/ and (fixed to build with modern compilers) https://github.com/anphsw/accel-pptpd/

I do find it odd that we've got dirutil.* here - I wasn't expecting that a pptp implementation here would need that?

Original code using dirnamex and make_valid_path to create call manager sockets. Is there local ppp code which doing same thing?

I'm struggling to understand the purpose of these sockets - what would they be used for? I can see we have a tcp socket for the control channel between the two sides, this is used to negotiate GRE (which likely requires another socket), and then PPP protocol uses that GRE socket for sending/receiving frames (which eventually gets handed off to the kernel). What other sockets are required?

That implies a server is potentially almost as simple as (obviously need some error and SIGCHLD handling, as well fixing some obviously glaring syntax issues in the below)

I think such code should work OK if we don't need various server-side stuff like dynamic ip allocation.

I haven't worked with a server deployment in a while that doesn't utilise radius for all of this, and one of the bigger challenges with some external managers was to get them to NOT perform IP allocation but to delegate. Managing that via radius et al honestly is easier, but I get what you're saying. In some cases having the manager process manage IP allocation could have advantages sure, but let's assume that we want "as simple as possible" a management daemon that does as little as humanly possible. The only option I'd actually want to pass honestly is an options file, and a remotenumber to give the remote IP of the incoming connection.

There are also some options that seem to duplicate existing options (pptp_phone vs remotenumber)

It's not duplicate, it's the number that we pass to our peer in "control message type 7" pptp packet (pptp_msg.h). Not sure if it was really used.

Okay.

Man page only seem to document pptp_server, but pptp_sock option.

Do we need just mention them as pptp server usage and not intended to be used in configuration files?

Yes, I think that both sets of options should be mentioned in the man page, perhaps split them into "Options for Servers", "Options for Clients" and "Common Options" sections?

[anphsw]

@jkroonza

What other sockets are required?
There were some utils in original accel-pptp/poptop package to manage active connections, and maybe custom utils. Maybe better to let them exist for compatibility reasons with "industry standard" code already running for a two decades.
I will review code of popular *wrt distributions to check if it was really used.

Added commits to remove dirutil dependency and shrinked some logging code too.

@paulusmack
This plugin interfacing with in-tree kernel module, so there are not much unique vulnerable code to run.
Also I will try to remove redundant and dead code from this module as much as possibe.

[paulusmack]

@paulusmack This plugin interfacing with in-tree kernel module, so there are not much unique vulnerable code to run.

So why does this PR need to add around 2500 lines of code (rough count)?

Also I will try to remove redundant and dead code from this module as much as possibe.

That would be great, thanks.

[jjkeijser]

I tend to agree with @paulusmack that this patch should be deferred until after the next release.
Having said that, I am actually inclined to NACK the whole concept: there are existing PPTP clients and servers out there (like poptop); why would you want to merge that into the pppd code base? what is the benefit? pppd runs fully in userspace, so do the PPTP client/servers. No benefit there. Is the pptp-client / poptop code no longer maintained? I'd say invest more time in that - resources on maintaining the pppd codebase seem stretchec as it is.

Perhaps I am old-school in the sense that (Linux) tools should do one thing and should do that well, instead of trying to assimilate bits and pieces from all over the place

[jkroonza]

Hi,

We've already agreed this should wait.

There are cases where encryption does not matter, and pptp is unavoidable (although, I highly prefer l2tp over pptp, but we are stuck with at least two cases where we can't do that).

The advantage of this over poptop is that the PPTP traffic ends up being handled in-kernel rather than by a user-space pty program. Keep in mind that pppd actually mostly just handles the control protocol packets, actual IP traffic is handled in-kernel with the pppd process for the most part doing nothing (it will periodically do LCP echo request and responses I believe).

This would definitely offload some of our work to the kernel and help to avoid quite a few context switches. So I'm pro the improvement, but pptp should be off from configure respect by default.

[jjkeijser]

How would that work? PPTP uses GRE for data transport, which is available as a kernel module. PPP has it's own corresponding kernel module. Handling everything in kernel space would mean that you move the data from the GRE module to the PPP module in kernel space . I don't see how the userspace pppd code can do that. What am I missing ?

[jkroonza]

userspace hands off to kernel space. The details of how that happens I'm not 100% familiar with myself. In essence pppd needs to inform the kernel to take control of the tty that would normally be used by read_packet() in sys-linux.c and to intercept the stuff that it understands and process it without handing that to userspace.

This is likely a multi-stage setup in the case of GRE. So I'm guessing there's a socket created for GRE send/receive, this then gets handed off to the kernel (which likely creates a new packet socket/tty) of sorts, which then reads decapsulated packets from the GRE tunnel, and writes decapsulated packets. In turn, there is no reason why pppd can't again request the kernel's ppp code to read/write directly to that specific socket/pipe, filtering what it understands and can process, and lets everything else pass through to userspace. Seems "simple enough" :).

[anphsw]

@jjkeijser

why would you want to merge that into the pppd code base? what is the benefit?

1. Speed (3-10x depending on mtu size and hardware)
2. Status quo (this code already used since 2010x by most embedded router projects as a patch for vanilla pppd)
3. pppd already have plugins for pppoe and l2tp with same structure.

pppd runs fully in userspace, so do the PPTP client/servers. No benefit there.

So isn't there absolutely useless pptp.ko module included in all recent kernels?

@jkroonza

This would definitely offload some of our work to the kernel and help to avoid quite a few context switches.

It's a lot of speedup. Decade ago soho routers just can't go over 10mbit's with userspace poptop due to weak cpu, so this module was invented and included to kernel. Now CPUs much faster, but userspace have same processing penalty in terms of Mhz/Mbit, so we can push wirespeed pptp now.
This applies to large pptp servers too (handling 10k connections is not a problem here). Yes, there are other projects like accel-ppp, but based on custom ppp implementations. Personally i'd like to use mainline pppd in my ISP installations (there are some places, better say, whole cities, which use PPTP for internet access).

[jkroonza]

Thanks, I do expect that we'll want to squash this, eventually @paulusmack

Also, I believe the aim right now is to perform a bugfix release without adding more features, so I'm happy to help push and drive this (by testing) once that release is out the door.

As stupid as this may sound, as much as I want this code merged, we should probably not encourage wider use of PPTP and this should be disabled by default. We're trying to eliminate it's use as far as possible.

Please also make sure that PPTP remains working even when PPTP support in kernel is disabled.

[anphsw]

@jkroonza

As stupid as this may sound, as much as I want this code merged, we should probably not encourage wider use of PPTP and this should be disabled by default.

It makes this PR totally useless. There is no difference between disabled out-of-tree code and not applied patch. Almost no distributions will turn it on at build time (thinking "if it's not enabled by default then something wrong with it" forcing user to use slow userspace pptp).

Example situation if "regular" user wants to use pptp:
enabled in-tree: use kernel plugin or userspace poptop
disabled in-tree and compiled by packager (unlikely): use kernel plugin or userspace poptop
disabled in-tree and not compiled by packager: use userspace poptop ONLY
out-of-tree: use userspace poptop ONLY

As you can see, from this point there is no option to forbid to use PPTP. You can only force user to run slower version.

Please also make sure that PPTP remains working even when PPTP support in kernel is disabled

There is no modifications to main ppp code, so it will always work as you expecting (just do not load pptp plugin).

[jkroonza]

I believe many distributions packages ppp into multiple packages already, say splitting off radius support into ppp-radius, but I get what you're saying. Please excuse my Gentoo bias where flipping things on and off is as simple as a USE flag. Happy to concede that, but please keep in mind Paulus has the final say - and he already indicated he's unsure of this.

I am wondering how easy it would be at this stage to package out-of-tree modules for PPP, I know significant effort has been made early 2.5 towards that purpose. Could even (possibly) host this under ppp-project then, but as an independent package entirely. @paulusmack - would you be open towards something like that?