Send connman mailing list submissions to
[email protected]
To subscribe or unsubscribe via email, send a message with subject or
body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of connman digest..."
Today's Topics:
1. Re: [PATCH 1/5] vpn-util: Create utility file for VPN core and plugins
(Daniel Wagner)
2. Re: [PATCH v2] openvpn: Add support for --ping, --ping-exit and
--remap-usr1
(Daniel Wagner)
3. Re: [PATCH] pptp: Support --idle-wait and --max-echo-wait options
(Daniel Wagner)
4. [PATCH] bluetooth: Move DBG statement after initial checks
(Daniel Wagner)
5. Re: [PATCH] bluetooth: Move DBG statement after initial checks
(Daniel Wagner)
6. Re: [PATCH 1/5] vpn-util: Create utility file for VPN core and plugins
(Jussi Laakkonen)
7. [PATCH v2 1/5] vpn-util: Create utility file for VPN core and plugins
(Jussi Laakkonen)
----------------------------------------------------------------------
Date: Fri, 23 Oct 2020 09:29:26 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH 1/5] vpn-util: Create utility file for VPN core
and plugins
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Tue, Oct 20, 2020 at 05:29:34PM +0300, Jussi Laakkonen wrote:
> Create vpn-util.c for VPN core and plugins to use. This is a first step
> in establishing some sort of lib for them.
>
> Add functionality to get the user struct passwd and struct group using
> the username and groupname:
> - vpn_util_get_passwd()
> - vpn_util_get_group()
>
> Add function vpn_util_create_path() to create dir for a VPN plugin.
> The dirname is extracted with g_path_get_dirname() and it must have a
> prefix of "/var/run/connman-vpn/", "/var/run/user/" or "/tmp/" and must
> not contain ".." or "./". One directory element must be provided after
> the prefix, e.g. "/tmp/dir/".
Could elaborate why only this limiting is necessary? I assume you have
some analysis of attack vectors and that's the way to prevent the
attacks, right?
> If the basename path is an existing dir
> permissions and ownership is changed according to the request ones,
> otherwise the file is removed and then created, as if it never existed.
> g_unlink() is used to handle safe removals of symlink, which also enables
> detection of parent dir write permissions - error is returned unless the
> dir exists, in which case ownership and permissions are attempted to be
> set accordingly.
The code looks good. I just would like to have some small explanation
why the path filtering is needed. Maybe add this info as comment to the
code, as I am sure anyone stumbling over this code will ask the same
question :)
Thanks,
Daniel
------------------------------
Date: Fri, 23 Oct 2020 09:34:57 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH v2] openvpn: Add support for --ping, --ping-exit
and --remap-usr1
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Thu, Oct 22, 2020 at 02:45:25PM +0300, Jussi Laakkonen wrote:
> Add support for --ping (OpenVPN.Ping) and --ping-exit (OpenVPN.PingExit)
> configuration values. Set defaults of 10 for ping and 60 for ping exit
> from https://community.openvpn.net/openvpn/wiki/Openvpn24ManPage
>
> Set --ping-restart only with TCP since with UDP it is more feasible to
> use --ping and --ping-exit with the default values if the values are
> unset. If with TCP --ping-exit is set ignore --ping-restart as the values
> are mutually exclusive.
>
> Add --remap-usr1 option which remaps SIGUSR1 as SIGHUP/SIGTERM in order
> to restart the process when errors are detected. OpenVPN does handle
> some errors internally and it may not always be good with ConnMan
> monitoring it.
> ---
>
> Changes since v2:
> * Remove [openvpn] tag line from the commit message.
Patch applied.
Thanks,
Daniel
------------------------------
Date: Fri, 23 Oct 2020 09:40:29 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH] pptp: Support --idle-wait and --max-echo-wait
options
To: Jussi Laakkonen <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
Hi Jussi,
On Thu, Oct 22, 2020 at 02:38:27PM +0300, Jussi Laakkonen wrote:
> Implement support for --idle-wait and --max-echo-wait options. By
> default these are set to 60 if omitted.
>
> Added OPT_PPTP_ONLY to be able to separate these from the PPPD options.
> All PPTP options need to be added with "pty" as one option in order for
> them to work.
Patch applied.
Thanks,
Daniel
------------------------------
Date: Fri, 23 Oct 2020 09:47:12 +0200
From: Daniel Wagner <[email protected]>
Subject: [PATCH] bluetooth: Move DBG statement after initial checks
To: [email protected]
Cc: Daniel Wagner <[email protected]>
Message-ID: <[email protected]>
gcc complains with format-overflow for the DBG statement for the
bridge argument. Let's move the DBG after the initial checks to avoid
the complain and also make the output more helpful. If we see it in
the log, the bridge has been created.
---
plugins/bluetooth.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/plugins/bluetooth.c b/plugins/bluetooth.c
index f759a902611c..5336103451fc 100644
--- a/plugins/bluetooth.c
+++ b/plugins/bluetooth.c
@@ -717,8 +717,6 @@ static bool tethering_create(const char *path,
const char *method;
bool result;
- DBG("path %s bridge %s", path, bridge);
-
if (!bridge) {
g_free(tethering);
return false;
@@ -730,6 +728,8 @@ static bool tethering_create(const char *path,
return false;
}
+ DBG("path %s bridge %s", path, bridge);
+
tethering->technology = technology;
tethering->bridge = g_strdup(bridge);
tethering->enable = enabled;
--
2.28.0
------------------------------
Date: Fri, 23 Oct 2020 09:47:44 +0200
From: Daniel Wagner <[email protected]>
Subject: Re: [PATCH] bluetooth: Move DBG statement after initial
checks
To: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=us-ascii
On Fri, Oct 23, 2020 at 09:47:12AM +0200, Daniel Wagner wrote:
> gcc complains with format-overflow for the DBG statement for the
> bridge argument. Let's move the DBG after the initial checks to avoid
> the complain and also make the output more helpful. If we see it in
> the log, the bridge has been created.
Patch applied.
------------------------------
Date: Fri, 23 Oct 2020 17:07:28 +0300
From: Jussi Laakkonen <[email protected]>
Subject: Re: [PATCH 1/5] vpn-util: Create utility file for VPN core
and plugins
To: Daniel Wagner <[email protected]>
Cc: [email protected]
Message-ID: <[email protected]>
Content-Type: text/plain; charset=utf-8; format=flowed
Hi Daniel,
On 10/23/20 10:29 AM, Daniel Wagner wrote:
> Hi Jussi,
>
> On Tue, Oct 20, 2020 at 05:29:34PM +0300, Jussi Laakkonen wrote:
>> Create vpn-util.c for VPN core and plugins to use. This is a first step
>> in establishing some sort of lib for them.
>>
>> Add functionality to get the user struct passwd and struct group using
>> the username and groupname:
>> - vpn_util_get_passwd()
>> - vpn_util_get_group()
>>
>> Add function vpn_util_create_path() to create dir for a VPN plugin.
>> The dirname is extracted with g_path_get_dirname() and it must have a
>> prefix of "/var/run/connman-vpn/", "/var/run/user/" or "/tmp/" and must
>> not contain ".." or "./". One directory element must be provided after
>> the prefix, e.g. "/tmp/dir/".
> Could elaborate why only this limiting is necessary? I assume you have
> some analysis of attack vectors and that's the way to prevent the
> attacks, right?
I'd assume that VPN core/plugins would need to create some temporary dirs
for run-time use only. These are regarded as "safe" I'd say, and use of the
g_unlink() ensures that user created dir with symlink to some harmful
location
is not going to be removed, but only the symlink is, well, unlinked.
>> If the basename path is an existing dir
>> permissions and ownership is changed according to the request ones,
>> otherwise the file is removed and then created, as if it never existed.
>> g_unlink() is used to handle safe removals of symlink, which also enables
>> detection of parent dir write permissions - error is returned unless the
>> dir exists, in which case ownership and permissions are attempted to be
>> set accordingly.
> The code looks good. I just would like to have some small explanation
> why the path filtering is needed. Maybe add this info as comment to the
> code, as I am sure anyone stumbling over this code will ask the same
> question :)
Thanks, I'll add comment to the code and send v2 of this still today.
Cheers,
Jussi
------------------------------
Date: Fri, 23 Oct 2020 17:41:56 +0300
From: Jussi Laakkonen <[email protected]>
Subject: [PATCH v2 1/5] vpn-util: Create utility file for VPN core and
plugins
To: [email protected]
Message-ID: <[email protected]>
Create vpn-util.c for VPN core and plugins to use. This is a first step
in establishing some sort of lib for them.
Add functionality to get the user struct passwd and struct group using
the username and groupname:
- vpn_util_get_passwd()
- vpn_util_get_group()
Add function vpn_util_create_path() to create dir for a VPN plugin.
The dirname is extracted with g_path_get_dirname() and it must have a
prefix of "/var/run/connman-vpn/", "/var/run/user/" or "/tmp/" and must
not contain ".." or "./". One directory element must be provided after
the prefix, e.g. "/tmp/dir/". If the basename path is an existing dir
permissions and ownership is changed according to the request ones,
otherwise the file is removed and then created, as if it never existed.
g_unlink() is used to handle safe removals of symlink, which also enables
detection of parent dir write permissions - error is returned unless the
dir exists, in which case ownership and permissions are attempted to be
set accordingly.
In order to reduce the potential of using the vpn_util_create_path()
with a malicious purpose the prefixes limit access only to run-time and
temporary locations. VPN core and plugins do not need more access to
file system, but to create a temp/run-time dir, e.g., for a pid file.
---
Changes since v2:
* Add a comment to the allowed_prefixes as per request by Daniel.
* Add a paragraph to commit message about the prefix limitation.
Makefile.am | 2 +-
vpn/vpn-util.c | 223 +++++++++++++++++++++++++++++++++++++++++++++++++
vpn/vpn.h | 5 ++
3 files changed, 229 insertions(+), 1 deletion(-)
create mode 100644 vpn/vpn-util.c
diff --git a/Makefile.am b/Makefile.am
index 5971ca9b..8b4b4996 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -181,7 +181,7 @@ vpn_connman_vpnd_SOURCES = $(builtin_vpn_sources)
$(backtrace_sources) \
vpn/vpn-ipconfig.c src/inet.c vpn/vpn-rtnl.c \
src/dbus.c src/storage.c src/ipaddress.c src/agent.c \
vpn/vpn-agent.c vpn/vpn-agent.h src/inotify.c \
- vpn/vpn-config.c vpn/vpn-settings.c
+ vpn/vpn-config.c vpn/vpn-settings.c vpn/vpn-util.c
vpn_connman_vpnd_LDADD = gdbus/libgdbus-internal.la $(builtin_vpn_libadd) \
@GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \
diff --git a/vpn/vpn-util.c b/vpn/vpn-util.c
new file mode 100644
index 00000000..abd2cd34
--- /dev/null
+++ b/vpn/vpn-util.c
@@ -0,0 +1,223 @@
+/*
+ * ConnMan VPN daemon utils
+ *
+ * Copyright (C) 2020 Jolla Ltd. All rights reserved.
+ * Copyright (C) 2020 Open Mobile Platform LLC.
+ * Contact: [email protected]
+ *
+ * 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
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <errno.h>
+#include <unistd.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+#include <pwd.h>
+#include <grp.h>
+
+#include <glib/gstdio.h>
+
+#include <connman/log.h>
+
+#include "vpn.h"
+
+static bool is_string_digits(const char *str)
+{
+ int i;
+
+ if (!str || !*str)
+ return false;
+
+ for (i = 0; str[i]; i++) {
+ if (!g_ascii_isdigit(str[i]))
+ return false;
+ }
+
+ return true;
+}
+
+static uid_t get_str_id(const char *username)
+{
+ if (!username || !*username)
+ return 0;
+
+ return (uid_t)g_ascii_strtoull(username, NULL, 10);
+}
+
+struct passwd *vpn_util_get_passwd(const char *username)
+{
+ struct passwd *pwd;
+ uid_t uid;
+
+ if (!username || !*username)
+ return NULL;
+
+ if (is_string_digits(username)) {
+ uid = get_str_id(username);
+ pwd = getpwuid(uid);
+ } else {
+ pwd = getpwnam(username);
+ }
+
+ return pwd;
+}
+
+struct group *vpn_util_get_group(const char *groupname)
+{
+ struct group *grp;
+ gid_t gid;
+
+ if (!groupname || !*groupname)
+ return NULL;
+
+ if (is_string_digits(groupname)) {
+ gid = get_str_id(groupname);
+ grp = getgrgid(gid);
+ } else {
+ grp = getgrnam(groupname);
+ }
+
+ return grp;
+}
+
+/*
+ * These prefixes are used for checking if the requested path for
+ * vpn_util_create_path() is acceptable. Allow only prefixes meant for run-time
+ * or temporary use to limit the access to any system resources.
+ *
+ * VPN core and plugins would need to create only temporary dirs for the
+ * run-time use. The requested dirs can be created for a specific user when
+ * running a VPN plugin as a different user and thus, user specific run dir is
+ * allowed and limitation to access any other system dir is restricted.
+ */
+static const char *allowed_prefixes[] = { "/var/run/connman-vpn/",
+ "/var/run/user/", "/tmp/", NULL };
+
+static int is_path_allowed(const char *path)
+{
+ int err = -EPERM;
+ int i;
+
+ if (!path || !*path || !g_path_is_absolute(path))
+ return -EINVAL;
+
+ if (g_strrstr(path, "..") || g_strrstr(path, "./"))
+ return -EPERM;
+
+ for (i = 0; allowed_prefixes[i]; i++) {
+ if (g_str_has_prefix(path, allowed_prefixes[i])) {
+ const char *suffix = path +
+ strlen(allowed_prefixes[i]);
+
+ /*
+ * Don't allow plain prefixes, an additional dir must
+ * be included after the prexix in the requested path.
+ */
+ if (suffix && *suffix != G_DIR_SEPARATOR &&
+ g_strrstr(suffix,
+ G_DIR_SEPARATOR_S)) {
+ DBG("allowed %s, has suffix %s", path, suffix);
+ err = 0;
+ }
+
+ break;
+ }
+ }
+
+ return err;
+}
+
+int vpn_util_create_path(const char *path, uid_t uid, gid_t grp, int mode)
+{
+ mode_t old_umask;
+ char *dir_p;
+ int err;
+
+ err = is_path_allowed(path);
+ if (err)
+ return err;
+
+ dir_p = g_path_get_dirname(path);
+ if (!dir_p)
+ return -ENOMEM;
+
+ err = g_unlink(dir_p);
+ if (err)
+ err = -errno;
+
+ switch (err) {
+ case 0:
+ /* Removed */
+ case -ENOENT:
+ /* Did not exist */
+ break;
+ case -EACCES:
+ /*
+ * Cannot get write access to the containing directory, check
+ * if the path exists.
+ */
+ if (!g_file_test(dir_p, G_FILE_TEST_EXISTS))
+ goto out;
+
+ /* If the dir does not exist new one cannot be created */
+ if (!g_file_test(dir_p, G_FILE_TEST_IS_DIR))
+ goto out;
+
+ /* Fall through to chmod as the dir exists */
+ case -EISDIR:
+ /* Exists as dir, just chmod and change owner */
+ err = g_chmod(dir_p, mode);
+ if (err) {
+ connman_warn("chmod %s failed, err %d", dir_p, err);
+ err = -errno;
+ }
+
+ goto chown;
+ default:
+ /* Any other error that is not handled here */
+ connman_warn("remove %s failed, err %d", dir_p, err);
+ goto out;
+ }
+
+ /* Set dir creation mask to correspond to the mode */
+ old_umask = umask(~mode & 0777);
+
+ DBG("mkdir %s", dir_p);
+ err = g_mkdir_with_parents(dir_p, mode);
+
+ umask(old_umask);
+
+ if (err) {
+ connman_warn("mkdir %s failed, err %d", dir_p, err);
+ err = -errno;
+ goto out;
+ }
+
+chown:
+ if (uid && grp) {
+ err = chown(dir_p, uid, grp);
+ if (err) {
+ err = -errno;
+ connman_warn("chown %s failed for %d/%d, err %d",
+ dir_p, uid, grp, err);
+ }
+ }
+
+out:
+ g_free(dir_p);
+
+ return err;
+}
+
diff --git a/vpn/vpn.h b/vpn/vpn.h
index 3356d53e..8d7c63d6 100644
--- a/vpn/vpn.h
+++ b/vpn/vpn.h
@@ -131,3 +131,8 @@ const char * vpn_settings_get_binary_user(struct
vpn_plugin_data *data);
const char * vpn_settings_get_binary_group(struct vpn_plugin_data *data);
char ** vpn_settings_get_binary_supplementary_groups(
struct vpn_plugin_data *data);
+bool vpn_settings_is_system_user(const char *user);
+
+struct passwd *vpn_util_get_passwd(const char *username);
+struct group *vpn_util_get_group(const char *groupname);
+int vpn_util_create_path(const char *path, uid_t uid, gid_t grp, int mode);
--
2.20.1
------------------------------
Subject: Digest Footer
_______________________________________________
connman mailing list -- [email protected]
To unsubscribe send an email to [email protected]
------------------------------
End of connman Digest, Vol 60, Issue 23
***************************************
