diff mbox series

[v2,5/5] Avoid re-opening and reading configuration file

Message ID 20210129115155.13116-6-michael.adler@siemens.com
State Changes Requested
Headers show
Series Overhaul swupdate.cfg handling | expand

Commit Message

Michael Adler Jan. 29, 2021, 11:51 a.m. UTC
During a regular startup, the configuration file (swupdate.cfg) is at
least open(2)ed and read(2) four times.

This patch introduces a new API for reading the configuration file,
which passes a handle to an in-memory copy of the configuration file
around.

For security reasons, the in-memory copy is not shared with
subprocesses, because a less privileged subprocess (setuid(3p)) might
not be allowed to read the configuration file. Future work could
optimize this further and check if the subprocess runs under the same
privileges and if so, share the configuration data via an appropriate
mechanism (e.g. mmap(3p).

Signed-off-by: Michael Adler <michael.adler@siemens.com>
Signed-off-by: Christian Storm <christian.storm@siemens.com>
---
 core/artifacts_versions.c     |  6 ++---
 core/pctl.c                   | 20 +++++++-------
 core/swupdate.c               | 44 +++++++++++++++++++++++--------
 corelib/downloader.c          |  7 ++++-
 corelib/swupdate_settings.c   | 49 ++++++++++++++++++++++-------------
 include/parselib.h            |  2 --
 include/pctl.h                |  9 +++++--
 include/swupdate_settings.h   | 40 ++++++++++++++++++++++------
 include/util.h                |  3 ++-
 mongoose/mongoose_interface.c |  7 ++++-
 suricatta/server_general.c    | 10 +++----
 suricatta/server_hawkbit.c    | 14 +++++-----
 suricatta/suricatta.c         | 11 +++++---
 13 files changed, 151 insertions(+), 71 deletions(-)

Comments

Stefano Babic Feb. 23, 2021, 7:31 p.m. UTC | #1
Hi Michael,

On 29.01.21 12:51, Michael Adler wrote:
> During a regular startup, the configuration file (swupdate.cfg) is at
> least open(2)ed and read(2) four times.
> 
> This patch introduces a new API for reading the configuration file,
> which passes a handle to an in-memory copy of the configuration file
> around.
> 
> For security reasons, the in-memory copy is not shared with
> subprocesses, because a less privileged subprocess (setuid(3p)) might
> not be allowed to read the configuration file. Future work could
> optimize this further and check if the subprocess runs under the same
> privileges and if so, share the configuration data via an appropriate
> mechanism (e.g. mmap(3p).
> 
> Signed-off-by: Michael Adler <michael.adler@siemens.com>
> Signed-off-by: Christian Storm <christian.storm@siemens.com>
> ---

The coverity tools discovers new issues with your patch. This is the report:

Please find the latest report on new defect(s) introduced to 
sbabic/swupdate found with Coverity Scan.

2 new defect(s) introduced to sbabic/swupdate found with Coverity Scan.


New defect(s) Reported-by: Coverity Scan
Showing 2 of 2 defect(s)


** CID 320783:  Memory - illegal accesses  (RETURN_LOCAL)
/core/swupdate.c: 843 in main()
diff mbox series

Patch

diff --git a/core/artifacts_versions.c b/core/artifacts_versions.c
index 9c1251c..90b5b32 100644
--- a/core/artifacts_versions.c
+++ b/core/artifacts_versions.c
@@ -129,10 +129,10 @@  static int versions_settings(void *setting, void *data)
 	return 0;
 }
 
-void get_sw_versions(char *cfgname, struct swupdate_cfg *sw)
+void get_sw_versions(swupdate_cfg_handle *handle, struct swupdate_cfg *sw)
 {
 	/* Try to read versions from configuration file */
-	if (cfgname && read_module_settings(cfgname, "versions", versions_settings, sw) == 0) {
+	if (handle != NULL && read_module_settings(handle, "versions", versions_settings, sw) == 0) {
 		return;
 	}
 	/* If not found, fall back to a legacy file in the format "<image name> <version>" */
@@ -140,7 +140,7 @@  void get_sw_versions(char *cfgname, struct swupdate_cfg *sw)
 }
 #else
 
-void get_sw_versions(char __attribute__ ((__unused__)) *cfgname,
+void get_sw_versions(swupdate_cfg_handle  __attribute__ ((__unused__))*handle,
 			struct swupdate_cfg *sw)
 {
 	read_sw_version_file(sw);
diff --git a/core/pctl.c b/core/pctl.c
index 01ad540..481f077 100644
--- a/core/pctl.c
+++ b/core/pctl.c
@@ -165,19 +165,16 @@  static int spawn_process(struct swupdate_task *task,
 	}
 }
 
-static void start_swupdate_subprocess(sourcetype type,
-			const char *name, const char *cfgfile,
+static void start_swupdate_subprocess(sourcetype type, const char *name,
+			uid_t run_as_userid, gid_t run_as_groupid,
+			const char* cfgfile,
 			int argc, char **argv,
 			swupdate_process start,
 			const char *cmdline)
 {
-	uid_t uid;
-	gid_t gid;
-
-	read_settings_user_id(cfgfile, name, &uid, &gid);
 	procs[nprocs].name = name;
 	procs[nprocs].type = type;
-	if (spawn_process(&procs[nprocs], uid, gid, cfgfile, argc, argv, start, cmdline) < 0) {
+	if (spawn_process(&procs[nprocs], run_as_userid, run_as_groupid, cfgfile, argc, argv, start, cmdline) < 0) {
 		ERROR("Spawning %s failed, exiting process...", name);
 		exit(1);
 	}
@@ -188,19 +185,22 @@  static void start_swupdate_subprocess(sourcetype type,
 
 
 void start_subprocess_from_file(sourcetype type, const char *name,
+			uid_t run_as_userid, gid_t run_as_groupid,
 			const char *cfgfile,
 			int argc, char **argv,
 			const char *cmdline)
 {
-	start_swupdate_subprocess(type, name, cfgfile, argc, argv, NULL, cmdline);
+	start_swupdate_subprocess(type, name, run_as_userid, run_as_groupid, cfgfile, argc, argv, NULL, cmdline);
 }
 
-void start_subprocess(sourcetype type, const char *name, const char *cfgfile,
+void start_subprocess(sourcetype type, const char *name,
+			uid_t run_as_userid, gid_t run_as_groupid,
+			const char *cfgfile,
 			int argc, char **argv,
 			swupdate_process start)
 {
 
-	start_swupdate_subprocess(type, name, cfgfile, argc, argv, start, NULL);
+	start_swupdate_subprocess(type, name, run_as_userid, run_as_groupid, cfgfile, argc, argv, start, NULL);
 }
 
 /*
diff --git a/core/swupdate.c b/core/swupdate.c
index f783c72..8e97567 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -548,13 +548,18 @@  int main(int argc, char **argv)
 		}
 	}
 
+	swupdate_cfg_handle *p_handle = NULL;
 	/* Load configuration file */
 	if (cfgfname != NULL) {
+		swupdate_cfg_handle handle;
+		int ret = swupdate_cfg_init(&handle, cfgfname);
+
 		/*
 		 * 'globals' section is mandatory if configuration file is specified.
 		 */
-		int ret = read_module_settings(cfgfname, "globals",
-					       read_globals_settings, &swcfg);
+		if (ret == 0) {
+			ret = read_module_settings(&handle, "globals", read_globals_settings, &swcfg);
+		}
 		if (ret != 0) {
 			/*
 			 * Exit on -ENODATA or -EINVAL errors.
@@ -563,6 +568,7 @@  int main(int argc, char **argv)
 			    "Error parsing configuration file: %s, exiting.\n",
 			    ret == -ENODATA ? "'globals' section missing"
 					    : "cannot read");
+			swupdate_cfg_destroy(&handle);
 			exit(EXIT_FAILURE);
 		}
 
@@ -572,11 +578,11 @@  int main(int argc, char **argv)
 		 * The following sections are optional, hence -ENODATA error code is
 		 * ignored if the section is not found. -EINVAL will not happen here.
 		 */
-		(void)read_module_settings(cfgfname, "logcolors",
-					   read_console_settings, &swcfg);
+		(void)read_module_settings(&handle, "logcolors", read_console_settings, &swcfg);
+		(void)read_module_settings(&handle, "processes", read_processes_settings, &swcfg);
 
-		(void)read_module_settings(cfgfname, "processes",
-					   read_processes_settings, &swcfg);
+		/* save the handle for later usage */
+		p_handle = &handle;
 	}
 
 	/*
@@ -828,7 +834,7 @@  int main(int argc, char **argv)
 	}
 
 	/* Read sw-versions */
-	get_sw_versions(cfgfname, &swcfg);
+	get_sw_versions(p_handle, &swcfg);
 
 	/*
 	 *  Start daemon if just a check is required
@@ -841,7 +847,10 @@  int main(int argc, char **argv)
 	/* Start embedded web server */
 #if defined(CONFIG_MONGOOSE)
 	if (opt_w) {
-		start_subprocess(SOURCE_WEBSERVER, "webserver",
+		uid_t uid;
+		gid_t gid;
+		read_settings_user_id(p_handle, "webserver", &uid, &gid);
+		start_subprocess(SOURCE_WEBSERVER, "webserver", uid, gid,
 				 cfgfname, ac, av,
 				 start_mongoose);
 		freeargs(av);
@@ -850,7 +859,10 @@  int main(int argc, char **argv)
 
 #if defined(CONFIG_SURICATTA)
 	if (opt_u) {
-		start_subprocess(SOURCE_SURICATTA, "suricatta",
+		uid_t uid;
+		gid_t gid;
+		read_settings_user_id(p_handle, "suricatta", &uid, &gid);
+		start_subprocess(SOURCE_SURICATTA, "suricatta", uid, gid,
 				 cfgfname, argcount,
 				 argvalues, start_suricatta);
 
@@ -860,7 +872,10 @@  int main(int argc, char **argv)
 
 #ifdef CONFIG_DOWNLOAD
 	if (opt_d) {
-		start_subprocess(SOURCE_DOWNLOADER, "download",
+		uid_t uid;
+		gid_t gid;
+		read_settings_user_id(p_handle, "download", &uid, &gid);
+		start_subprocess(SOURCE_DOWNLOADER, "download", uid, gid,
 				 cfgfname, dwlac,
 				 dwlav, start_download);
 		freeargs(dwlav);
@@ -877,7 +892,10 @@  int main(int argc, char **argv)
 
 		dwlav[dwlac] = NULL;
 
-		start_subprocess_from_file(SOURCE_UNKNOWN, proc->name,
+		uid_t uid;
+		gid_t gid;
+		read_settings_user_id(p_handle, proc->name, &uid, &gid);
+		start_subprocess_from_file(SOURCE_UNKNOWN, proc->name, uid, gid,
 					   cfgfname, dwlac,
 					   dwlav, dwlav[0]);
 
@@ -904,6 +922,10 @@  int main(int argc, char **argv)
 	sa.sa_handler = sigterm_handler;
 	sigaction(SIGTERM, &sa, NULL);
 
+	if (p_handle != NULL) {
+		swupdate_cfg_destroy(p_handle);
+	}
+
 	/*
 	 * Go into supervisor loop
 	 */
diff --git a/corelib/downloader.c b/corelib/downloader.c
index cda5cb1..600e4c8 100644
--- a/corelib/downloader.c
+++ b/corelib/downloader.c
@@ -108,7 +108,12 @@  static channel_data_t channel_options = {
 int start_download(const char *fname, int argc, char *argv[])
 {
 	if (fname) {
-		read_module_settings(fname, "download", download_settings, &channel_options);
+		swupdate_cfg_handle handle;
+		int ret = swupdate_cfg_init(&handle, fname);
+		if (ret == 0) {
+			read_module_settings(&handle, "download", download_settings, &channel_options);
+		}
+		swupdate_cfg_destroy(&handle);
 	}
 
 	/* reset to optind=1 to parse download's argument vector */
diff --git a/corelib/swupdate_settings.c b/corelib/swupdate_settings.c
index 1802436..53f0d89 100644
--- a/corelib/swupdate_settings.c
+++ b/corelib/swupdate_settings.c
@@ -10,6 +10,7 @@ 
  * starting swupdate with a long list of parameters.
  */
 
+#include <libconfig.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -53,6 +54,9 @@  static int read_settings_file(config_t *cfg, const char *filename)
 {
 	int ret;
 
+ 	if (!filename)
+		return -EINVAL;
+
 	DEBUG("Reading config file %s", filename);
 	ret = config_read_file(cfg, filename);
 	if (ret != CONFIG_TRUE) {
@@ -67,36 +71,23 @@  static int read_settings_file(config_t *cfg, const char *filename)
 	return ret;
 }
 
-int read_module_settings(const char *filename, const char *module, settings_callback fcn, void *data)
+int read_module_settings(swupdate_cfg_handle *handle, const char *module, settings_callback fcn, void *data)
 {
-	config_t cfg;
 	config_setting_t *elem;
 
-	if (!fcn || !filename)
-		return -EINVAL;
-
-	memset(&cfg, 0, sizeof(cfg));
-	config_init(&cfg);
-
-	if (read_settings_file(&cfg, filename) != CONFIG_TRUE) {
-		config_destroy(&cfg);
-		ERROR("Error reading configuration file, skipping....");
+	if (handle == NULL || !fcn)
 		return -EINVAL;
-	}
 
-	elem = find_settings_node(&cfg, module);
+	elem = find_settings_node(&handle->cfg, module);
 
 	if (!elem) {
 		DEBUG("No config settings found for module %s", module);
-		config_destroy(&cfg);
 		return -ENODATA;
 	}
 
 	DEBUG("Reading config settings for module %s", module);
 	fcn(elem, data);
 
-	config_destroy(&cfg);
-
 	return 0;
 }
 
@@ -110,7 +101,7 @@  static int get_run_as(void *elem, void *data)
 	return 0;
 }
 
-int read_settings_user_id(const char *filename, const char *module, uid_t *userid, gid_t *groupid)
+int read_settings_user_id(swupdate_cfg_handle *handle, const char *module, uid_t *userid, gid_t *groupid)
 {
 	struct run_as ids;
 	int ret;
@@ -118,7 +109,7 @@  int read_settings_user_id(const char *filename, const char *module, uid_t *useri
 	*userid = ids.userid = getuid();
 	*groupid = ids.groupid = getgid();
 
-	ret = read_module_settings(filename, module, get_run_as, &ids);
+	ret = read_module_settings(handle, module, get_run_as, &ids);
 	if (ret)
 		return -EINVAL;
 
@@ -162,3 +153,25 @@  int settings_into_dict(void *settings, void *data)
 
 	return 0;
 }
+
+/*
+ * Initialize handle with the settings found in filename.
+ * This allocates memory which needs to be released in swupdate_cfg_destroy().
+ */
+int swupdate_cfg_init(swupdate_cfg_handle *handle, const char *filename)
+{
+	config_init(&handle->cfg);
+	if (read_settings_file(&handle->cfg, filename) != CONFIG_TRUE) {
+		ERROR("Error reading configuration file %s", filename);
+		return -EINVAL;
+	}
+	return 0;
+}
+
+/*
+ * This releases (internally) allocated memory by handle.
+ */
+void swupdate_cfg_destroy(swupdate_cfg_handle *handle)
+{
+	config_destroy(&handle->cfg);
+}
diff --git a/include/parselib.h b/include/parselib.h
index 5cbc255..84a51e2 100644
--- a/include/parselib.h
+++ b/include/parselib.h
@@ -85,8 +85,6 @@  void *get_node_json(json_object *root, const char **nodes);
 #define get_node_json(root, nodes)	(NULL)
 #endif
 
-typedef int (*settings_callback)(void *elem, void *data);
-
 const char *get_field_string(parsertype p, void *e, const char *path);
 void get_field_string_with_size(parsertype p, void *e, const char *path,
 				char *d, size_t n);
diff --git a/include/pctl.h b/include/pctl.h
index de5f3b5..84cf549 100644
--- a/include/pctl.h
+++ b/include/pctl.h
@@ -9,6 +9,7 @@ 
 #define _SWUPDATE_PCTL_H
 
 #include <swupdate_status.h>
+#include <sys/types.h>
 
 extern int pid;
 extern int sw_sockfd;
@@ -28,11 +29,15 @@  pthread_t start_thread(void *(* start_routine) (void *), void *arg);
 
 typedef int (*swupdate_process)(const char *cfgname, int argc, char **argv);
 
-void start_subprocess(sourcetype type, const char *name, const char *cfgfile,
+void start_subprocess(sourcetype type, const char *name,
+			uid_t run_as_userid, gid_t run_as_groupid,
+			const char *cfgfile,
 			int argc, char **argv,
 			swupdate_process start);
 
-void start_subprocess_from_file(sourcetype type, const char *name, const char *cfgfile,
+void start_subprocess_from_file(sourcetype type, const char *name,
+			uid_t run_as_userid, gid_t run_as_groupid,
+			const char *cfgfile,
 			int argc, char **argv,
 			const char *cmd);
 
diff --git a/include/swupdate_settings.h b/include/swupdate_settings.h
index c3b083d..3da7777 100644
--- a/include/swupdate_settings.h
+++ b/include/swupdate_settings.h
@@ -8,24 +8,48 @@ 
 #ifndef _SWUPDATE_SETTINGS_H
 #define _SWUPDATE_SETTINGS_H
 
+#include <unistd.h>
+
+typedef int (*settings_callback)(void *elem, void *data);
+
 #ifdef CONFIG_LIBCONFIG
-int read_module_settings(const char *filename, const char *module, settings_callback fcn, void *data);
-int read_settings_user_id(const char *filename, const char *module, uid_t *userid, gid_t *groupid);
+
+#include <libconfig.h>
+
+typedef struct {
+	config_t cfg;
+} swupdate_cfg_handle;
+
+int swupdate_cfg_init(swupdate_cfg_handle *handle, const char *filename);
+void swupdate_cfg_destroy(swupdate_cfg_handle *handle);
+int read_module_settings(swupdate_cfg_handle *handle, const char *module, settings_callback fcn, void *data);
+int read_settings_user_id(swupdate_cfg_handle *handle, const char *module, uid_t *userid, gid_t *groupid);
 int settings_into_dict(void *settings, void *data);
 #else
-#include <unistd.h>
-static inline int read_module_settings(const char __attribute__ ((__unused__))*filename,
-		const char __attribute__ ((__unused__)) *module,
-		settings_callback __attribute__ ((__unused__)) fcn,
-		void __attribute__ ((__unused__)) *data)
+
+typedef struct {} swupdate_cfg_handle;
+
+static inline int swupdate_cfg_init(swupdate_cfg_handle __attribute__ ((__unused__))*handle,
+		const char __attribute__ ((__unused__))*filename)
 {
 	return -1;
 }
 
+static inline void swupdate_cfg_destroy(swupdate_cfg_handle __attribute__ ((__unused__))*handle) {
+	return;
+}
+
+static inline int read_module_settings(swupdate_cfg_handle __attribute__ ((__unused__))*handle,
+		const char __attribute__ ((__unused__))*module,
+		settings_callback __attribute__ ((__unused__))fcn,
+		void __attribute__ ((__unused__))*data) {
+	return -1;
+};
+
 /*
  * Without LIBCONFIG, let run with current user
  */
-static inline int read_settings_user_id(const char __attribute__ ((__unused__))*filename,
+static inline int read_settings_user_id(swupdate_cfg_handle __attribute__ ((__unused__))*handle,
 					const char __attribute__ ((__unused__))*module,
 					uid_t *userid, gid_t *groupid)
 {
diff --git a/include/util.h b/include/util.h
index a0edd3e..ad2e90c 100644
--- a/include/util.h
+++ b/include/util.h
@@ -17,6 +17,7 @@ 
 #endif
 #include "swupdate.h"
 #include "swupdate_status.h"
+#include "swupdate_settings.h"
 #include "compat.h"
 
 #define NOTIFY_BUF_SIZE 	2048
@@ -208,7 +209,7 @@  char *substring(const char *src, int first, int len);
 size_t snescape(char *dst, size_t n, const char *src);
 void freeargs (char **argv);
 int get_hw_revision(struct hw_type *hw);
-void get_sw_versions(char *cfgfname, struct swupdate_cfg *sw);
+void get_sw_versions(swupdate_cfg_handle *handle, struct swupdate_cfg *sw);
 int compare_versions(const char* left_version, const char* right_version);
 int hwid_match(const char* rev, const char* hwrev);
 int check_hw_compatibility(struct swupdate_cfg *cfg);
diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c
index cae9e89..9014a71 100644
--- a/mongoose/mongoose_interface.c
+++ b/mongoose/mongoose_interface.c
@@ -504,7 +504,12 @@  int start_mongoose(const char *cfgfname, int argc, char *argv[])
 	watchdog_conn = 0;
 
 	if (cfgfname) {
-		read_module_settings(cfgfname, "webserver", mongoose_settings, &opts);
+		swupdate_cfg_handle handle;
+		int ret = swupdate_cfg_init(&handle, cfgfname);
+		if (ret == 0) {
+			read_module_settings(&handle, "webserver", mongoose_settings, &opts);
+		}
+		swupdate_cfg_destroy(&handle);
 	}
 
 	optind = 1;
diff --git a/suricatta/server_general.c b/suricatta/server_general.c
index 1803981..a12ee4b 100644
--- a/suricatta/server_general.c
+++ b/suricatta/server_general.c
@@ -604,11 +604,11 @@  server_op_res_t server_start(char *fname, int argc, char *argv[])
 	LIST_INIT(&server_general.configdata);
 
 	if (fname) {
-
-		read_module_settings(fname, "gservice", server_general_settings,
-					NULL);
-		read_module_settings(fname, "identify", settings_into_dict,
-					&server_general.configdata);
+		swupdate_cfg_handle handle;
+		swupdate_cfg_init(&handle, fname);
+		read_module_settings(&handle, "gservice", server_general_settings, NULL);
+		read_module_settings(&handle, "identify", settings_into_dict, &server_general.configdata);
+		swupdate_cfg_destroy(&handle);
 	}
 
 	if (loglevel >= DEBUGLEVEL) {
diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index df48180..e95ea68 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -1650,19 +1650,21 @@  server_op_res_t server_start(char *fname, int argc, char *argv[])
 	LIST_INIT(&server_hawkbit.configdata);
 
 	if (fname) {
+		swupdate_cfg_handle handle;
+		swupdate_cfg_init(&handle, fname);
+
 		/*
 		 * Search "suricatta" section to be compatible with past
 		 */
-		read_module_settings(fname, "suricatta", server_hawkbit_settings,
-					NULL);
+		read_module_settings(&handle, "suricatta", server_hawkbit_settings, NULL);
 		/*
 		 * Then try "hawkBit" because each server has its own
 		 * section
 		 */
-		read_module_settings(fname, "hawkbit", server_hawkbit_settings,
-					NULL);
-		read_module_settings(fname, "identify", settings_into_dict,
-					&server_hawkbit.configdata);
+		read_module_settings(&handle, "hawkbit", server_hawkbit_settings, NULL);
+		read_module_settings(&handle, "identify", settings_into_dict, &server_hawkbit.configdata);
+
+		swupdate_cfg_destroy(&handle);
 	}
 
 	if (loglevel >= DEBUGLEVEL) {
diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c
index 9e07efb..83b517a 100644
--- a/suricatta/suricatta.c
+++ b/suricatta/suricatta.c
@@ -181,9 +181,14 @@  int start_suricatta(const char *cfgfname, int argc, char *argv[])
 	 * First check for common properties that do not depend
 	 * from server implementation
 	 */
-	if (cfgfname)
-		read_module_settings(cfgfname, "suricatta", suricatta_settings,
-					NULL);
+	if (cfgfname) {
+		swupdate_cfg_handle handle;
+		int ret = swupdate_cfg_init(&handle, cfgfname);
+		if (ret == 0) {
+			read_module_settings(&handle, "suricatta", suricatta_settings, NULL);
+		}
+		swupdate_cfg_destroy(&handle);
+	}
 	optind = 1;
 	opterr = 0;