Add the possibility to set the sockets ownership

From: BELOUARGA Mohamed <m.belouarga@technologyandstrategy.com>

In many projects, where cybersecurity is important. It is needed
to set sockets ownerships to be able to limit the risk of updating
the system from a user without rights. This commit makes it possible
to limit the communication with swupdate.

Signed-off-by: BELOUARGA Mohamed <m.belouarga@technologyandstrategy.com>
---
 Kconfig                 | 40 ++++++++++++++++++++++++++++++++++++++++
 core/network_thread.c   | 11 ++++++++++-
 core/network_utils.c    | 31 ++++++++++++++++++++++++++++++-
 core/progress_thread.c  | 11 ++++++++++-
 include/network_utils.h |  6 +++++-
 5 files changed, 95 insertions(+), 4 deletions(-)

Hi Mohamed,

On 29.11.23 00:17, belouargamohamed@gmail.com wrote:
> From: BELOUARGA Mohamed <m.belouarga@technologyandstrategy.com>
> 
> In many projects, where cybersecurity is important. It is needed
> to set sockets ownerships to be able to limit the risk of updating
> the system from a user without rights. This commit makes it possible
> to limit the communication with swupdate.
> 

Nevertheless, socket ownership is ruled in another way. The changes you 
introduce can be reached with standard tools outside SWUpdate, and 
without changing the code.

SWUpdate supports systemd - that means, SWUpdate is able to get the 
socket created by systemd without creating new ones. Ownership and 
permissions are set by the run unit for the socket, and it is duty of 
the integrator to set up them accordingly with the security required for 
the specific project. There are also cases where SWUpdate does not run 
under root, and calling getpwnam(), chown()etc. fails. That means a 
default set to "root" for SOCKET_CTRL_OWNER_* is wrong, and trieds to 
scale to an upper right.

That means: you as integrator and not SWUpdate are responsible to set up 
the permissions of the sockets - at least with systemd. You can set 
owner / permission by configuring the run unit that create the socket.

With SystemV (or s6, or ..), sockets are still created by SWUpdate. But 
again, we have to distinguish.

Setting the ctrl socket to a user that is not the one that runs SWUpdate 
makes no sense. SWUpdate must still have full control of this socket. 
What makes sense in that case is to change permissions to 660, so to 
remove world access. But trying to grant the ownership to another user 
just creates confusion, and this fails when SWUpdate does not run with 
enough rights to do it.

Removing permission to the progress socket makes no sense IMO: this is 
for design a read only socket. Listeners have no way to change something 
using this socket because SWUpdate is not reading from it. SWUpdate 
generates events about the current update, and it makes sense that 
permissions are more weak. An application running a GUI does not require 
high rights, but it is should be able to show the progress of a running 
update. This does not change security, because as I said, SWUpdate is 
not influenced by this socket and it just uses to send event.

Best regards,
Stefano Babic

> Signed-off-by: BELOUARGA Mohamed <m.belouarga@technologyandstrategy.com>
> ---
>   Kconfig                 | 40 ++++++++++++++++++++++++++++++++++++++++
>   core/network_thread.c   | 11 ++++++++++-
>   core/network_utils.c    | 31 ++++++++++++++++++++++++++++++-
>   core/progress_thread.c  | 11 ++++++++++-
>   include/network_utils.h |  6 +++++-
>   5 files changed, 95 insertions(+), 4 deletions(-)
> 
> diff --git a/Kconfig b/Kconfig
> index 5a3dc9a..f298952 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -218,11 +218,51 @@ config SOCKET_CTRL_PATH
>   	help
>   	  Path to SWUpdate's IPC socket.
>   
> +config SOCKET_CTRL_SET_OWNERSHIP
> +	bool "SWUpdate control socket ownership"
> +	default n
> +	help
> +	  Enable setting the control socket owners (user and group)
> +
> +config SOCKET_CTRL_OWNER_USER
> +	string "SWUpdate control user owner"
> +	depends on SOCKET_CTRL_SET_OWNERSHIP
> +	default "root"
> +	help
> +	  The user that owns the control socket.
> +
> +config SOCKET_CTRL_OWNER_GRP
> +	string "SWUpdate control socket group owner"
> +	depends on SOCKET_CTRL_SET_OWNERSHIP
> +	default "root"
> +	help
> +	  The group that owns the control socket.
> +
>   config SOCKET_PROGRESS_PATH
>   	string "SWUpdate progress socket path"
>   	help
>   	  Path to the socket progress information is sent to.
>   
> +config SOCKET_PROGRESS_SET_OWNERSHIP
> +	bool "SWUpdate control socket ownership"
> +	default n
> +	help
> +	  Enable setting the progress socket owners (user and group).
> +
> +config SOCKET_PROGRESS_OWNER_USER
> +	string "SWUpdate progress socket user owner"
> +	depends on SOCKET_PROGRESS_SET_OWNERSHIP
> +	default "root"
> +	help
> +	  The user that owns the progress socket.
> +
> +config SOCKET_PROGRESS_OWNER_GRP
> +	string "SWUpdate progress socket group owner"
> +	depends on SOCKET_PROGRESS_SET_OWNERSHIP
> +	default "root"
> +	help
> +	  The group that owns the progress socket.
> +
>   config SOCKET_NOTIFIER_DIRECTORY
>   	string "SWUpdate notifier socket directory"
>   	depends on HAVE_FREEBSD
> diff --git a/core/network_thread.c b/core/network_thread.c
> index ca23908..756505b 100644
> --- a/core/network_thread.c
> +++ b/core/network_thread.c
> @@ -39,6 +39,14 @@
>   #define NUM_CACHED_MESSAGES 100
>   #define DEFAULT_INTERNAL_TIMEOUT 60
>   
> +#ifdef CONFIG_SOCKET_CTRL_SET_OWNERSHIP
> +static char* SOCKET_CTRL_OWNER_USER = (char*)CONFIG_SOCKET_CTRL_OWNER_USER;
> +static char* SOCKET_CTRL_OWNER_GRP = (char*)CONFIG_SOCKET_CTRL_OWNER_GRP;
> +#else
> +static char* SOCKET_CTRL_OWNER_USER = NULL;
> +static char* SOCKET_CTRL_OWNER_GRP = NULL;
> +#endif
> +
>   struct msg_elem {
>   	RECOVERY_STATUS status;
>   	int error;
> @@ -396,7 +404,8 @@ void *network_thread (void *data)
>   	subprocess_ipc_handler_thread_id = start_thread(subprocess_thread, NULL);
>   
>   	/* Initialize and bind to UDS */
> -	ctrllisten = listener_create(get_ctrl_socket(), SOCK_STREAM);
> +	ctrllisten = listener_create(get_ctrl_socket(), SOCK_STREAM,
> +				SOCKET_CTRL_OWNER_USER, SOCKET_CTRL_OWNER_GRP);
>   	if (ctrllisten < 0 ) {
>   		ERROR("Error creating IPC control socket");
>   		exit(2);
> diff --git a/core/network_utils.c b/core/network_utils.c
> index 350c7f6..a9fddd6 100644
> --- a/core/network_utils.c
> +++ b/core/network_utils.c
> @@ -13,6 +13,8 @@
>   #include <sys/un.h>
>   #include <sys/stat.h>
>   #include <sys/socket.h>
> +#include <pwd.h>
> +#include <grp.h>
>   
>   #ifdef CONFIG_SYSTEMD
>   #include <systemd/sd-daemon.h>
> @@ -34,10 +36,14 @@ SIMPLEQ_HEAD(self_sockets, socket_meta);
>   static struct self_sockets sockets_toclose;
>   
>   
> -int listener_create(const char *path, int type)
> +int listener_create(const char *path, int type,
> +                const char *owner_user,
> +                const char *owner_grp)
>   {
>   	struct sockaddr_un servaddr;
>   	int listenfd = -1;
> +	struct passwd *pwd;
> +	struct group  *grp;
>   
>   #ifdef CONFIG_SYSTEMD
>   	for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + sd_listen_fds(0); fd++) {
> @@ -84,6 +90,29 @@ int listener_create(const char *path, int type)
>   			close(listenfd);
>   			return -1;
>   		}
> +
> +	if (owner_user != NULL && owner_grp != NULL)
> +	{
> +		/* Change socket ownership */
> +		pwd = getpwnam(owner_user);
> +		grp = getgrnam(owner_grp);
> +		if (pwd == NULL)
> +		{
> +			WARN("Could not get UID of %s", owner_user);
> +			return -1;
> +		}
> +		else if (grp == NULL)
> +		{
> +			WARN("Could not get GID of %s", owner_grp);
> +			return -1;
> +		}
> +		else if (chown(path, pwd->pw_uid, grp->gr_gid) == -1)
> +		{
> +			WARN("Could not change ownership of %s to user %s and group %s",
> +				path, owner_user, owner_grp);
> +			return -1;
> +		}
> +	}
>   	return listenfd;
>   }
>   
> diff --git a/core/progress_thread.c b/core/progress_thread.c
> index 90687f2..144b0ef 100644
> --- a/core/progress_thread.c
> +++ b/core/progress_thread.c
> @@ -36,6 +36,14 @@
>   #include <systemd/sd-daemon.h>
>   #endif
>   
> +#ifdef CONFIG_SOCKET_PROGRESS_SET_OWNERSHIP
> +static char* SOCKET_PROGRESS_OWNER_USER = (char*)CONFIG_SOCKET_PROGRESS_OWNER_USER;
> +static char* SOCKET_PROGRESS_OWNER_GRP  = (char*)CONFIG_SOCKET_PROGRESS_OWNER_GRP;
> +#else
> +static char* SOCKET_PROGRESS_OWNER_USER = NULL;
> +static char* SOCKET_PROGRESS_OWNER_GRP  = NULL;
> +#endif
> +
>   struct progress_conn {
>   	SIMPLEQ_ENTRY(progress_conn) next;
>   	int sockfd;
> @@ -266,7 +274,8 @@ void *progress_bar_thread (void __attribute__ ((__unused__)) *data)
>   	SIMPLEQ_INIT(&pprog->conns);
>   
>   	/* Initialize and bind to UDS */
> -	listen = listener_create(get_prog_socket(), SOCK_STREAM);
> +	listen = listener_create(get_prog_socket(), SOCK_STREAM,
> +				SOCKET_PROGRESS_OWNER_USER, SOCKET_PROGRESS_OWNER_GRP);
>   	if (listen < 0 ) {
>   		ERROR("Error creating IPC socket %s, exiting.", get_prog_socket());
>   		exit(2);
> diff --git a/include/network_utils.h b/include/network_utils.h
> index bbb1d17..de585be 100644
> --- a/include/network_utils.h
> +++ b/include/network_utils.h
> @@ -11,9 +11,13 @@
>    *
>    * \param path absolute path to socket file
>    * \param type socket type of socket()
> + * \param owner_user The user owner of the socket
> + * \param owner_grp The group owner of the socket
>    * \return fd on success, -1 on error
>   */
> -int listener_create(const char *path, int type);
> +int listener_create(const char *path, int type,
> +                const char *owner_user,
> +                const char *owner_grp);
>   
>   /**
>    * \brief initialize unlink functionality for sockets

Hi Stefano,

Thank you for your time and all these explanations.

Best regards,
BELOUARGA Mohamed

Le mercredi 29 novembre 2023 à 09:22:37 UTC+1, Stefano Babic a écrit :

> Hi Mohamed,
>
> On 29.11.23 00:17, belouarg...@gmail.com wrote:
> > From: BELOUARGA Mohamed <m.bel...@technologyandstrategy.com>
> > 
> > In many projects, where cybersecurity is important. It is needed
> > to set sockets ownerships to be able to limit the risk of updating
> > the system from a user without rights. This commit makes it possible
> > to limit the communication with swupdate.
> > 
>
> Nevertheless, socket ownership is ruled in another way. The changes you 
> introduce can be reached with standard tools outside SWUpdate, and 
> without changing the code.
>
> SWUpdate supports systemd - that means, SWUpdate is able to get the 
> socket created by systemd without creating new ones. Ownership and 
> permissions are set by the run unit for the socket, and it is duty of 
> the integrator to set up them accordingly with the security required for 
> the specific project. There are also cases where SWUpdate does not run 
> under root, and calling getpwnam(), chown()etc. fails. That means a 
> default set to "root" for SOCKET_CTRL_OWNER_* is wrong, and trieds to 
> scale to an upper right.
>
> That means: you as integrator and not SWUpdate are responsible to set up 
> the permissions of the sockets - at least with systemd. You can set 
> owner / permission by configuring the run unit that create the socket.
>
> With SystemV (or s6, or ..), sockets are still created by SWUpdate. But 
> again, we have to distinguish.
>
> Setting the ctrl socket to a user that is not the one that runs SWUpdate 
> makes no sense. SWUpdate must still have full control of this socket. 
> What makes sense in that case is to change permissions to 660, so to 
> remove world access. But trying to grant the ownership to another user 
> just creates confusion, and this fails when SWUpdate does not run with 
> enough rights to do it.
>
> Removing permission to the progress socket makes no sense IMO: this is 
> for design a read only socket. Listeners have no way to change something 
> using this socket because SWUpdate is not reading from it. SWUpdate 
> generates events about the current update, and it makes sense that 
> permissions are more weak. An application running a GUI does not require 
> high rights, but it is should be able to show the progress of a running 
> update. This does not change security, because as I said, SWUpdate is 
> not influenced by this socket and it just uses to send event.
>
> Best regards,
> Stefano Babic
>
> > Signed-off-by: BELOUARGA Mohamed <m.bel...@technologyandstrategy.com>
> > ---
> > Kconfig | 40 ++++++++++++++++++++++++++++++++++++++++
> > core/network_thread.c | 11 ++++++++++-
> > core/network_utils.c | 31 ++++++++++++++++++++++++++++++-
> > core/progress_thread.c | 11 ++++++++++-
> > include/network_utils.h | 6 +++++-
> > 5 files changed, 95 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Kconfig b/Kconfig
> > index 5a3dc9a..f298952 100644
> > --- a/Kconfig
> > +++ b/Kconfig
> > @@ -218,11 +218,51 @@ config SOCKET_CTRL_PATH
> > help
> > Path to SWUpdate's IPC socket.
> > 
> > +config SOCKET_CTRL_SET_OWNERSHIP
> > + bool "SWUpdate control socket ownership"
> > + default n
> > + help
> > + Enable setting the control socket owners (user and group)
> > +
> > +config SOCKET_CTRL_OWNER_USER
> > + string "SWUpdate control user owner"
> > + depends on SOCKET_CTRL_SET_OWNERSHIP
> > + default "root"
> > + help
> > + The user that owns the control socket.
> > +
> > +config SOCKET_CTRL_OWNER_GRP
> > + string "SWUpdate control socket group owner"
> > + depends on SOCKET_CTRL_SET_OWNERSHIP
> > + default "root"
> > + help
> > + The group that owns the control socket.
> > +
> > config SOCKET_PROGRESS_PATH
> > string "SWUpdate progress socket path"
> > help
> > Path to the socket progress information is sent to.
> > 
> > +config SOCKET_PROGRESS_SET_OWNERSHIP
> > + bool "SWUpdate control socket ownership"
> > + default n
> > + help
> > + Enable setting the progress socket owners (user and group).
> > +
> > +config SOCKET_PROGRESS_OWNER_USER
> > + string "SWUpdate progress socket user owner"
> > + depends on SOCKET_PROGRESS_SET_OWNERSHIP
> > + default "root"
> > + help
> > + The user that owns the progress socket.
> > +
> > +config SOCKET_PROGRESS_OWNER_GRP
> > + string "SWUpdate progress socket group owner"
> > + depends on SOCKET_PROGRESS_SET_OWNERSHIP
> > + default "root"
> > + help
> > + The group that owns the progress socket.
> > +
> > config SOCKET_NOTIFIER_DIRECTORY
> > string "SWUpdate notifier socket directory"
> > depends on HAVE_FREEBSD
> > diff --git a/core/network_thread.c b/core/network_thread.c
> > index ca23908..756505b 100644
> > --- a/core/network_thread.c
> > +++ b/core/network_thread.c
> > @@ -39,6 +39,14 @@
> > #define NUM_CACHED_MESSAGES 100
> > #define DEFAULT_INTERNAL_TIMEOUT 60
> > 
> > +#ifdef CONFIG_SOCKET_CTRL_SET_OWNERSHIP
> > +static char* SOCKET_CTRL_OWNER_USER = 
> (char*)CONFIG_SOCKET_CTRL_OWNER_USER;
> > +static char* SOCKET_CTRL_OWNER_GRP = 
> (char*)CONFIG_SOCKET_CTRL_OWNER_GRP;
> > +#else
> > +static char* SOCKET_CTRL_OWNER_USER = NULL;
> > +static char* SOCKET_CTRL_OWNER_GRP = NULL;
> > +#endif
> > +
> > struct msg_elem {
> > RECOVERY_STATUS status;
> > int error;
> > @@ -396,7 +404,8 @@ void *network_thread (void *data)
> > subprocess_ipc_handler_thread_id = start_thread(subprocess_thread, NULL);
> > 
> > /* Initialize and bind to UDS */
> > - ctrllisten = listener_create(get_ctrl_socket(), SOCK_STREAM);
> > + ctrllisten = listener_create(get_ctrl_socket(), SOCK_STREAM,
> > + SOCKET_CTRL_OWNER_USER, SOCKET_CTRL_OWNER_GRP);
> > if (ctrllisten < 0 ) {
> > ERROR("Error creating IPC control socket");
> > exit(2);
> > diff --git a/core/network_utils.c b/core/network_utils.c
> > index 350c7f6..a9fddd6 100644
> > --- a/core/network_utils.c
> > +++ b/core/network_utils.c
> > @@ -13,6 +13,8 @@
> > #include <sys/un.h>
> > #include <sys/stat.h>
> > #include <sys/socket.h>
> > +#include <pwd.h>
> > +#include <grp.h>
> > 
> > #ifdef CONFIG_SYSTEMD
> > #include <systemd/sd-daemon.h>
> > @@ -34,10 +36,14 @@ SIMPLEQ_HEAD(self_sockets, socket_meta);
> > static struct self_sockets sockets_toclose;
> > 
> > 
> > -int listener_create(const char *path, int type)
> > +int listener_create(const char *path, int type,
> > + const char *owner_user,
> > + const char *owner_grp)
> > {
> > struct sockaddr_un servaddr;
> > int listenfd = -1;
> > + struct passwd *pwd;
> > + struct group *grp;
> > 
> > #ifdef CONFIG_SYSTEMD
> > for (int fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + 
> sd_listen_fds(0); fd++) {
> > @@ -84,6 +90,29 @@ int listener_create(const char *path, int type)
> > close(listenfd);
> > return -1;
> > }
> > +
> > + if (owner_user != NULL && owner_grp != NULL)
> > + {
> > + /* Change socket ownership */
> > + pwd = getpwnam(owner_user);
> > + grp = getgrnam(owner_grp);
> > + if (pwd == NULL)
> > + {
> > + WARN("Could not get UID of %s", owner_user);
> > + return -1;
> > + }
> > + else if (grp == NULL)
> > + {
> > + WARN("Could not get GID of %s", owner_grp);
> > + return -1;
> > + }
> > + else if (chown(path, pwd->pw_uid, grp->gr_gid) == -1)
> > + {
> > + WARN("Could not change ownership of %s to user %s and group %s",
> > + path, owner_user, owner_grp);
> > + return -1;
> > + }
> > + }
> > return listenfd;
> > }
> > 
> > diff --git a/core/progress_thread.c b/core/progress_thread.c
> > index 90687f2..144b0ef 100644
> > --- a/core/progress_thread.c
> > +++ b/core/progress_thread.c
> > @@ -36,6 +36,14 @@
> > #include <systemd/sd-daemon.h>
> > #endif
> > 
> > +#ifdef CONFIG_SOCKET_PROGRESS_SET_OWNERSHIP
> > +static char* SOCKET_PROGRESS_OWNER_USER = 
> (char*)CONFIG_SOCKET_PROGRESS_OWNER_USER;
> > +static char* SOCKET_PROGRESS_OWNER_GRP = 
> (char*)CONFIG_SOCKET_PROGRESS_OWNER_GRP;
> > +#else
> > +static char* SOCKET_PROGRESS_OWNER_USER = NULL;
> > +static char* SOCKET_PROGRESS_OWNER_GRP = NULL;
> > +#endif
> > +
> > struct progress_conn {
> > SIMPLEQ_ENTRY(progress_conn) next;
> > int sockfd;
> > @@ -266,7 +274,8 @@ void *progress_bar_thread (void __attribute__ 
> ((__unused__)) *data)
> > SIMPLEQ_INIT(&pprog->conns);
> > 
> > /* Initialize and bind to UDS */
> > - listen = listener_create(get_prog_socket(), SOCK_STREAM);
> > + listen = listener_create(get_prog_socket(), SOCK_STREAM,
> > + SOCKET_PROGRESS_OWNER_USER, SOCKET_PROGRESS_OWNER_GRP);
> > if (listen < 0 ) {
> > ERROR("Error creating IPC socket %s, exiting.", get_prog_socket());
> > exit(2);
> > diff --git a/include/network_utils.h b/include/network_utils.h
> > index bbb1d17..de585be 100644
> > --- a/include/network_utils.h
> > +++ b/include/network_utils.h
> > @@ -11,9 +11,13 @@
> > *
> > * \param path absolute path to socket file
> > * \param type socket type of socket()
> > + * \param owner_user The user owner of the socket
> > + * \param owner_grp The group owner of the socket
> > * \return fd on success, -1 on error
> > */
> > -int listener_create(const char *path, int type);
> > +int listener_create(const char *path, int type,
> > + const char *owner_user,
> > + const char *owner_grp);
> > 
> > /**
> > * \brief initialize unlink functionality for sockets
>