[PATCH 00/13] Security: do not run network processes as root

[Stefano Babic]

SWUpdate runs with high privileges because it needs
to write into the hardware. This means that the same
privileges are available for all threads belonging
to SWUpdate, such as the downloader, the webserver
and suricatta.

The patch intends split threads accessing to the
network and move them into separate processes.
This allows to declassthem and run with a different
user / group as the installer itself.

In the configuration file it is possible for each
group (download, webserver, suricatta) to define
the user / groupid of the process.

Stefano Babic (13):
Move code to introduce process control
Use constant for TRACE / ERROR buffer size
Drop double output of ERROR messages
suricatta: wrong integer values from configuration file
settings: add function to extract user/group id
mongoose: changes prototype to be started as process
suricatta: changes prototype to be started as process
swupdate: start mongoose / suricatta as processes
Update configuration example swupdate.cfg
Notifier: get notification between processes
notify: check that pointer is not null
settings: check if configuration file is passed
Move downloader to a separate process

core/Makefile | 1 +
core/notifier.c | 154 ++++++++++++++++++++++++++++++++++--
core/pctl.c | 96 ++++++++++++++++++++++
core/swupdate.c | 97 ++++++++++++++++-------
corelib/downloader.c | 95 +++++++++++++++++++++-
corelib/stream_interface.c | 1 +
corelib/swupdate_settings.c | 35 +++++++-
examples/configuration/swupdate.cfg | 8 +-
include/download_interface.h | 31 ++++++++
include/installer.h | 6 --
include/mongoose_interface.h | 2 +-
include/network_ipc.h | 1 -
include/pctl.h | 33 ++++++++
include/suricatta/suricatta.h | 2 +-
include/swupdate_settings.h | 11 +++
include/util.h | 5 +-
ipc/network_ipc.c | 15 +---
mongoose/mongoose_interface.c | 12 ++-
suricatta/server_hawkbit.c | 15 ++--
suricatta/suricatta.c | 2 +-
20 files changed, 542 insertions(+), 80 deletions(-)
create mode 100644 core/pctl.c
create mode 100644 include/download_interface.h
create mode 100644 include/pctl.h

--
2.7.4

[Stefano Babic]

Security reasons lead that all processes
that communicates via network should not run as root.
SWUpdate, even if does not run as root, must have
high privileges to be able to change the software,
that means the access of most hardware devices must be
guaranteed.
It is desirable to split SWUpdate in a multi-process
application, where network processes can be downgraded
to another user.

Add functions for moving some
parts of SWUpdate into separate
processes.

Signed-off-by: Stefano Babic <sba...@denx.de>
---
core/Makefile | 1 +
core/pctl.c | 94 ++++++++++++++++++++++++++++++++++++++++++++++
core/swupdate.c | 1 +
corelib/stream_interface.c | 1 +
include/network_ipc.h | 1 -
include/pctl.h | 33 ++++++++++++++++
ipc/network_ipc.c | 15 +-------
7 files changed, 131 insertions(+), 15 deletions(-)
create mode 100644 core/pctl.c
create mode 100644 include/pctl.h

diff --git a/core/Makefile b/core/Makefile
index 8629639..659f55b 100644
--- a/core/Makefile
+++ b/core/Makefile
@@ -14,4 +14,5 @@ obj-y += swupdate.o \
handler.o \
util.o \
parser.o \
+ pctl.o \
syslog.o \
diff --git a/core/pctl.c b/core/pctl.c
new file mode 100644
index 0000000..783e8dc
--- /dev/null
+++ b/core/pctl.c
@@ -0,0 +1,94 @@
+/*
+ * (C) Copyright 2016
+ * Stefano Babic, DENX Software Engineering, sba...@denx.de.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <errno.h>
+#include <pthread.h>
+#include <network_ipc.h>
+#include <pctl.h>
+#include <util.h>
+
+/*
+ * The global pid is used to identify if context is
+ * the main process (SWUpdate, pid=0) or it is
+ * a child process.
+ * This is required when using internal libraries and can be
+ * decided between a direct call or (in case of child process)
+ * a IPC is required.
+ */
+int pid = 0;
+
+/*
+ * This is used to spawn internal threads
+ */
+pthread_t start_thread(void *(* start_routine) (void *), void *arg)
+{
+ int ret;
+ pthread_t id;
+ pthread_attr_t attr;
+
+ pthread_attr_init(&attr);
+ pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
+
+ ret = pthread_create(&id, &attr, start_routine, arg);
+ if (ret) {
+ exit(1);
+ }
+ return id;
+}
+
+/*
+ * spawn_process forks and start a new process
+ * under a new user
+ */
+int spawn_process(uid_t run_as_userid, gid_t run_as_groupid, char *cfgname,
+ int ac, char **av, swupdate_process start)
+{
+ int process_id;
+
+ process_id = fork();
+
+ /*
+ * if father, it is finished
+ */
+ if (process_id)
+ return process_id;
+
+ /* process is running as root, drop privileges */
+ if (getuid() == 0) {
+ if (setgid(run_as_groupid) != 0) {
+ ERROR("setgid: Unable to drop group privileges: %s", strerror(errno));
+ return errno;
+ }
+
+ if (setuid(run_as_userid) != 0) {
+ ERROR("setuid: Unable to drop user privileges: %s", strerror(errno));
+ return errno;
+ }
+ }
+
+ /* Save new pid */
+ pid = getpid();
+
+ return (*start)(cfgname, ac, av);
+}
diff --git a/core/swupdate.c b/core/swupdate.c
index d912573..135ef2d 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -53,6 +53,7 @@
#include "progress.h"
#include "parselib.h"
#include "swupdate_settings.h"
+#include "pctl.h"

#define MODULE_NAME "swupdate"

diff --git a/corelib/stream_interface.c b/corelib/stream_interface.c
index 90f649b..a81de2b 100644
--- a/corelib/stream_interface.c
+++ b/corelib/stream_interface.c
@@ -53,6 +53,7 @@
#include "mongoose_interface.h"
#include "installer.h"
#include "progress.h"
+#include "pctl.h"

#define BUFF_SIZE 4096
#define PERCENT_LB_INDEX 4
diff --git a/include/network_ipc.h b/include/network_ipc.h
index 9d9ac34..46827ee 100644
--- a/include/network_ipc.h
+++ b/include/network_ipc.h
@@ -62,6 +62,5 @@ int ipc_wait_for_complete(getstatus callback);
int swupdate_image_write(char *buf, int size);
int swupdate_async_start(writedata wr_func, getstatus status_func,
terminated end_func);
-pthread_t start_thread(void *(* start_routine) (void *), void *arg);

#endif
diff --git a/include/pctl.h b/include/pctl.h
new file mode 100644
index 0000000..db43945
--- /dev/null
+++ b/include/pctl.h
@@ -0,0 +1,33 @@
+/*
+ * (C) Copyright 2016
+ * Stefano Babic, DENX Software Engineering, sba...@denx.de.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ * MA 02111-1307 USA
+ */
+
+#ifndef _SWUPDATE_PCTL_H
+#define _SWUPDATE_PCTL_H
+
+extern int pid;
+
+pthread_t start_thread(void *(* start_routine) (void *), void *arg);
+
+typedef int (*swupdate_process)(char *cfgname, int argc, char **argv);
+
+int spawn_process(uid_t userid, gid_t groupid, char *cfgname, int ac, char **av,
+ swupdate_process start);
+
+#endif
diff --git a/ipc/network_ipc.c b/ipc/network_ipc.c
index 0564ee7..736e725 100644
--- a/ipc/network_ipc.c
+++ b/ipc/network_ipc.c
@@ -41,6 +41,7 @@

#include "util.h"
#include "network_ipc.h"
+#include "pctl.h"

struct async_lib {
int connfd;
@@ -294,18 +295,4 @@ int swupdate_async_start(writedata wr_func, getstatus status_func,
return handle;
}

-pthread_t start_thread(void *(* start_routine) (void *), void *arg)
-{
- int ret;
- pthread_t id;
- pthread_attr_t attr;
-
- pthread_attr_init(&attr);
- pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);

- ret = pthread_create(&id, &attr, start_routine, arg);
- if (ret) {
- exit(1);
- }
- return id;
-}
--
2.7.4

[Stefano Babic]

Signed-off-by: Stefano Babic <sba...@denx.de>
---

include/util.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/util.h b/include/util.h
index f906aa5..737c513 100644
--- a/include/util.h
+++ b/include/util.h
@@ -26,6 +26,8 @@
#include "swupdate.h"
#include "swupdate_status.h"

+#define NOTIFY_BUF_SIZE 2048
+
extern int loglevel;

typedef enum {
@@ -54,7 +56,7 @@ typedef void (*notifier) (RECOVERY_STATUS status, int level, const char *msg);

#define swupdate_notify(status, format, level, arg...) do { \
if (loglevel >= level) { \
- char tmpbuf[2048]; \
+ char tmpbuf[NOTIFY_BUF_SIZE]; \
if (status == FAILURE) { \
if (loglevel >= DEBUGLEVEL) \
snprintf(tmpbuf, sizeof(tmpbuf), \
--
2.7.4

[Stefano Babic]

ERROR messages are not only sent to the notifier,
but also printed directly to the stdout.
This creates double output because the
console notifier is already in charge to
print the messages to stdout.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

include/util.h | 1 -
1 file changed, 1 deletion(-)

diff --git a/include/util.h b/include/util.h
index 737c513..905d0f4 100644
--- a/include/util.h
+++ b/include/util.h
@@ -68,7 +68,6 @@ typedef void (*notifier) (RECOVERY_STATUS status, int level, const char *msg);
else \
snprintf(tmpbuf, sizeof(tmpbuf), \
"ERROR : " format, ## arg); \
- fprintf(stderr, "%s\n", tmpbuf); \
notify(FAILURE, 0, tmpbuf); \
} else {\
snprintf(tmpbuf, sizeof(tmpbuf), \
--
2.7.4

[Stefano Babic]

Integer values (retry and pollwait) are wrongly extracted
from the configuration file.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

suricatta/server_hawkbit.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/suricatta/server_hawkbit.c b/suricatta/server_hawkbit.c
index 076e34a..fd641e9 100644
--- a/suricatta/server_hawkbit.c
+++ b/suricatta/server_hawkbit.c
@@ -1082,15 +1082,13 @@ static int suricatta_settings(void *elem, void __attribute__ ((__unused__)) *da
SETSTRING(server_hawkbit.url, tmp);
mandatory_argument_count |= URL_BIT;
}
- GET_FIELD_STRING(LIBCFG_PARSER, elem, "polldelay", tmp);
- if (strlen(tmp))
- server_hawkbit.polling_interval =
- (unsigned int)strtoul(tmp, NULL, 10);

- GET_FIELD_STRING(LIBCFG_PARSER, elem, "retry", tmp);
- if (strlen(tmp))
- channel_data_defaults.retries =
- (unsigned int)strtoul(tmp, NULL, 10);
+ get_field(LIBCFG_PARSER, elem, "polldelay",
+ &server_hawkbit.polling_interval);
+
+ get_field(LIBCFG_PARSER, elem, "retry",
+ &channel_data_defaults.retries);
+
GET_FIELD_STRING(LIBCFG_PARSER, elem, "retrywait", tmp);
if (strlen(tmp))
channel_data_defaults.retry_sleep =
@@ -1269,6 +1267,7 @@ server_op_res_t server_start(char *fname, int argc, char *argv[])
}
return state_handled; /* Report error to main loop, exiting. */
}
+
return SERVER_OK;
}

--
2.7.4

[Stefano Babic]

Spawned processes can be run under another user.
The couple user / group is read from the

configuration file.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

corelib/swupdate_settings.c | 33 +++++++++++++++++++++++++++++++++
include/swupdate_settings.h | 11 +++++++++++
2 files changed, 44 insertions(+)

diff --git a/corelib/swupdate_settings.c b/corelib/swupdate_settings.c
index cdc6209..6a36029 100644
--- a/corelib/swupdate_settings.c
+++ b/corelib/swupdate_settings.c
@@ -38,6 +38,11 @@
#include "parselib.h"
#include "swupdate_settings.h"

+struct run_as {
+ uid_t userid;
+ gid_t groupid;
+};
+
static config_setting_t *find_settings_node(config_t *cfg,
const char *field)
{
@@ -103,3 +108,31 @@ int read_module_settings(char *filename, const char *module, settings_callback f

return 0;
}
+
+static int get_run_as(void *elem, void *data)
+{
+ struct run_as *pid = (struct run_as *)data;
+
+ get_field(LIBCFG_PARSER, elem, "userid", &pid->userid);
+ get_field(LIBCFG_PARSER, elem, "groupid", &pid->groupid);
+
+ return 0;
+}
+
+int read_settings_user_id(char *filename, const char *module, uid_t *userid, gid_t *groupid)
+{
+ struct run_as ids;
+ int ret;
+
+ ids.userid = getuid();
+ ids.groupid = getgid();
+
+ ret = read_module_settings(filename, module, get_run_as, &ids);
+ if (ret)
+ return -EINVAL;
+
+ *userid = ids.userid;
+ *groupid = ids.groupid;
+
+ return 0;
+}
diff --git a/include/swupdate_settings.h b/include/swupdate_settings.h
index ff25639..771dfe1 100644
--- a/include/swupdate_settings.h
+++ b/include/swupdate_settings.h
@@ -24,6 +24,7 @@

#ifdef CONFIG_LIBCONFIG
int read_module_settings(char *filename, const char *module, settings_callback fcn, void *data);
+int read_settings_user_id(char *filename, const char *module, uid_t *userid, gid_t *groupid);
#else
static inline int read_module_settings(char __attribute__ ((__unused__))*filename,
const char __attribute__ ((__unused__)) *module,
@@ -32,6 +33,16 @@ static inline int read_module_settings(char __attribute__ ((__unused__))*filenam
{
return -1;
}
+
+/*
+ * Without LIBCONFIG, let run with current user
+ */
+int read_settings_user_id(char *filename, const char *module,
+ uid_t *userid, gid_t *groupid);
+{
+ *userid = getuid();
+ *groupid = getgid();
+}
#endif

#endif
--
2.7.4

[Stefano Babic]

Signed-off-by: Stefano Babic <sba...@denx.de>
---

include/mongoose_interface.h | 2 +-
mongoose/mongoose_interface.c | 12 +++++++++---
2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/include/mongoose_interface.h b/include/mongoose_interface.h
index 15cc9ed..81b9181 100644
--- a/include/mongoose_interface.h
+++ b/include/mongoose_interface.h
@@ -29,7 +29,7 @@
/*
* This is used by swupdate to start the Webserver
*/
-void start_mongoose(char *cfgfname, int argc, char *argv[]);
+int start_mongoose(char *cfgfname, int argc, char *argv[]);

void mongoose_print_help(void);

diff --git a/mongoose/mongoose_interface.c b/mongoose/mongoose_interface.c
index adb0111..520e87c 100644
--- a/mongoose/mongoose_interface.c
+++ b/mongoose/mongoose_interface.c
@@ -418,13 +418,13 @@ static void start_mongoose_server(char **options) {
die("%s", "Failed to start Mongoose.");
}

- printf("%s started on port(s) %s with web root [%s]\n",
- server_name, mg_get_option(ctx, "listening_ports"),
+ printf("%s with pid %d started on port(s) %s with web root [%s]\n",
+ server_name, getpid(), mg_get_option(ctx, "listening_ports"),
mg_get_option(ctx, "document_root"));

}

-void start_mongoose(char *cfgfname, int argc, char *argv[])
+int start_mongoose(char *cfgfname, int argc, char *argv[])
{

char *options[MAX_OPTIONS];
@@ -433,4 +433,10 @@ void start_mongoose(char *cfgfname, int argc, char *argv[])
process_command_line_arguments(cfgfname, argc, argv, options);

start_mongoose_server(options);
+
+ while (1) {
+ sleep(1000);
+ }
+
+ return 0;
}
--
2.7.4

[Stefano Babic]

Signed-off-by: Stefano Babic <sba...@denx.de>
---

include/suricatta/suricatta.h | 2 +-
suricatta/suricatta.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/suricatta/suricatta.h b/include/suricatta/suricatta.h
index 3663d57..74c29c8 100644
--- a/include/suricatta/suricatta.h
+++ b/include/suricatta/suricatta.h
@@ -55,5 +55,5 @@ typedef enum {
SERVER_ID_REQUESTED,
} server_op_res_t;

-void start_suricatta(char *cfgname, int argc, char *argv[]) __attribute__((noreturn));
+int start_suricatta(char *cfgname, int argc, char *argv[]) __attribute__((noreturn));
void suricatta_print_help(void);
diff --git a/suricatta/suricatta.c b/suricatta/suricatta.c
index 4a4876f..2913374 100644
--- a/suricatta/suricatta.c
+++ b/suricatta/suricatta.c
@@ -25,7 +25,7 @@
#include "suricatta/suricatta.h"
#include "suricatta/server.h"

-void start_suricatta(char *cfgfname, int argc, char *argv[])
+int start_suricatta(char *cfgfname, int argc, char *argv[])
{
int action_id;
if (server.start(cfgfname, argc, argv) != SERVER_OK) {
--
2.7.4

[Stefano Babic]

While the installer with its handler should
run with high privileges, it is better
that code communicating with the network
runs with lower right.

Webserver / suricatta will be started
with user / groupid (if any) that
can be set into the configuration file.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

core/swupdate.c | 59 +++++++++++++++++++++++++++++++++++++++------------------
1 file changed, 41 insertions(+), 18 deletions(-)

diff --git a/core/swupdate.c b/core/swupdate.c
index 135ef2d..4da15f5 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -631,11 +631,10 @@ int main(int argc, char **argv)
}
}

-#ifdef CONFIG_MTD
- if (strlen(swcfg.globals.mtdblacklist))
- mtd_set_ubiblacklist(swcfg.globals.mtdblacklist);
-#endif
-
+ /*
+ * Parameters are parsed: now performs plausibility
+ * tests before starting processes and threads
+ */
if (public_key_mandatory && !strlen(swcfg.globals.publickeyfname)) {
fprintf(stderr,
"swupdate built for signed image, provide a public key file\n");
@@ -661,6 +660,43 @@ int main(int argc, char **argv)
}

/*
+ * If hust a check is required, do not
+ * start background processes and threads
+ */
+ if (!opt_c) {
+ /* Start embedded web server */
+#if defined(CONFIG_MONGOOSE)
+ if (opt_w) {
+ uid_t webserver_uid;
+ gid_t webserver_gid;
+
+ read_settings_user_id(cfgfname, "webserver",
+ &webserver_uid, &webserver_gid);
+ spawn_process(webserver_uid, webserver_gid, cfgfname,
+ ac, av, start_mongoose);
+ }
+#endif
+
+#if defined(CONFIG_SURICATTA)
+ if (opt_u) {
+ uid_t suricatta_uid;
+ gid_t suricatta_gid;
+
+ read_settings_user_id(cfgfname, "suricatta",
+ &suricatta_uid, &suricatta_gid);
+
+ spawn_process(suricatta_uid, suricatta_gid, cfgfname,
+ argcount, argvalues, start_suricatta);
+ }
+#endif
+ }
+
+#ifdef CONFIG_MTD
+ if (strlen(swcfg.globals.mtdblacklist))
+ mtd_set_ubiblacklist(swcfg.globals.mtdblacklist);
+#endif
+
+ /*
* If a aes key is passed, load it to allow
* to decrypt images
*/
@@ -722,19 +758,6 @@ int main(int argc, char **argv)
else
exit(1);
}
-
- /* Start embedded web server */
-#if defined(CONFIG_MONGOOSE)
- if (opt_w)
- start_mongoose(cfgfname, ac, av);
-#endif
-
-#if defined(CONFIG_SURICATTA)
- if (opt_u) {
- start_suricatta(cfgfname, argcount, argvalues);
- }
-#endif
-
if (opt_w || opt_s || opt_u)
pthread_join(network_daemon, NULL);

--
2.7.4

[Stefano Babic]

Signed-off-by: Stefano Babic <sba...@denx.de>
---

core/notifier.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/core/notifier.c b/core/notifier.c
index b2fe376..8c6b66d 100644
--- a/core/notifier.c
+++ b/core/notifier.c
@@ -104,7 +104,10 @@ void notify(RECOVERY_STATUS status, int error, const char *msg)
if (notifyfd > 0) {
notifymsg.status = status;
notifymsg.error = error;
- strcpy(notifymsg.buf, msg);
+ if (msg)
+ strcpy(notifymsg.buf, msg);
+ else
+ notifymsg.buf[0] = '\0';
sendto(notifyfd, &notifymsg, sizeof(notifymsg), 0,
(struct sockaddr *) &notify_server,
sizeof(struct sockaddr_un));
--
2.7.4

[Stefano Babic]

Change some values from default and add
userid / groupid.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

examples/configuration/swupdate.cfg | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/examples/configuration/swupdate.cfg b/examples/configuration/swupdate.cfg
index 7a4766e..a94a687 100644
--- a/examples/configuration/swupdate.cfg
+++ b/examples/configuration/swupdate.cfg
@@ -42,11 +42,13 @@ suricatta :
id = "123456";
confirm = 0;
url = "http://patito.fritz.box:8080";
- polldelay = 300;
+ polldelay = 360;
nocheckcert = true;
- retry = 3;
+ retry = 4;
retrywait = 200;
loglevel = 10;
+ userid = 1000;
+ groupid = 1000;
/*
cafile = "/etc/ssl/cafile";
sslkey = "/etc/ssl/sslkey";
@@ -57,4 +59,6 @@ suricatta :
webserver :
{
document_root = "./www";
+ userid = 1000;
+ groupid = 1000;
};
--
2.7.4

[Stefano Babic]

Introduce an IPC for all SWUpdate's processes to
have a central place where TRACE / ERROR are managed.
Notification are then dispatched in the usual way.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

core/notifier.c | 151 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
core/pctl.c | 2 +
2 files changed, 145 insertions(+), 8 deletions(-)

diff --git a/core/notifier.c b/core/notifier.c
index f402f1a..b2fe376 100644
--- a/core/notifier.c
+++ b/core/notifier.c
@@ -1,5 +1,5 @@
/*
- * (C) Copyright 2008-2013
+ * (C) Copyright 2013-2016

* Stefano Babic, DENX Software Engineering, sba...@denx.de.

*

* This program is free software; you can redistribute it and/or

@@ -21,10 +21,24 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
+#include <unistd.h>
+#include <errno.h>
+#include <sys/select.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <arpa/inet.h>
+#include <netinet/in.h>
+#include <pthread.h>

#include "bsdqueue.h"
#include "util.h"
+#include "pctl.h"

+/*
+ * There is a list of notifier. Each registered
+ * notifier will receive the notification
+ * and can process it.
+ */
struct notify_elem {
notifier client;
STAILQ_ENTRY(notify_elem) next;
@@ -34,6 +48,30 @@ STAILQ_HEAD(notifylist, notify_elem);

static struct notifylist clients;

+/*
+ * Notification can be sent even by other
+ * processes - if they are started by
+ * SWUpdate.
+ * It is checked if the notification is
+ * coming from the main process - if not,
+ * message is sent to an internal IPC
+ */
+struct notify_ipc_msg {
+ RECOVERY_STATUS status;
+ int error;
+ char buf[NOTIFY_BUF_SIZE];
+};
+
+static struct sockaddr_un notify_client;
+static struct sockaddr_un notify_server;
+static int notifyfd = -1;
+
+/*
+ * This allows to extend the list of notifier.
+ * One can register a new notifier and it will
+ * receive any notification that is sent via
+ * the notify() call
+ */
int register_notifier(notifier client)
{

@@ -50,16 +88,36 @@ int register_notifier(notifier client)
return 0;
}

+/*
+ * Main function to send notification. It is checked
+ * if it is sent by the main process, where the notifier
+ * are running. If not, send the notification via
+ * IPC to the main process that will dispatch it
+ * to the notifiers.
+ */

void notify(RECOVERY_STATUS status, int error, const char *msg)

{
-
struct notify_elem *elem;
-
- STAILQ_FOREACH(elem, &clients, next)
- (elem->client)(status, error, msg);
-
+ struct notify_ipc_msg notifymsg;
+
+ if ((pid == getpid())) {
+ if (notifyfd > 0) {
+ notifymsg.status = status;
+ notifymsg.error = error;
+ strcpy(notifymsg.buf, msg);
+ sendto(notifyfd, &notifymsg, sizeof(notifymsg), 0,
+ (struct sockaddr *) &notify_server,
+ sizeof(struct sockaddr_un));
+ }
+ } else { /* Main process */
+ STAILQ_FOREACH(elem, &clients, next)
+ (elem->client)(status, error, msg);
+ }
}

+/*
+ * Default notifier, it prints to stdout
+ */
static void console_notifier (RECOVERY_STATUS status, int error, const char *msg)
{
char current[80];
@@ -91,8 +149,85 @@ static void console_notifier (RECOVERY_STATUS status, int error, const char *msg
fflush(stdout);
}

+/*
+ * Utility function to setup the internal IPC
+ */
+static void addr_init(struct sockaddr_un *addr, const char *path)
+{
+ memset(addr, 0, sizeof(struct sockaddr_un));
+ addr->sun_family = AF_UNIX;
+ strcpy(&addr->sun_path[1], path);
+ addr->sun_path[0] = 0;
+}
+
+/*
+ * Notifier thread: it runs in the context of the main
+ * process.
+ * This allows to have a central point to manage
+ * all logs.
+ */
+static void *notifier_thread (void __attribute__ ((__unused__)) *data)
+{
+ int serverfd;
+ int len;
+ struct notify_ipc_msg msg;
+
+ /* Initialize and bind to UDS */
+ serverfd = socket(AF_UNIX, SOCK_DGRAM, 0);
+ if (serverfd < 0) {
+ TRACE("Error creating notifier daemon");
+ exit(2);
+ }
+ memset(&notify_server, 0, sizeof(notify_server));
+ notify_server.sun_family = AF_UNIX;
+ strcpy(notify_server.sun_path, "#NotifyServer");
+
+ /*
+ * Use Abstract Socket Address because this is an internal interface
+ */
+ notify_server.sun_path[0] = 0;
+
+ if (bind(serverfd, (const struct sockaddr *) &notify_server,
+ sizeof(struct sockaddr_un)) < 0) {
+ close(serverfd);
+ TRACE("Error bind notifier socket");
+ exit(2);
+ }
+
+ do {
+ len = recvfrom(serverfd, &msg, sizeof(msg), 0, NULL, NULL);
+
+ if (len > 0) {
+ notify(msg.status, msg.error, msg.buf);
+ }
+
+ } while(1);
+}
+
void notify_init(void)
{
- STAILQ_INIT(&clients);
- register_notifier(console_notifier);
+ addr_init(&notify_server, "NotifyServer");
+
+ if (pid == getpid()) {
+ char buf[60];
+ snprintf(buf, sizeof(buf), "Notify%d", pid);
+ addr_init(&notify_client, buf);
+ notifyfd = socket(AF_UNIX, SOCK_DGRAM, 0);
+ if (notifyfd < 0) {
+ printf("Error creating notifier socket for pid %d", pid);
+ return;
+ }
+ if (bind(notifyfd, (const struct sockaddr *) &notify_client,
+ sizeof(struct sockaddr_un)) < 0) {
+ /* Trace cannot work here, use printf */
+ fprintf(stderr, "Cannot initialize notification for pid %d\n",
+ pid);
+ close(notifyfd);
+ return;
+ }
+ } else {
+ STAILQ_INIT(&clients);
+ register_notifier(console_notifier);
+ start_thread(notifier_thread, NULL);
+ }
}
diff --git a/core/pctl.c b/core/pctl.c
index 783e8dc..489c2c0 100644
--- a/core/pctl.c
+++ b/core/pctl.c
@@ -90,5 +90,7 @@ int spawn_process(uid_t run_as_userid, gid_t run_as_groupid, char *cfgname,

/* Save new pid */

pid = getpid();

+ notify_init();

+
return (*start)(cfgname, ac, av);
}

--
2.7.4

[Stefano Babic]

If no configuration file is provided,
the following warning is generated:

(null) 0 file I/O error (null):0 - file I/O error

This is generated after calling libconfig,
but the check can be done before calling the
library.

Signed-off-by: Stefano Babic <sba...@denx.de>
---

corelib/swupdate_settings.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/corelib/swupdate_settings.c b/corelib/swupdate_settings.c
index 6a36029..cfe0bba 100644
--- a/corelib/swupdate_settings.c
+++ b/corelib/swupdate_settings.c
@@ -82,7 +82,7 @@ int read_module_settings(char *filename, const char *module, settings_callback f
config_t cfg;
config_setting_t *elem;

- if (!fcn)
+ if (!fcn || !filename)
return -EINVAL;

memset(&cfg, 0, sizeof(cfg));
--
2.7.4

[Stefano Babic]

Signed-off-by: Stefano Babic <sba...@denx.de>
---

core/swupdate.c | 37 ++++++++++++-----
corelib/downloader.c | 95 ++++++++++++++++++++++++++++++++++++++++++--
include/download_interface.h | 31 +++++++++++++++
include/installer.h | 6 ---
4 files changed, 149 insertions(+), 20 deletions(-)
create mode 100644 include/download_interface.h

diff --git a/core/swupdate.c b/core/swupdate.c
index 4da15f5..ff03fa1 100644
--- a/core/swupdate.c
+++ b/core/swupdate.c
@@ -47,6 +47,7 @@
#endif
#include "lua_util.h"
#include "mongoose_interface.h"
+#include "download_interface.h"
#include "network_ipc.h"
#include "sslapi.h"
#include "suricatta/suricatta.h"
@@ -442,10 +443,8 @@ int main(int argc, char **argv)
int opt_w = 0;
int opt_c = 0;
char image_url[MAX_URL];
- int opt_d = 0;
unsigned long __attribute__ ((__unused__)) opt_t = DL_LOWSPEED_TIME;
int __attribute__ ((__unused__)) opt_r = 3;
- RECOVERY_STATUS result;
char main_options[256];
unsigned int public_key_mandatory = 0;

@@ -460,6 +459,13 @@ int main(int argc, char **argv)
int ac = 0;
#endif

+#ifdef CONFIG_DOWNLOAD
+ char dwloptions[1024];
+ char **dwlav = NULL;
+ int dwlac = 0;
+ int opt_d = 0;
+#endif
+
#ifdef CONFIG_MTD
memset(&flashdesc, 0, sizeof(flashdesc));
#endif
@@ -584,10 +590,13 @@ int main(int argc, char **argv)
usage(argv[0]);
exit(0);
break;
+#ifdef CONFIG_DOWNLOAD
case 'd':
- strncpy(image_url, optarg, sizeof(image_url));
+ (void)snprintf(dwloptions, sizeof(dwloptions), "%s %s", argv[0], optarg);
+ dwlav = splitargs(dwloptions, &dwlac);
opt_d = 1;
break;
+#endif
case 'r':
errno = 0;
opt_r = strtoul(optarg, NULL, 10);
@@ -688,6 +697,19 @@ int main(int argc, char **argv)
spawn_process(suricatta_uid, suricatta_gid, cfgfname,

argcount, argvalues, start_suricatta);
}
+
+#endif
+

+#ifdef CONFIG_DOWNLOAD
+ if (opt_d) {
+ uid_t dwl_uid;
+ gid_t dwl_gid;
+
+ read_settings_user_id(cfgfname, "download",
+ &dwl_uid, &dwl_gid);
+ spawn_process(dwl_uid, dwl_gid, cfgfname,
+ dwlac, dwlav, start_download);
+ }
#endif
}

@@ -751,14 +773,7 @@ int main(int argc, char **argv)
notify(SUCCESS, 0, 0);
}

- if (opt_d) {
- result = download_from_url(image_url, opt_r, opt_t);
- if (result == SUCCESS)
- exit(0);
- else
- exit(1);
- }

- if (opt_w || opt_s || opt_u)

+ if (opt_w || opt_s || opt_u || opt_d)
pthread_join(network_daemon, NULL);

}
diff --git a/corelib/downloader.c b/corelib/downloader.c
index 16df71b..e5efb31 100644
--- a/corelib/downloader.c
+++ b/corelib/downloader.c
@@ -24,6 +24,7 @@
#include <string.h>
#include <errno.h>
#include <ctype.h>
+#include <getopt.h>
#include <sys/types.h>
#include <fcntl.h>
#include <sys/stat.h>
@@ -35,9 +36,25 @@
#include "swupdate.h"
#include "installer.h"
#include "network_ipc.h"
+#include "parselib.h"
+#include "swupdate_status.h"
+#include "swupdate_settings.h"
+#include "download_interface.h"
+
+#define SETSTRING(p, v) do { \
+ if (p) \
+ free(p); \
+ p = strdup(v); \
+} while (0)

static int cnt = 0;

+struct dwl_options {
+ char *url;
+ int retries;
+ int timeout;
+};
+
/* notify download progress each second */
#define MINIMAL_PROGRESS_INTERVAL 1

@@ -46,6 +63,13 @@ struct dlprogress {
CURL *curl;
};

+static struct option long_options[] = {
+ {"url", required_argument, NULL, 'u'},
+ {"retry", required_argument, NULL, 'r'},
+ {"timeout", required_argument, NULL, 't'},
+ {NULL, 0, NULL, 0}};
+
+
/*
* Callback from the libcurl API to progress meter function
* This function gets called by libcurl instead of its internal equivalent.
@@ -166,19 +190,20 @@ static void set_option_common(CURL *curl_handle,
* It si not thought to work with local (file://)
* for that, the -i option is used.
*/
-RECOVERY_STATUS download_from_url(char *image_url, int retries,
+static RECOVERY_STATUS download_from_url(char *image_url, int retries,
unsigned long lowspeed_time)
{
CURL *curl_handle;
CURLcode res = CURLE_GOT_NOTHING;
int fd;
- int attempt = 3;
+ int attempt = 10;
int result;
double dummy;
unsigned long long dwlbytes = 0;
int i;
struct dlprogress progress;

+ TRACE("download from url started : %s", image_url);
if (!strlen(image_url)) {
ERROR("Image URL not provided... aborting download and update\n");
return FAILURE;
@@ -258,7 +283,6 @@ RECOVERY_STATUS download_from_url(char *image_url, int retries,
curl_global_cleanup();

if (res == CURLE_OK) {
- INFO("Success : %llu bytes", dwlbytes);
result = ipc_wait_for_complete(NULL);
} else {
INFO("Error : %s", curl_easy_strerror(res));
@@ -269,3 +293,68 @@ RECOVERY_STATUS download_from_url(char *image_url, int retries,

return result;
}
+
+static int download_settings(void *elem, void __attribute__ ((__unused__)) *data)
+{
+ struct dwl_options *opt = (struct dwl_options *)data;
+ char tmp[128];
+
+ GET_FIELD_STRING(LIBCFG_PARSER, elem, "url", tmp);
+ if (strlen(tmp)) {
+ SETSTRING(opt->url, tmp);
+ }

+
+ get_field(LIBCFG_PARSER, elem, "retry",

+ &opt->retries);
+ get_field(LIBCFG_PARSER, elem, "timeout",
+ &opt->timeout);

+
+ return 0;
+}
+

+int start_download(char *fname, int argc, char *argv[])
+{
+ struct dwl_options options;
+ int choice = 0;
+ RECOVERY_STATUS result;
+
+ memset(&options, 0, sizeof(options));
+ if (fname) {
+ read_module_settings(fname, "download", download_settings,
+ &options);
+ }
+
+
+ /* reset to optind=1 to parse download's argument vector */
+ optind = 1;
+ while ((choice = getopt_long(argc, argv, "t:u:r:",
+ long_options, NULL)) != -1) {
+ switch (choice) {
+ case 't':
+ options.timeout =
+ (unsigned char)strtoul(optarg, NULL, 10);
+ break;
+ case 'u':
+ SETSTRING(options.url, optarg);
+ break;
+ case 'r':
+ options.retries =
+ (unsigned char)strtoul(optarg, NULL, 10);
+ break;
+ case '?':
+ default:

+ return -EINVAL;
+ }
+ }

+
+ result = FAILURE;
+
+ while (1) {
+ if (result == FAILURE)
+ result = download_from_url(options.url, options.retries,
+ options.timeout);
+ sleep(60);

+ }
+
+ return 0;

+}
diff --git a/include/download_interface.h b/include/download_interface.h
new file mode 100644
index 0000000..dd8ef23
--- /dev/null
+++ b/include/download_interface.h
@@ -0,0 +1,31 @@

+/*
+ * (C) Copyright 2016
+ * Stefano Babic, DENX Software Engineering, sba...@denx.de.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc.

+ */
+
+
+#ifndef _DWL_INTERFACE_H
+#define _DWL_INTERFACE_H
+
+/*
+ * This is used by swupdate to start the Downloader Process
+ */
+int start_download(char *cfgfname, int argc, char *argv[]);
+
+void download_print_help(void);
+
+#endif
diff --git a/include/installer.h b/include/installer.h
index 801e4ac..ec70d00 100644
--- a/include/installer.h
+++ b/include/installer.h
@@ -34,10 +34,4 @@ int run_prepost_scripts(struct swupdate_cfg *sw, script_fn type);

void cleanup_files(struct swupdate_cfg *software);

-#ifdef CONFIG_DOWNLOAD
-RECOVERY_STATUS download_from_url(char *image_url, int retries,
- unsigned long lowspeed_time);
-#else
-#define download_from_url(url, retries, lowspeed_time) (0)
-#endif
#endif
--
2.7.4

[Christian Storm]

Hi Stefano,

> While the installer with its handler should
> run with high privileges, it is better
> that code communicating with the network
> runs with lower right.
>
> Webserver / suricatta will be started
> with user / groupid (if any) that
> can be set into the configuration file.
>
> Signed-off-by: Stefano Babic <sba...@denx.de>
> ---
>

> [...]

>
> +
> +#if defined(CONFIG_SURICATTA)
> + if (opt_u) {
> + uid_t suricatta_uid;
> + gid_t suricatta_gid;
> +
> + read_settings_user_id(cfgfname, "suricatta",
> + &suricatta_uid, &suricatta_gid);
> +
> + spawn_process(suricatta_uid, suricatta_gid, cfgfname,
> + argcount, argvalues, start_suricatta);
> + }
> +#endif

I have to say that I'm a bit unhappy with the current implementation:

The process that runs suricatta may (early-) exit. See, e.g.,
suricatta/suricatta.c:32 and the documentation update commit 31f93f8
"suricatta: document initial server connect behavior".
This exit() results in the suricatta process being terminated.

However, the swupdate process is unaffected by this and still runs and
waits forever in core/swupdate.c: pthread_join(network_daemon, NULL); As
the suricatta process is dead, effectively, swupdate is also dead from a
service point of view since no sensible work can be done anymore --
assuming suricatta is the only service functionality provided, that is.

As the main process swupdate is not exit()'ing with an error, there's no
easy way for a service supervision daemon to detect and handle this
situation as it would have to have knowledge about the specific
subprocesses of swupdate and what to do if one dies.

What I'd like to see is some mechanism to also take down the swupdate
process when the suricatta process dies. At least for the initial setup
of the subprocesses, swupdate should be able to react appropriately on,
e.g., fatal initialization errors.


What do you think?


Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Stefano Babic]

Hi Christian,

I understand that this is a big step - anyway, I think this is necessary
for the project. Security cannot be forgotten, and running network
communication under root is not a great idea. I have already got
complaints about this and degrading the communication to the network to
a user with lower privileges is high desired.

Of course, a big step can require some adjustments to make everybody
happy again :-)

> The process that runs suricatta may (early-) exit. See, e.g.,
> suricatta/suricatta.c:32 and the documentation update commit 31f93f8
> "suricatta: document initial server connect behavior".
> This exit() results in the suricatta process being terminated.

Yes, this is now not consistent. It just means that it must be fixed.

>
> However, the swupdate process is unaffected by this and still runs and
> waits forever in core/swupdate.c: pthread_join(network_daemon, NULL); As
> the suricatta process is dead, effectively, swupdate is also dead from a
> service point of view since no sensible work can be done anymore --
> assuming suricatta is the only service functionality provided, that is.
>
> As the main process swupdate is not exit()'ing with an error, there's no
> easy way for a service supervision daemon to detect and handle this
> situation as it would have to have knowledge about the specific
> subprocesses of swupdate and what to do if one dies.
>
> What I'd like to see is some mechanism to also take down the swupdate
> process when the suricatta process dies. At least for the initial setup
> of the subprocesses, swupdate should be able to react appropriately on,
> e.g., fatal initialization errors.
>
>
> What do you think?

The main() function in swupdate ends by checking the network_thread(),
because this was (and it is) the core functionality of SWUpdate.
Supervising this thread should be done in future, too.

The new concept moves the "interfaces" (webserver, suricatta,
downloader,..) to new processes, and they should be supervised as well.

I think a suitable was should be:

- spawn_process() already returns the pid of the created process.
Currently, the value is ignored. SWUpdate should collect all pids.

- Instead of just calling pthread_join, we need to use waitpid for all
processes. It remains the problem to supervise the network thread,
because pthread_join() is blocking. There is a pthread_tryjoin_np(), but
it is not portable. Well, I run only on linux, but ok, it is not the
first solution.

Another solution is to check if the thread is functional: the main
thread could send a IPC_STATUS message on a time basis, just to check
that everything is ok. Supervising becomes then waitpid (in not blocking
mode) + network_thread check.

Best regards,
Stefano Babic

--
=====================================================================
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de
=====================================================================