[Libstoragemgmt-devel] [PATCH] lsmd: Allow plugins to run as root

[Tony Asleson]

We need the ability to run local plugins as root to allow them
to change the state of the system. This patch does the following:

- We don't drop privileges if the daemon was started with them
- When client connects and after the daemon forks, but before it
exec's the plugin we check to see if the client. Root client,
plugin will execute as root too. Otherwise, the plugin runs
as user libStorageMgmt if it exists. If libStorageMgmt user
does not exist, then the plugin runs with the same privileges
as the client.

I added a -u option to the command line to allow the user to
specify that they want the daemon to run unprivileged. This is
for those users that want extra security and don't need to run
local plugins.

I would appreciate people reviewing this patch closely and see
if it opens any security holes in the design.

Thanks!
-Tony

Signed-off-by: Tony Asleson <***@redhat.com>
---
daemon/lsm_daemon.c | 107 +++++++++++++++++++++++---------
packaging/daemon/libstoragemgmt.service | 2 +-
2 files changed, 79 insertions(+), 30 deletions(-)

diff --git a/daemon/lsm_daemon.c b/daemon/lsm_daemon.c
index acc8ef5..47f269f 100644
--- a/daemon/lsm_daemon.c
+++ b/daemon/lsm_daemon.c
@@ -17,10 +17,10 @@
* Author: tasleson
*/

+#define _GNU_SOURCE
#include <stdio.h>
#include <stdlib.h>
#include <getopt.h>
-#define _GNU_SOURCE
#include <string.h>
#include <ctype.h>
#include <signal.h>
@@ -61,6 +61,7 @@

int verbose_flag = 0;
int systemd = 0;
+int privileged = 1;

char *socket_dir = SOCKET_DIR;
char *plugin_dir = PLUGIN_DIR;
@@ -133,7 +134,7 @@ void signal_handler(int s)
}

/**
- * Installs our signal handler
+ * Installs our signal handlers
*/
void install_sh(void)
{
@@ -142,39 +143,61 @@ void install_sh(void)
}

/**
- * If we are running as root, we will try to drop our privs. to our default
- * user.
+ * Change the process to the specified user and group
+ * @param u User id to change to
+ * @param g Group id to change to
*/
-void drop_privileges(void)
+void change_privs(uid_t u, gid_t g)
{
int err = 0;
- struct passwd *pw = NULL;

- if( !systemd ) {
- pw = getpwnam(LSM_USER);
- if( pw ) {
- if ( !geteuid() ) {
+ if( -1 == setgid(g) ) {
+ err = errno;
+ loud("Unexpected error on setgid(errno %d)\n", err);
+ }

- if( -1 == setgid(pw->pw_gid) ) {
- err = errno;
- loud("Unexpected error on setgid(errno %d)\n", err);
- }
+ if( -1 == setgroups(1, &g) ) {
+ err = errno;
+ loud("Unexpected error on setgroups(errno %d)\n", err);
+ }

- if( -1 == setgroups(1, &pw->pw_gid) ) {
- err = errno;
- loud("Unexpected error on setgroups(errno %d)\n", err);
- }
+ if( -1 == setuid(u) ) {
+ err = errno;
+ loud("Unexpected error on setuid(errno %d)\n", err);
+ }
+}

- if( -1 == setuid(pw->pw_uid) ) {
- err = errno;
- loud("Unexpected error on setuid(errno %d)\n", err);
- }
- } else if ( pw->pw_uid != getuid() ) {
- warn("Daemon not running as correct user\n");
- }
- } else {
- info("Warn: Missing %s user, running as existing user!\n",
+/**
+ * Changes the process to LSM_USER uid/gid if that user exists. Else change
+ * them to the passed in uid/gid
+ * @param client_uid Uid to set process to if LSM_USER does not exist
+ * @param client_gid Gid to set process to if LSM_USER does not exist
+ */
+void set_privs_to_lsm_user(uid_t client_uid, gid_t client_gid)
+{
+ struct passwd *pw = NULL;
+
+ pw = getpwnam(LSM_USER);
+ if( pw ) {
+ change_privs(pw->pw_uid, pw->pw_gid);
+ } else {
+ info("Warn: Missing %s user, running as existing user!\n",
LSM_USER);
+ change_privs(client_uid, client_gid);
+ }
+}
+
+/**
+ * If we are running as root, we will try to drop our privs. to our default
+ * user if the user requested us to.
+ */
+void drop_privileges(void)
+{
+ if ( !privileged && !systemd && !getuid() ) {
+ set_privs_to_lsm_user(getuid(), getgid());
+ } else {
+ if ( !getuid()) {
+ warn("Daemon not started by root, running as existing user!");
}
}
}
@@ -204,12 +227,13 @@ void flight_check(void)
void usage(void)
{
printf("libStorageMgmt plug-in daemon.\n");
- printf("lsmd [--plugindir <directory>] [--socketdir <dir>] [-v] [-d]\n");
+ printf("lsmd [--plugindir <directory>] [--socketdir <dir>] [-v] [-d] [-u]\n");
printf(" --plugindir = The directory where the plugins are located\n");
printf(" --socketdir = The directory where the Unix domain sockets will "
"be created\n");
printf(" -v = Verbose logging\n");
printf(" -d = new style daemon (systemd)\n");
+ printf(" -u = run daemon unprivileged\n");
}

/**
@@ -535,6 +559,27 @@ void exec_plugin( char *plugin, int client_fd )
char fd_str[12];
char *plugin_argv[7];
extern char **environ;
+ struct ucred cr;
+ socklen_t cl = sizeof(cr);
+
+ /*
+ * If we are running as root we need to check to see if the client is
+ * too. If the client is running as root we will execute the plugin
+ * as root, otherwise we will change to libStorageMgmt user if present.
+ * Otherwise the plugin will execute the same as the client
+ * requesting to use it.
+ */
+ if( !getuid() ) {
+ if( 0 == getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &cr, &cl)) {
+ if( cr.uid ) {
+ /* Not root, so drop */
+ set_privs_to_lsm_user(cr.uid, cr.gid);
+ }
+ } else {
+ /* getsockopt failed, we default to dropping then */
+ set_privs_to_lsm_user(cr.uid, cr.gid);
+ }
+ }

/* Make copy of plug-in string as once we call empty_plugin_list it
* will be deleted :-) */
@@ -663,7 +708,7 @@ int main(int argc, char *argv[])
};

int option_index = 0;
- c = getopt_long(argc, argv, "hvd", l_options, &option_index);
+ c = getopt_long(argc, argv, "hvdu", l_options, &option_index);

if ( c == -1 ) {
break;
@@ -693,6 +738,10 @@ int main(int argc, char *argv[])
systemd = 1;
break;

+ case 'u':
+ privileged = 0;
+ break;
+
case '?':
break;

diff --git a/packaging/daemon/libstoragemgmt.service b/packaging/daemon/libstoragemgmt.service
index e0dcf98..81a69ae 100644
--- a/packaging/daemon/libstoragemgmt.service
+++ b/packaging/daemon/libstoragemgmt.service
@@ -6,7 +6,7 @@ After=syslog.target
ExecStart=/usr/bin/lsmd -d
ExecReload=/bin/kill -HUP $MAINPID
StandardError=syslog
-User=libstoragemgmt
+User=root

[Install]
WantedBy=multi-user.target

[Gris Ge]

Post by Tony Asleson
I would appreciate people reviewing this patch closely and see
if it opens any security holes in the design.
Thanks!
-Tony

Hi Tony,

Post by Tony Asleson
+ if( !getuid() ) {
+ if( 0 == getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &cr, &cl)) {

AFAIK, only process with one of these two capabilities(manpage capabilities(7))
can forge the UID in a socket credentials:

CAP_SETUID
CAP_SYS_ADMIN

Not sure that's a security issue or not.
But for me, any of these two capabilities equal to 'root' privilege which has
every right to manage the local storage.

Post by Tony Asleson
+ if( cr.uid ) {
+ /* Not root, so drop */
+ set_privs_to_lsm_user(cr.uid, cr.gid);

If we failed to drop to non-privilege, we will continue to run as root
for all plugins.
It might be a potential security problem.

Not sure related or not, I found an old-fixed kernel bug using setuid()
failure of sendmail to get root privilege:
http://mailman13.u.washington.edu/pipermail/linux/2000-June/001914.html

Post by Tony Asleson
+ }
+ } else {
+ /* getsockopt failed, we default to dropping then */
+ set_privs_to_lsm_user(cr.uid, cr.gid);
+ }
+ }

Thank you.
Best regards.

[Tony Asleson]

Post by Gris Ge

Post by Tony Asleson
+ if( cr.uid ) {
+ /* Not root, so drop */
+ set_privs_to_lsm_user(cr.uid, cr.gid);

If we failed to drop to non-privilege, we will continue to run as root
for all plugins.
It might be a potential security problem.

I agree, that's why if we fail anywhere in the process to drop
privileges the process exits and the plugin never gets executed.

Now that the plugins can execute as root I can see the following which
needs to be mitigated. Some user creates a malicious file
/tmp/lsm_sim_data. Later a root user runs lsmcli and uses the simulator
plugin, result is compromised system. Not sure at the moment what is
the best way is to fix this. In general not using mkstemp or mkdtemp
results in vulnerable code. Suggestions on this one?

What else?

Thanks!

Regards,
-Tony

[Gris Ge]

Post by Tony Asleson
Now that the plugins can execute as root I can see the following which
needs to be mitigated. Some user creates a malicious file
/tmp/lsm_sim_data. Later a root user runs lsmcli and uses the simulator
plugin, result is compromised system. Not sure at the moment what is
the best way is to fix this. In general not using mkstemp or mkdtemp
results in vulnerable code. Suggestions on this one?

Use json.dumps() and json.loads(), or use sqlite.
I intend to use sqlite which could be shared with simc in the future.

Will post a patch soon for sim plugin.

Thank you.
Best regards.

[Tony Asleson]

Post by Gris Ge
Use json.dumps() and json.loads(), or use sqlite.
I intend to use sqlite which could be shared with simc in the future.

I think there is still the potential for bad things to happen even if we
replace pickle with json. For example, sym link sim file to system file
and the plugin gets tricked into over writing it for example. Not sure
of that particular example, but others scenarios could exist.

To improve security even more I'm thinking that we should add a
configuration file to lsmd. That file could contain a list of plugins
that are allowed to run as root. This way only the local plugin would
be running as root if a root user used the command line or the API.

Otherwise I think we should consider placing the state data in a more
suitable location, like /var/run/lsm/state/lsm_sim_data where state
directory is only accessible by root.

Being selective about which plugin runs as root minimises our attack
surface.

Regards,
-Tony