Move VPN settings to vpn-settings.c. --- Makefile.am | 2 +- vpn/main.c | 68 ++---------------------------- vpn/vpn-settings.c | 100 +++++++++++++++++++++++++++++++++++++++++++++ vpn/vpn.h | 5 +++ 4 files changed, 110 insertions(+), 65 deletions(-) create mode 100644 vpn/vpn-settings.c diff --git a/Makefile.am b/Makefile.am index 745bef05..77f5d8a8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -182,7 +182,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-config.c vpn/vpn-settings.c vpn_connman_vpnd_LDADD = gdbus/libgdbus-internal.la $(builtin_vpn_libadd) \ @GLIB_LIBS@ @DBUS_LIBS@ @GNUTLS_LIBS@ \ diff --git a/vpn/main.c b/vpn/main.c index ee88aacd..133acd27 100644 --- a/vpn/main.c +++ b/vpn/main.c @@ -44,71 +44,10 @@ #define CONFIGMAINFILE CONFIGDIR "/connman-vpn.conf" -#define DEFAULT_INPUT_REQUEST_TIMEOUT 300 * 1000 - static GMainLoop *main_loop = NULL; static unsigned int __terminated = 0; -static struct { - unsigned int timeout_inputreq; -} connman_vpn_settings = { - .timeout_inputreq = DEFAULT_INPUT_REQUEST_TIMEOUT, -}; - -static GKeyFile *load_config(const char *file) -{ - GError *err = NULL; - GKeyFile *keyfile; - - keyfile = g_key_file_new(); - - g_key_file_set_list_separator(keyfile, ','); - - if (!g_key_file_load_from_file(keyfile, file, 0, &err)) { - if (err->code != G_FILE_ERROR_NOENT) { - connman_error("Parsing %s failed: %s", file, - err->message); - } - - g_error_free(err); - g_key_file_free(keyfile); - return NULL; - } - - return keyfile; -} - -static void parse_config(GKeyFile *config, const char *file) -{ - GError *error = NULL; - int timeout; - - if (!config) - return; - - DBG("parsing %s", file); - - timeout = g_key_file_get_integer(config, "General", - "InputRequestTimeout", &error); - if (!error && timeout >= 0) - connman_vpn_settings.timeout_inputreq = timeout * 1000; - - g_clear_error(&error); -} - -static int config_init(const char *file) -{ - GKeyFile *config; - - config = load_config(file); - parse_config(config, file); - if (config) - g_key_file_free(config); - - return 0; -} - static gboolean signal_handler(GIOChannel *channel, GIOCondition cond, gpointer user_data) { @@ -230,7 +169,7 @@ static GOptionEntry options[] = { */ unsigned int connman_timeout_input_request(void) { - return connman_vpn_settings.timeout_inputreq; + return __vpn_settings_get_timeout_inputreq(); } int main(int argc, char *argv[]) @@ -314,9 +253,9 @@ int main(int argc, char *argv[]) __connman_dbus_init(conn); if (!option_config) - config_init(CONFIGMAINFILE); + __vpn_settings_init(CONFIGMAINFILE); else - config_init(option_config); + __vpn_settings_init(option_config); __connman_inotify_init(); __connman_agent_init(); @@ -348,6 +287,7 @@ int main(int argc, char *argv[]) __connman_inotify_cleanup(); __connman_dbus_cleanup(); __connman_log_cleanup(false); + __vpn_settings_cleanup(); dbus_connection_unref(conn); diff --git a/vpn/vpn-settings.c b/vpn/vpn-settings.c new file mode 100644 index 00000000..40d2905a --- /dev/null +++ b/vpn/vpn-settings.c @@ -0,0 +1,100 @@ +/* + * ConnMan VPN daemon settings + * + * Copyright (C) 2012-2013 Intel Corporation. All rights reserved. + * Copyright (C) 2018-2019 Jolla Ltd. All rights reserved. + * Contact: jussi.laakkonen(a)jolla.com + * + * 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 <stdlib.h> +#include <string.h> + +#include <connman/log.h> + +#include "vpn.h" + +#define DEFAULT_INPUT_REQUEST_TIMEOUT 300 * 1000 + +static struct { + unsigned int timeout_inputreq; +} connman_vpn_settings = { + .timeout_inputreq = DEFAULT_INPUT_REQUEST_TIMEOUT, +}; + +unsigned int __vpn_settings_get_timeout_inputreq() +{ + return connman_vpn_settings.timeout_inputreq; +} + +static void parse_config(GKeyFile *config, const char *file) +{ + const char *group = "General"; + GError *error = NULL; + int timeout; + + if (!config) + return; + + DBG("parsing %s", file); + + timeout = g_key_file_get_integer(config, group, + "InputRequestTimeout", &error); + if (!error && timeout >= 0) + connman_vpn_settings.timeout_inputreq = timeout * 1000; + + g_clear_error(&error); +} + +GKeyFile *__vpn_settings_load_config(const char *file) +{ + GError *err = NULL; + GKeyFile *keyfile; + + keyfile = g_key_file_new(); + + g_key_file_set_list_separator(keyfile, ','); + + if (!g_key_file_load_from_file(keyfile, file, 0, &err)) { + if (err->code != G_FILE_ERROR_NOENT) { + connman_error("Parsing %s failed: %s", file, + err->message); + } + + g_error_free(err); + g_key_file_unref(keyfile); + return NULL; + } + + return keyfile; +} + +int __vpn_settings_init(const char *file) +{ + GKeyFile *config; + + config = __vpn_settings_load_config(file); + parse_config(config, file); + if (config) + g_key_file_unref(config); + + return 0; +} + +void __vpn_settings_cleanup() +{ + return; +} diff --git a/vpn/vpn.h b/vpn/vpn.h index 8bf86bd1..01e6aab1 100644 --- a/vpn/vpn.h +++ b/vpn/vpn.h @@ -116,3 +116,8 @@ char *__vpn_config_get_string(GKeyFile *key_file, const char *group_name, const char *key, GError **error); char **__vpn_config_get_string_list(GKeyFile *key_file, const char *group_name, const char *key, gsize *length, GError **error); + +int __vpn_settings_init(const char *file); +void __vpn_settings_cleanup(void); +GKeyFile *__vpn_settings_load_config(const char *file); +unsigned int __vpn_settings_get_timeout_inputreq(void); -- 2.20.1

This commit adds support for configurable user, group and supplementary groups for VPNs. Common configuration is loaded from the main VPN config file CONFIGDIR/connman-vpn.conf, which applies to all VPNs. This commit also adds support for VPN specific configuration in CONFIGDIR/vpn-plugin/ based on the plugin name + .conf suffix. Both configs follow the name format: - User = user to use for running VPN binary (username or uid) - Group = the main group to use for running VPN binary (groupname or gid) - SupplementaryGroups = supplementary groups used (separator: comma, groupnames or gids) An example config is: [VPNBinary] User = username Group = vpn SupplementaryGroups = inet,net_admin Common config for VPNs is loaded at initialization. The VPN specific configuration is loaded and stored into hash table when vpn_settings_parse_vpn_plugin_config() is called. If the config was already loaded, -EALREADY is returned, if the config was invalid or did not exist -ENOENT is returned. Configuration for a VPN is to be retrieved with vpn_settings_get_plugin_config(), which will return a pointer to struct vpn_plugin_data or NULL. Each configuration (user, group and supplementary groups) can be retrieved by passing the returned pointer or NULL to functions: - vpn_settings_get_binary_user() - vpn_settings_get_binary_group() - vpn_settings_get_binary_supplementary_groups() In case the struct pointer is NULL then the default value set in common config will be returned. Otherwise the value within the struct is returned. --- vpn/vpn-settings.c | 156 ++++++++++++++++++++++++++++++++++++++++++++- vpn/vpn.h | 11 ++++ 2 files changed, 166 insertions(+), 1 deletion(-) diff --git a/vpn/vpn-settings.c b/vpn/vpn-settings.c index 40d2905a..f37d1f61 100644 --- a/vpn/vpn-settings.c +++ b/vpn/vpn-settings.c @@ -22,24 +22,89 @@ #include <stdlib.h> #include <string.h> +#include <errno.h> #include <connman/log.h> #include "vpn.h" #define DEFAULT_INPUT_REQUEST_TIMEOUT 300 * 1000 +#define PLUGIN_CONFIGDIR CONFIGDIR "/vpn-plugin" +#define VPN_GROUP "VPNBinary" static struct { unsigned int timeout_inputreq; + char *binary_user; + char *binary_group; + char **binary_supplementary_groups; } connman_vpn_settings = { .timeout_inputreq = DEFAULT_INPUT_REQUEST_TIMEOUT, + .binary_user = NULL, + .binary_group = NULL, + .binary_supplementary_groups = NULL, }; +struct vpn_plugin_data { + char *binary_user; + char *binary_group; + char **binary_supplementary_groups; +}; + +GHashTable *plugin_hash = NULL; + +const char *vpn_settings_get_binary_user(struct vpn_plugin_data *data) +{ + if (data && data->binary_user) + return data->binary_user; + + return connman_vpn_settings.binary_user; +} + +const char *vpn_settings_get_binary_group(struct vpn_plugin_data *data) +{ + if (data && data->binary_group) + return data->binary_group; + + return connman_vpn_settings.binary_group; +} + +char **vpn_settings_get_binary_supplementary_groups + (struct vpn_plugin_data *data) +{ + if (data && data->binary_supplementary_groups) + return data->binary_supplementary_groups; + + return connman_vpn_settings.binary_supplementary_groups; +} + unsigned int __vpn_settings_get_timeout_inputreq() { return connman_vpn_settings.timeout_inputreq; } +static char *get_string(GKeyFile *config, const char *group, const char *key) +{ + char *str = g_key_file_get_string(config, group, key, NULL); + return str ? g_strstrip(str) : NULL; +} + +static char **get_string_list(GKeyFile *config, const char *group, + const char *key) +{ + gsize len = 0; + char **str = g_key_file_get_string_list(config, group, key, &len, NULL); + + if (str) { + guint i = 0; + + for (i = 0; i < len ; i++) { + str[i] = g_strstrip(str[i]); + } + } + + return str; +} + static void parse_config(GKeyFile *config, const char *file) { const char *group = "General"; @@ -57,6 +122,88 @@ static void parse_config(GKeyFile *config, const char *file) connman_vpn_settings.timeout_inputreq = timeout * 1000; g_clear_error(&error); + + connman_vpn_settings.binary_user = get_string(config, VPN_GROUP, + "User"); + connman_vpn_settings.binary_group = get_string(config, VPN_GROUP, + "Group"); + connman_vpn_settings.binary_supplementary_groups = get_string_list( + config, VPN_GROUP, + "SupplementaryGroups"); +} + +struct vpn_plugin_data *vpn_settings_get_vpn_plugin_config(const char *name) +{ + struct vpn_plugin_data *data = NULL; + + if (plugin_hash) + data = g_hash_table_lookup(plugin_hash, name); + + return data; +} + +static void vpn_plugin_data_free(gpointer data) +{ + struct vpn_plugin_data *plugin_data = (struct vpn_plugin_data*)data; + + g_free(plugin_data->binary_user); + g_free(plugin_data->binary_group); + g_strfreev(plugin_data->binary_supplementary_groups); + + g_free(data); +} + +int vpn_settings_parse_vpn_plugin_config(const char *name) +{ + struct vpn_plugin_data *data; + gchar *file; + gchar *ext = ".conf"; + GKeyFile *config; + gint err = 0; + + if (!name || !*name) + return -EINVAL; + + if (vpn_settings_get_vpn_plugin_config(name)) + return -EALREADY; + + file = g_strconcat(PLUGIN_CONFIGDIR, "/", name, ext, NULL); + + config = __vpn_settings_load_config(file); + + if (!config) { + err = -ENOENT; + DBG("Cannot load config %s for %s", file, name); + goto out; + } + + data = g_try_new0(struct vpn_plugin_data, 1); + + data->binary_user = get_string(config, VPN_GROUP, "User"); + data->binary_group = get_string(config, VPN_GROUP, "Group"); + data->binary_supplementary_groups = get_string_list(config, VPN_GROUP, + "SupplementaryGroups"); + + DBG("Loaded settings for %s: %s - %s", + name, data->binary_user, data->binary_group); + + if (!plugin_hash) + plugin_hash = g_hash_table_new_full(g_str_hash, g_str_equal, + g_free, vpn_plugin_data_free); + + g_hash_table_replace(plugin_hash, g_strdup(name), data); + + g_key_file_unref(config); + +out: + g_free(file); + return err; +} + +void vpn_settings_delete_vpn_plugin_config(const char *name) +{ + if (plugin_hash && name) + g_hash_table_remove(plugin_hash, name); } GKeyFile *__vpn_settings_load_config(const char *file) @@ -96,5 +243,12 @@ int __vpn_settings_init(const char *file) void __vpn_settings_cleanup() { - return; + g_free(connman_vpn_settings.binary_user); + g_free(connman_vpn_settings.binary_group); + g_strfreev(connman_vpn_settings.binary_supplementary_groups); + + if (plugin_hash) { + g_hash_table_destroy(plugin_hash); + plugin_hash = NULL; + } } diff --git a/vpn/vpn.h b/vpn/vpn.h index 01e6aab1..126f5e10 100644 --- a/vpn/vpn.h +++ b/vpn/vpn.h @@ -121,3 +121,14 @@ int __vpn_settings_init(const char *file); void __vpn_settings_cleanup(void); GKeyFile *__vpn_settings_load_config(const char *file); unsigned int __vpn_settings_get_timeout_inputreq(void); + +struct vpn_plugin_data; + +int vpn_settings_parse_vpn_plugin_config(const char* plugin_name); +void vpn_settings_delete_vpn_plugin_config(const char *name); +struct vpn_plugin_data* vpn_settings_get_vpn_plugin_config(const char *name); + +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); -- 2.20.1

Hi Jussi, Nitpick of the day: just called it setup_func. That matches the exit_func. Thanks, Daniel

This commit adds support for running VPN plugin binaries with configurable privileges. Function vpn_task_setup() is registered as a custom setup callback function into task.c and is called when the task is run and task_setup() is called. vpn_task_setup() retrieves user, group and supplementary groups for the plugin at vpn_register() in order to do it once and frees the plugin data at vpn_unregister(). The plugin data is stored by vpn-settings.c and is retrieved when VPN plugin is to be connected. Function vpn_task_setup() sets the permissions when VPN plugin's task is started. Use of setgid(), setgroups() and setuid() needs CAP_SETGID and CAP_SETUID capabilities. NOTE/TODO: It was observed that this worked correctly for external plugins but same configuration was not successful with builtin plugins. --- vpn/connman-vpn.service.in | 2 +- vpn/plugins/vpn.c | 136 ++++++++++++++++++++++++++++++++++++- 2 files changed, 136 insertions(+), 2 deletions(-) diff --git a/vpn/connman-vpn.service.in b/vpn/connman-vpn.service.in index e98fb714..dd15bab4 100644 --- a/vpn/connman-vpn.service.in +++ b/vpn/connman-vpn.service.in @@ -6,7 +6,7 @@ Type=dbus BusName=net.connman.vpn ExecStart=@sbindir@/connman-vpnd -n StandardOutput=null -CapabilityBoundingSet=CAP_KILL CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW +CapabilityBoundingSet=CAP_KILL CAP_NET_ADMIN CAP_NET_BIND_SERVICE CAP_NET_RAW CAP_SETGID CAP_SETUID ProtectHome=read-only ProtectSystem=full diff --git a/vpn/plugins/vpn.c b/vpn/plugins/vpn.c index 9919928b..a051cc75 100644 --- a/vpn/plugins/vpn.c +++ b/vpn/plugins/vpn.c @@ -33,6 +33,9 @@ #include <sys/types.h> #include <linux/if_tun.h> #include <net/if.h> +#include <sys/types.h> +#include <pwd.h> +#include <grp.h> #include <dbus/dbus.h> @@ -47,6 +50,7 @@ #include "../vpn-provider.h" #include "vpn.h" +#include "../vpn.h" struct vpn_data { struct vpn_provider *provider; @@ -415,12 +419,134 @@ exist_err: return ret; } +static gboolean is_numeric(const char *str) +{ + gint i; + + if(!str || !(*str)) + return false; + + for(i = 0; str[i] ; i++) { + if(!g_ascii_isdigit(str[i])) + return false; + } + + return true; +} + +static gint get_gid(const char *group_name) +{ + gint gid = -1; + struct group *grp; + + if(!group_name || !(*group_name)) + return gid; + + if (is_numeric(group_name)) { + gid_t group_id = (gid_t)g_ascii_strtoull(group_name, NULL, 10); + grp = getgrgid(group_id); + } else { + grp = getgrnam(group_name); + } + + if (grp) + gid = grp->gr_gid; + + return gid; +} + +static gint get_uid(const char *user_name) +{ + gint uid = -1; + struct passwd *pw; + + if(!user_name || !(*user_name)) + return uid; + + if (is_numeric(user_name)) { + uid_t user_id = (uid_t)g_ascii_strtoull(user_name, NULL, 10); + pw = getpwuid(user_id); + } else { + pw = getpwnam(user_name); + } + + if (pw) + uid = pw->pw_uid; + + return uid; +} + +static gint get_supplementary_gids(gchar **groups, gid_t **gid_list) +{ + gint group_count = 0; + gid_t *list = NULL; + int i; + + if (groups) { + for(i = 0; groups[i]; i++) { + group_count++; + + list = (gid_t*)g_try_realloc(list, + sizeof(gid_t) * group_count); + + if(!list) { + DBG("cannot allocate supplementary group list"); + break; + } + + list[i] = get_gid(groups[i]); + } + } + + *gid_list = list; + + return group_count; +} + +static void vpn_task_setup(gpointer user_data) +{ + struct vpn_plugin_data *data; + gint uid; + gint gid; + gid_t *gid_list = NULL; + size_t gid_list_size; + const gchar *user; + const gchar *group; + gchar **suppl_groups; + + data = user_data; + user = vpn_settings_get_binary_user(data); + group = vpn_settings_get_binary_group(data); + suppl_groups = vpn_settings_get_binary_supplementary_groups(data); + + uid = get_uid(user); + gid = get_gid(group); + gid_list_size = get_supplementary_gids(suppl_groups, &gid_list); + + DBG("vpn_task_setup uid:%d gid:%d supplementary group list size:%lu", + uid, gid, gid_list_size); + + + /* Change group if proper group name was set, requires CAP_SETGID.*/ + if (gid > 0 && setgid(gid)) + connman_error("error setting gid %d %s", gid, strerror(errno)); + + /* Set the supplementary groups if list exists, requires CAP_SETGID. */ + if (gid_list_size && gid_list && setgroups(gid_list_size, gid_list)) + connman_error("error setting gid list %s", strerror(errno)); + + /* Change user for the task if set, requires CAP_SETUID */ + if (uid > 0 && setuid(uid)) + connman_error("error setting uid %d %s", uid, strerror(errno)); +} + static int vpn_connect(struct vpn_provider *provider, vpn_provider_connect_cb_t cb, const char *dbus_sender, void *user_data) { struct vpn_data *data = vpn_provider_get_data(provider); struct vpn_driver_data *vpn_driver_data; + struct vpn_plugin_data *vpn_plugin_data; const char *name; int ret = 0, tun_flags = IFF_TUN; enum vpn_state state = VPN_STATE_UNKNOWN; @@ -478,7 +604,11 @@ static int vpn_connect(struct vpn_provider *provider, goto exist_err; } - data->task = connman_task_create(vpn_driver_data->program, NULL); + vpn_plugin_data = vpn_settings_get_vpn_plugin_config( + vpn_driver_data->name); + + data->task = connman_task_create(vpn_driver_data->program, + vpn_task_setup, vpn_plugin_data); if (!data->task) { ret = -ENOMEM; @@ -650,6 +780,9 @@ int vpn_register(const char *name, struct vpn_driver *vpn_driver, data->name = name; data->program = program; + if (vpn_settings_parse_vpn_plugin_config(data->name) != 0) + DBG("No configuration provided for VPN plugin %s", data->name); + data->vpn_driver = vpn_driver; data->provider_driver.name = name; @@ -688,6 +821,7 @@ void vpn_unregister(const char *name) return; vpn_provider_driver_unregister(&data->provider_driver); + vpn_settings_delete_vpn_plugin_config(name); g_hash_table_remove(driver_hash, name); -- 2.20.1

Hi Jussi, On Mon, Apr 29, 2019 at 06:40:02PM +0300, Jussi Laakkonen wrote: Not that I am a good documentation writer, but maybe 'Access permission can be controlled by ...' would be better start into this paragraph. Apart of my nitpicking, the series looks good. One thing I haven't understood so far, is why is the VPN API so different to the ConnMan API that we need to handle the access control directly. I must miss something. I though with polkit or even D-Bus config files you can do this kind of things. Thanks, Daniel