[PATCH 0/3] Improve invalid auth and disconnect notify on l2tp and pptp

[PATCH 0/3] Improve invalid auth and disconnect notify on l2tp and pptp

[Jussi Laakkonen]

Both l2tp and pptp have the symptom that when authentication is invalid
the binary keeps on looping disconnect notify. As a solution to this the
task can be stopped to ensure that binary also dies. And to further make
the process better adapt similar connect done -approach that is used in,
e.g., OpenVPN to ensure the callback (vpn-provider.c:connect_cb()) will
get called only once.

This change makes both plugins to respond with 0 ("ok") only after the
notify from the binary is received that connection is established. Both
of the plugins did reply with 0 right after run_connect() which was not
the proper way.

Also add support for using ConnMan specific control file with xl2tpd
using -C switch. 

Jussi Laakkonen (2):
  l2tp: Improve invalid auth and disconnect notify, fix cb use
  pptp: Improve invalid auth and disconnect notify, fix cb use

Matt Vogt (1):
  l2tp: Create control file for xl2tpd

 vpn/plugins/l2tp.c | 133 +++++++++++++++++++++++++++++++--------------
 vpn/plugins/pptp.c | 127 ++++++++++++++++++++++++++++---------------
 2 files changed, 175 insertions(+), 85 deletions(-)

-- 
2.20.1

[PATCH 1/3] l2tp: Create control file for xl2tpd

[Jussi Laakkonen]

From: Matt Vogt <matthew.vogt@jollamobile.com>

Create and use connman specific control file with xl2tpd.
---
 vpn/plugins/l2tp.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/vpn/plugins/l2tp.c b/vpn/plugins/l2tp.c
index 1e4fcd1f..5213b345 100644
--- a/vpn/plugins/l2tp.c
+++ b/vpn/plugins/l2tp.c
@@ -651,7 +651,7 @@ static int run_connect(struct vpn_provider *provider,
 			vpn_provider_connect_cb_t cb, void *user_data,
 			const char *username, const char *password)
 {
-	char *l2tp_name, *pppd_name;
+	char *l2tp_name, *ctrl_name, *pppd_name;
 	int l2tp_fd, pppd_fd;
 	int err;
 
@@ -674,12 +674,24 @@ static int run_connect(struct vpn_provider *provider,
 		goto done;
 	}
 
-	pppd_name = g_strdup_printf(VPN_STATEDIR "/connman-ppp-option.conf");
+	ctrl_name = g_strconcat(VPN_STATEDIR, "/connman-xl2tpd-control", NULL);
+
+	if (mkfifo(ctrl_name, S_IRUSR|S_IWUSR) != 0 && errno != EEXIST) {
+		connman_error("Error creating xl2tp control pipe");
+		g_free(l2tp_name);
+		g_free(ctrl_name);
+		close(l2tp_fd);
+		err = -EIO;
+		goto done;
+	}
+
+	pppd_name = g_strconcat(VPN_STATEDIR, "/connman-ppp-option.conf", NULL);
 
 	pppd_fd = open(pppd_name, O_RDWR|O_CREAT|O_TRUNC, S_IRUSR|S_IWUSR);
 	if (pppd_fd < 0) {
 		connman_error("Error writing pppd config");
 		g_free(l2tp_name);
+		g_free(ctrl_name);
 		g_free(pppd_name);
 		close(l2tp_fd);
 		err = -EIO;
@@ -691,9 +703,11 @@ static int run_connect(struct vpn_provider *provider,
 	write_pppd_option(provider, pppd_fd);
 
 	connman_task_add_argument(task, "-D", NULL);
+	connman_task_add_argument(task, "-C", ctrl_name);
 	connman_task_add_argument(task, "-c", l2tp_name);
 
 	g_free(l2tp_name);
+	g_free(ctrl_name);
 	g_free(pppd_name);
 	close(l2tp_fd);
 	close(pppd_fd);
-- 
2.20.1

[PATCH 2/3] l2tp: Improve invalid auth and disconnect notify, fix cb use

[Jussi Laakkonen]

Stop the task running xl2tpd when it notifies being disconnected as in
case the authentication fails the process keeps on looping with the
notify and cannot be given new credentials until stopped with a D-Bus
call to disconnect. Implement a connect done function to ensure that the
callback is called only once as it sends a reply back to the caller. Use
this function when stopping, having invalid credentials or other errors.

Also, call the callback via connect done only after the connection is
established. This fixes the issue of l2tp reporting back 0 ("ok") before
it gets an response from the connection negotiation, which informed the
caller with completely invalid information.
---
 vpn/plugins/l2tp.c | 115 +++++++++++++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 40 deletions(-)

diff --git a/vpn/plugins/l2tp.c b/vpn/plugins/l2tp.c
index 5213b345..ee40dd72 100644
--- a/vpn/plugins/l2tp.c
+++ b/vpn/plugins/l2tp.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2010,2013  BMW Car IT GmbH.
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -121,12 +122,40 @@ struct {
 static DBusConnection *connection;
 
 struct l2tp_private_data {
+	struct vpn_provider *provider;
 	struct connman_task *task;
 	char *if_name;
 	vpn_provider_connect_cb_t cb;
 	void *user_data;
 };
 
+static void l2tp_connect_done(struct l2tp_private_data *data, int err)
+{
+	vpn_provider_connect_cb_t cb;
+	void *user_data;
+
+	if (!data || !data->cb)
+		return;
+
+	/* Ensure that callback is called only once */
+	cb = data->cb;
+	user_data = data->user_data;
+	data->cb = NULL;
+	data->user_data = NULL;
+	cb(data->provider, user_data, err);
+}
+
+static void free_private_data(struct l2tp_private_data *data)
+{
+	if (vpn_provider_get_plugin_data(data->provider) == data)
+		vpn_provider_set_plugin_data(data->provider, NULL);
+
+	l2tp_connect_done(data, EIO);
+	vpn_provider_unref(data->provider);
+	g_free(data->if_name);
+	g_free(data);
+}
+
 static DBusMessage *l2tp_get_sec(struct connman_task *task,
 			DBusMessage *msg, void *user_data)
 {
@@ -164,6 +193,9 @@ static int l2tp_notify(DBusMessage *msg, struct vpn_provider *provider)
 	char *addressv4 = NULL, *netmask = NULL, *gateway = NULL;
 	char *ifname = NULL, *nameservers = NULL;
 	struct connman_ipaddress *ipaddress = NULL;
+	struct l2tp_private_data *data;
+
+	data = vpn_provider_get_plugin_data(provider);
 
 	dbus_message_iter_init(msg, &iter);
 
@@ -182,11 +214,22 @@ static int l2tp_notify(DBusMessage *msg, struct vpn_provider *provider)
 		vpn_provider_set_string_hide_value(provider, "L2TP.Password",
 					NULL);
 
+		l2tp_connect_done(data, EACCES);
 		return VPN_STATE_AUTH_FAILURE;
 	}
 
-	if (strcmp(reason, "connect"))
+	if (strcmp(reason, "connect")) {
+		l2tp_connect_done(data, EIO);
+
+		/*
+		 * Stop the task to avoid potential looping of this state when
+		 * authentication fails.
+		 */
+		if (data && data->task)
+			connman_task_stop(data->task);
+
 		return VPN_STATE_DISCONNECT;
+	}
 
 	dbus_message_iter_recurse(&iter, &dict);
 
@@ -257,6 +300,8 @@ static int l2tp_notify(DBusMessage *msg, struct vpn_provider *provider)
 	g_free(nameservers);
 	connman_ipaddress_free(ipaddress);
 
+	l2tp_connect_done(data, 0);
+
 	return VPN_STATE_CONNECT;
 }
 
@@ -476,9 +521,10 @@ static int l2tp_write_config(struct vpn_provider *provider,
 
 static void l2tp_died(struct connman_task *task, int exit_code, void *user_data)
 {
+	struct l2tp_private_data *data = user_data;
 	char *conf_file;
 
-	vpn_died(task, exit_code, user_data);
+	vpn_died(task, exit_code, data->provider);
 
 	conf_file = g_strdup_printf(VPN_STATEDIR "/connman-xl2tpd.conf");
 	unlink(conf_file);
@@ -487,6 +533,8 @@ static void l2tp_died(struct connman_task *task, int exit_code, void *user_data)
 	conf_file = g_strdup_printf(VPN_STATEDIR "/connman-ppp-option.conf");
 	unlink(conf_file);
 	g_free(conf_file);
+
+	free_private_data(data);
 }
 
 struct request_input_reply {
@@ -646,11 +694,11 @@ static int request_input(struct vpn_provider *provider,
 	return -EINPROGRESS;
 }
 
-static int run_connect(struct vpn_provider *provider,
-			struct connman_task *task, const char *if_name,
-			vpn_provider_connect_cb_t cb, void *user_data,
+static int run_connect(struct l2tp_private_data *data,
 			const char *username, const char *password)
 {
+	struct vpn_provider *provider = data->provider;
+	struct connman_task *task = data->task;
 	char *l2tp_name, *ctrl_name, *pppd_name;
 	int l2tp_fd, pppd_fd;
 	int err;
@@ -712,27 +760,19 @@ static int run_connect(struct vpn_provider *provider,
 	close(l2tp_fd);
 	close(pppd_fd);
 
-	err = connman_task_run(task, l2tp_died, provider,
-				NULL, NULL, NULL);
+	err = connman_task_run(task, l2tp_died, data, NULL, NULL, NULL);
 	if (err < 0) {
 		connman_error("l2tp failed to start");
 		err = -EIO;
-		goto done;
 	}
 
 done:
-	if (cb)
-		cb(provider, user_data, err);
+	if (err)
+		l2tp_connect_done(data, -err);
 
 	return err;
 }
 
-static void free_private_data(struct l2tp_private_data *data)
-{
-	g_free(data->if_name);
-	g_free(data);
-}
-
 static void request_input_cb(struct vpn_provider *provider,
 			const char *username,
 			const char *password,
@@ -750,10 +790,7 @@ static void request_input_cb(struct vpn_provider *provider,
 	vpn_provider_set_string_hide_value(provider, "L2TP.Password",
 								password);
 
-	run_connect(provider, data->task, data->if_name, data->cb,
-		data->user_data, username, password);
-
-	free_private_data(data);
+	run_connect(data, username, password);
 }
 
 static int l2tp_connect(struct vpn_provider *provider,
@@ -761,9 +798,21 @@ static int l2tp_connect(struct vpn_provider *provider,
 			vpn_provider_connect_cb_t cb, const char *dbus_sender,
 			void *user_data)
 {
+	struct l2tp_private_data *data;
 	const char *username, *password;
 	int err;
 
+	data = g_try_new0(struct l2tp_private_data, 1);
+	if (!data)
+		return -ENOMEM;
+
+	data->provider = vpn_provider_ref(provider);
+	data->task = task;
+	data->if_name = g_strdup(if_name);
+	data->cb = cb;
+	data->user_data = user_data;
+	vpn_provider_set_plugin_data(provider, data);
+
 	if (connman_task_set_notify(task, "getsec",
 					l2tp_get_sec, provider) != 0) {
 		err = -ENOMEM;
@@ -776,33 +825,19 @@ static int l2tp_connect(struct vpn_provider *provider,
 	DBG("user %s password %p", username, password);
 
 	if (!username || !*username || !password || !*password) {
-		struct l2tp_private_data *data;
-
-		data = g_try_new0(struct l2tp_private_data, 1);
-		if (!data)
-			return -ENOMEM;
-
-		data->task = task;
-		data->if_name = g_strdup(if_name);
-		data->cb = cb;
-		data->user_data = user_data;
-
 		err = request_input(provider, request_input_cb, dbus_sender,
 									data);
-		if (err != -EINPROGRESS) {
-			free_private_data(data);
-			goto done;
-		}
+		if (err != -EINPROGRESS)
+			goto error;
+
 		return err;
 	}
 
-done:
-	return run_connect(provider, task, if_name, cb, user_data,
-							username, password);
+	return run_connect(data, username, password);
 
 error:
-	if (cb)
-		cb(provider, user_data, err);
+	l2tp_connect_done(data, -err);
+	free_private_data(data);
 
 	return err;
 }
-- 
2.20.1

[PATCH 3/3] pptp: Improve invalid auth and disconnect notify, fix cb use

[Jussi Laakkonen]

Stop the task running pptp when it notifies being disconnected as in
case the authentication fails the process keeps on looping with the
notify and cannot be given new credentials until stopped with a D-Bus
call to disconnect. Implement a connect done function to ensure that the
callback is called only once as it sends a reply back to the caller. Use
this function when stopping, having invalid credentials or other errors.

Also, call the callback via connect done only after the connection is
established. This fixes the issue of pptp reporting back 0 ("ok") before
it gets an response from the connection negotiation, which informed the
caller with completely invalid information.
---
 vpn/plugins/pptp.c | 127 ++++++++++++++++++++++++++++++---------------
 1 file changed, 84 insertions(+), 43 deletions(-)

diff --git a/vpn/plugins/pptp.c b/vpn/plugins/pptp.c
index 4a704bb1..7274376f 100644
--- a/vpn/plugins/pptp.c
+++ b/vpn/plugins/pptp.c
@@ -4,6 +4,7 @@
  *
  *  Copyright (C) 2010,2013-2014  BMW Car IT GmbH.
  *  Copyright (C) 2012-2013  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2019-2021  Jolla Ltd.
  *
  *  This program is free software; you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License version 2 as
@@ -86,12 +87,40 @@ struct {
 static DBusConnection *connection;
 
 struct pptp_private_data {
+	struct vpn_provider *provider;
 	struct connman_task *task;
 	char *if_name;
 	vpn_provider_connect_cb_t cb;
 	void *user_data;
 };
 
+static void pptp_connect_done(struct pptp_private_data *data, int err)
+{
+	vpn_provider_connect_cb_t cb;
+	void *user_data;
+
+	if (!data || !data->cb)
+		return;
+
+	/* Ensure that callback is called only once */
+	cb = data->cb;
+	user_data = data->user_data;
+	data->cb = NULL;
+	data->user_data = NULL;
+	cb(data->provider, user_data, err);
+}
+
+static void free_private_data(struct pptp_private_data *data)
+{
+	if (vpn_provider_get_plugin_data(data->provider) == data)
+		vpn_provider_set_plugin_data(data->provider, NULL);
+
+	pptp_connect_done(data, EIO);
+	vpn_provider_unref(data->provider);
+	g_free(data->if_name);
+	g_free(data);
+}
+
 static DBusMessage *pptp_get_sec(struct connman_task *task,
 				DBusMessage *msg, void *user_data)
 {
@@ -125,6 +154,9 @@ static int pptp_notify(DBusMessage *msg, struct vpn_provider *provider)
 	char *addressv4 = NULL, *netmask = NULL, *gateway = NULL;
 	char *ifname = NULL, *nameservers = NULL;
 	struct connman_ipaddress *ipaddress = NULL;
+	struct pptp_private_data *data;
+
+	data = vpn_provider_get_plugin_data(provider);
 
 	dbus_message_iter_init(msg, &iter);
 
@@ -143,11 +175,22 @@ static int pptp_notify(DBusMessage *msg, struct vpn_provider *provider)
 		vpn_provider_set_string_hide_value(provider, "PPTP.Password",
 					NULL);
 
+		pptp_connect_done(data, EACCES);
 		return VPN_STATE_AUTH_FAILURE;
 	}
 
-	if (strcmp(reason, "connect"))
+	if (strcmp(reason, "connect")) {
+		pptp_connect_done(data, EIO);
+
+		/*
+		 * Stop the task to avoid potential looping of this state when
+		 * authentication fails.
+		 */
+		if (data && data->task)
+			connman_task_stop(data->task);
+
 		return VPN_STATE_DISCONNECT;
+	}
 
 	dbus_message_iter_recurse(&iter, &dict);
 
@@ -217,6 +260,7 @@ static int pptp_notify(DBusMessage *msg, struct vpn_provider *provider)
 	g_free(nameservers);
 	connman_ipaddress_free(ipaddress);
 
+	pptp_connect_done(data, 0);
 	return VPN_STATE_CONNECT;
 }
 
@@ -278,6 +322,16 @@ static void pptp_write_bool_option(struct connman_task *task,
 	}
 }
 
+static void pptp_died(struct connman_task *task, int exit_code,
+							void *user_data)
+{
+	struct pptp_private_data *data = user_data;
+
+	vpn_died(task, exit_code, data->provider);
+
+	free_private_data(data);
+}
+
 struct request_input_reply {
 	struct vpn_provider *provider;
 	vpn_provider_password_cb_t callback;
@@ -435,18 +489,18 @@ static int request_input(struct vpn_provider *provider,
 	return -EINPROGRESS;
 }
 
-static int run_connect(struct vpn_provider *provider,
-			struct connman_task *task, const char *if_name,
-			vpn_provider_connect_cb_t cb, void *user_data,
-			const char *username, const char *password)
+static int run_connect(struct pptp_private_data *data, const char *username,
+							const char *password)
 {
+	struct vpn_provider *provider = data->provider;
+	struct connman_task *task = data->task;
 	GString *pptp_opt_s;
 	const char *opt_s;
 	const char *host;
 	char *str;
 	int err, i;
 
-	if (!username || !password) {
+	if (!username || !*username || !password || !*password) {
 		DBG("Cannot connect username %s password %p",
 						username, password);
 		err = -EINVAL;
@@ -498,27 +552,19 @@ static int run_connect(struct vpn_provider *provider,
 	connman_task_add_argument(task, "plugin",
 				SCRIPTDIR "/libppp-plugin.so");
 
-	err = connman_task_run(task, vpn_died, provider,
-				NULL, NULL, NULL);
+	err = connman_task_run(task, pptp_died, data, NULL, NULL, NULL);
 	if (err < 0) {
 		connman_error("pptp failed to start");
 		err = -EIO;
-		goto done;
 	}
 
 done:
-	if (cb)
-		cb(provider, user_data, err);
+	if (err)
+		pptp_connect_done(data, -err);
 
 	return err;
 }
 
-static void free_private_data(struct pptp_private_data *data)
-{
-	g_free(data->if_name);
-	g_free(data);
-}
-
 static void request_input_cb(struct vpn_provider *provider,
 			const char *username,
 			const char *password,
@@ -526,7 +572,7 @@ static void request_input_cb(struct vpn_provider *provider,
 {
 	struct pptp_private_data *data = user_data;
 
-	if (!username || !password)
+	if (!username || !*username || !password || !*password)
 		DBG("Requesting username %s or password failed, error %s",
 			username, error);
 	else if (error)
@@ -536,10 +582,7 @@ static void request_input_cb(struct vpn_provider *provider,
 	vpn_provider_set_string_hide_value(provider, "PPTP.Password",
 								password);
 
-	run_connect(provider, data->task, data->if_name, data->cb,
-		data->user_data, username, password);
-
-	free_private_data(data);
+	run_connect(data, username, password);
 }
 
 static int pptp_connect(struct vpn_provider *provider,
@@ -547,9 +590,21 @@ static int pptp_connect(struct vpn_provider *provider,
 			vpn_provider_connect_cb_t cb, const char *dbus_sender,
 			void *user_data)
 {
+	struct pptp_private_data *data;
 	const char *username, *password;
 	int err;
 
+	data = g_try_new0(struct pptp_private_data, 1);
+	if (!data)
+		return -ENOMEM;
+
+	data->provider = vpn_provider_ref(provider);
+	data->task = task;
+	data->if_name = g_strdup(if_name);
+	data->cb = cb;
+	data->user_data = user_data;
+	vpn_provider_set_plugin_data(provider, data);
+
 	DBG("iface %s provider %p user %p", if_name, provider, user_data);
 
 	if (connman_task_set_notify(task, "getsec",
@@ -563,34 +618,20 @@ static int pptp_connect(struct vpn_provider *provider,
 
 	DBG("user %s password %p", username, password);
 
-	if (!username || !password) {
-		struct pptp_private_data *data;
-
-		data = g_try_new0(struct pptp_private_data, 1);
-		if (!data)
-			return -ENOMEM;
-
-		data->task = task;
-		data->if_name = g_strdup(if_name);
-		data->cb = cb;
-		data->user_data = user_data;
-
+	if (!username || !*username || !password || !*password) {
 		err = request_input(provider, request_input_cb, dbus_sender,
 									data);
-		if (err != -EINPROGRESS) {
-			free_private_data(data);
-			goto done;
-		}
+		if (err != -EINPROGRESS)
+			goto error;
+
 		return err;
 	}
 
-done:
-	return run_connect(provider, task, if_name, cb, user_data,
-							username, password);
+	return run_connect(data, username, password);
 
 error:
-	if (cb)
-		cb(provider, user_data, err);
+	pptp_connect_done(data, -err);
+	free_private_data(data);
 
 	return err;
 }
-- 
2.20.1

Re: [PATCH 0/3] Improve invalid auth and disconnect notify on l2tp and pptp

[Daniel Wagner]

Hi Jussi,

On Fri, Aug 13, 2021 at 04:42:06PM +0300, Jussi Laakkonen wrote:
> Both l2tp and pptp have the symptom that when authentication is invalid
> the binary keeps on looping disconnect notify. As a solution to this the
> task can be stopped to ensure that binary also dies. And to further make
> the process better adapt similar connect done -approach that is used in,
> e.g., OpenVPN to ensure the callback (vpn-provider.c:connect_cb()) will
> get called only once.
> 
> This change makes both plugins to respond with 0 ("ok") only after the
> notify from the binary is received that connection is established. Both
> of the plugins did reply with 0 right after run_connect() which was not
> the proper way.
> 
> Also add support for using ConnMan specific control file with xl2tpd
> using -C switch. 

Patches applied. Thanks!
Daniel