[Libstoragemgmt-devel] [PATCH 2/4] lsmd: Clean up logging

[Tony Asleson]

- Renamed loud() to log_and_exit()
- Changes the logger function to use severity in syslog
call

Signed-off-by: Tony Asleson <***@redhat.com>
---
daemon/lsm_daemon.c | 61 ++++++++++++++++++++++++++++-------------------------
1 file changed, 32 insertions(+), 29 deletions(-)

diff --git a/daemon/lsm_daemon.c b/daemon/lsm_daemon.c
index eb44d3f..8aba12b 100644
--- a/daemon/lsm_daemon.c
+++ b/daemon/lsm_daemon.c
@@ -97,15 +97,18 @@ void logger(int severity, const char *fmt, ...)
{
char buf[2048];

- if( (LOG_INFO == severity && verbose_flag) || LOG_ERR == LOG_WARNING
- || LOG_ERR == severity) {
+ if( verbose_flag || LOG_WARNING == severity || LOG_ERR == severity) {
va_list arg;
va_start(arg, fmt);
vsnprintf(buf, sizeof(buf), fmt, arg);
va_end(arg);

if( !systemd ) {
- syslog(LOG_ERR, "%s", buf);
+ if( verbose_flag ) {
+ syslog(LOG_ERR, "%s", buf);
+ } else {
+ syslog(severity, "%s", buf);
+ }
} else {
fprintf(stdout, "%s", buf);
fflush(stdout);
@@ -116,7 +119,7 @@ void logger(int severity, const char *fmt, ...)
}
}
}
-#define loud(fmt, ...) logger(LOG_ERR, fmt, ##__VA_ARGS__)
+#define log_and_exit(fmt, ...) logger(LOG_ERR, fmt, ##__VA_ARGS__)
#define warn(fmt, ...) logger(LOG_WARNING, fmt, ##__VA_ARGS__)
#define info(fmt, ...) logger(LOG_INFO, fmt, ##__VA_ARGS__)

@@ -153,17 +156,17 @@ void change_privs(uid_t u, gid_t g)

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

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

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

@@ -181,8 +184,7 @@ void set_privs_to_lsm_user(uid_t client_uid, gid_t client_gid)
if( pw ) {
change_privs(pw->pw_uid, pw->pw_gid);
} else {
- info("Warn: Missing %s user, running as existing user!\n",
- LSM_USER);
+ warn("Missing %s user, running as existing user!\n", LSM_USER);
change_privs(client_uid, client_gid);
}
}
@@ -210,14 +212,14 @@ void flight_check(void)
int err = 0;
if( -1 == access(socket_dir, R_OK|W_OK )) {
err = errno;
- loud("Unable to access socket directory %s, errno= %d\n",
- socket_dir, err);
+ log_and_exit("Unable to access socket directory %s, errno= %d\n",
+ socket_dir, err);
}

if( -1 == access(plugin_dir, R_OK|X_OK)) {
err = errno;
- loud("Unable to access plug-in directory %s, errno= %d\n",
- plugin_dir, err);
+ log_and_exit("Unable to access plug-in directory %s, errno= %d\n",
+ plugin_dir, err);
}
}

@@ -249,7 +251,7 @@ char *path_form(const char* path, const char *name)
if( full ) {
snprintf(full, s, "%s/%s", path, name);
} else {
- loud("malloc failure while trying to allocate %d bytes\n", s);
+ log_and_exit("malloc failure while trying to allocate %d bytes\n", s);
}
return full;
}
@@ -301,12 +303,13 @@ void process_directory( char *dir, void *p, file_op call_back)

if( closedir(dp) ) {
err = errno;
- loud("Error on closing dir %s: %s\n", dir, strerror(err));
+ log_and_exit("Error on closing dir %s: %s\n", dir,
+ strerror(err));
}
} else {
err = errno;
- loud("Error on processing directory %s: %s\n", dir,
- strerror(err));
+ log_and_exit("Error on processing directory %s: %s\n", dir,
+ strerror(err));
}
}
}
@@ -328,7 +331,7 @@ int delete_socket(void *p, char *full_name)
if( S_ISSOCK(statbuf.st_mode)) {
if( unlink(full_name) ) {
err = errno;
- loud("Error on unlinking file %s: %s\n",
+ log_and_exit("Error on unlinking file %s: %s\n",
full_name, strerror(err));
}
}
@@ -375,23 +378,23 @@ int setup_socket(char *full_name)

if( -1 == bind(fd, (struct sockaddr *)&addr, sizeof(struct sockaddr_un))) {
err = errno;
- loud("Error on binding socket %s: %s\n", socket_file, strerror(err));
+ log_and_exit("Error on binding socket %s: %s\n", socket_file, strerror(err));
}

if( -1 == chmod(socket_file, S_IREAD|S_IWRITE|S_IRGRP|S_IWGRP
|S_IROTH|S_IWOTH)) {
err = errno;
- loud("Error on chmod socket file %s: %s\n", socket_file, strerror(err));
+ log_and_exit("Error on chmod socket file %s: %s\n", socket_file, strerror(err));
}

if( -1 == listen(fd, 5)) {
err = errno;
- loud("Error on listening %s: %s\n", socket_file, strerror(err));
+ log_and_exit("Error on listening %s: %s\n", socket_file, strerror(err));
}

} else {
err = errno;
- loud("Error on socket create %s: %s\n",
+ log_and_exit("Error on socket create %s: %s\n",
socket_file, strerror(err));
}

@@ -453,10 +456,10 @@ int process_plugin(void *p, char *full_name)
setup_socket will exit on error. */
free(item);
item = NULL;
- loud("strdup failed %s\n", full_name);
+ log_and_exit("strdup failed %s\n", full_name);
}
} else {
- loud("Memory allocation failure!\n");
+ log_and_exit("Memory allocation failure!\n");
}
}
}
@@ -480,7 +483,7 @@ void child_cleanup(void)

if( -1 == rc ) {
err = errno;
- warn("Error: waitid %d - %s\n", err, strerror(err));
+ info("waitid %d - %s\n", err, strerror(err));
break;
} else {
if( 0 == rc && si.si_pid == 0 ) {
@@ -611,7 +614,7 @@ void exec_plugin( char *plugin, int client_fd )

if( -1 == exec_rc ) {
int err = errno;
- loud("Error on exec'ing Plugin %s: %s\n",
+ log_and_exit("Error on exec'ing Plugin %s: %s\n",
p_copy, strerror(err));
}
}
@@ -643,7 +646,7 @@ void _serving(void)
}

if( !nfds ) {
- loud("No plugins found in directory %s\n", plugin_dir);
+ log_and_exit("No plugins found in directory %s\n", plugin_dir);
}

nfds += 1;
@@ -654,7 +657,7 @@ void _serving(void)
return;
} else {
err = errno;
- loud("Error on selecting Plugin: %s", strerror(err));
+ log_and_exit("Error on selecting Plugin: %s", strerror(err));
}
} else if( ready > 0 ) {
int fd = 0;
@@ -778,7 +781,7 @@ int main(int argc, char *argv[])
if( !systemd ) {
if ( -1 == daemon(0, 0) ) {
int err = errno;
- loud("Error on calling daemon: %s\n", strerror(err));
+ log_and_exit("Error on calling daemon: %s\n", strerror(err));
}
}

[Tony Asleson]

The message is "plugin_unregister" not "shutdown"

Signed-off-by: Tony Asleson <***@redhat.com>
---
c_binding/lsm_plugin_ipc.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/c_binding/lsm_plugin_ipc.cpp b/c_binding/lsm_plugin_ipc.cpp
index ced64d3..7e0d034 100644
--- a/c_binding/lsm_plugin_ipc.cpp
+++ b/c_binding/lsm_plugin_ipc.cpp
@@ -282,6 +282,7 @@ typedef int (*handler)(lsm_plugin_ptr p, Value &params, Value &response);

static int handle_unregister(lsm_plugin_ptr p, Value &params, Value &response)
{
+ /* This is handled in the event loop */
return LSM_ERR_OK;
}

@@ -2198,7 +2199,7 @@ static int lsm_plugin_run(lsm_plugin_ptr p)
error_send(p, rc);
}

- if( method == "shutdown" ) {
+ if( method == "plugin_unregister" ) {
flags = LSM_FLAG_GET_VALUE(req["params"]);
break;
}

[Tony Asleson]

In the cases where the command line is exiting because of an
invalid parameter we are not allowing the client library to
close cleanly causing the plugin to report a syslog message.

Fixed this error case and added a check to the plugin to only
report this message if the client is not following protocol.

Signed-off-by: Tony Asleson <***@redhat.com>
---
python_binding/lsm/_pluginrunner.py | 6 ++++--
tools/lsmcli/cmdline.py | 20 +++++++++++++++++---
2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/python_binding/lsm/_pluginrunner.py b/python_binding/lsm/_pluginrunner.py
index 49f7a96..a6d095c 100644
--- a/python_binding/lsm/_pluginrunner.py
+++ b/python_binding/lsm/_pluginrunner.py
@@ -132,8 +132,10 @@ class PluginRunner(object):
self.tp.send_error(msg_id, lsm_err.code, lsm_err.msg,
lsm_err.data)
except _SocketEOF:
- #Client went away
- error('Client went away, exiting plug-in')
+ #Client went away and didn't meet our expectations for protocol,
+ #this error message should not be seen as it shouldn't be occuring.
+ if need_shutdown:
+ error('Client went away, exiting plug-in')
except Exception:
error("Unhandled exception in plug-in!\n" + traceback.format_exc())

diff --git a/tools/lsmcli/cmdline.py b/tools/lsmcli/cmdline.py
index 1bb6135..a70b38f 100644
--- a/tools/lsmcli/cmdline.py
+++ b/tools/lsmcli/cmdline.py
@@ -48,19 +48,33 @@ def cmd_line_wrapper(c=None):
"""
Common command line code, called.
"""
+ err_exit = 0
+ cli = None
+
try:
cli = CmdLine()
cli.process(c)
except ArgError as ae:
sys.stderr.write(str(ae))
sys.stderr.flush()
- sys.exit(2)
+ err_exit = 2
except LsmError as le:
sys.stderr.write(str(le) + "\n")
sys.stderr.flush()
- sys.exit(4)
+ err_exit = 4
except KeyboardInterrupt:
- sys.exit(1)
+ err_exit = 1
+ finally:
+ # We got here because of an exception, but we still may have a valid
+ # connection to do an orderly shutdown with, lets try it before we
+ # just exit closing the connection.
+ if cli:
+ try:
+ # This will exit if are successful
+ cli.shutdown(err_exit)
+ except Exception:
+ pass
+ sys.exit(err_exit)


## Get a character from stdin without needing a return key pressed.

[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

V2: Correct conditional for not running as root as start.

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..eb44d3f 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 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.

Hi Tony,

How about we add a config file defining the paths of plugins which
are allowed to run as root?

Happy new year!

[Tony Asleson]

Post by Gris Ge

Post by Tony Asleson
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.

Hi Tony,
How about we add a config file defining the paths of plugins which
are allowed to run as root?

This is a good idea.

How can we implement this so that the user doesn't have to edit a
configuration file by hand? Ideally when the plugin package gets
installed it would add an entry and remove the entry when uninstalled.
Not sure what is the best approach to accommodate the different package
installers out there.

Any suggestions?

Post by Gris Ge
Happy new year!

Happy new year too!

Regards,
Tony

[Gris Ge]

Post by Tony Asleson
Any suggestions?

Hi Tony,

I am intend to use the 'yum' way.

/etc/lsm/lsm.conf
# searching path, socket path, allow run as root, debug level and etc.
/etc/lsm/local.conf or /etc/lsm/plugin/local.conf
# allow-root-privilege = true

Comments?

Best regards.

[Tony Asleson]

Post by Gris Ge

Post by Tony Asleson
Any suggestions?

Hi Tony,
I am intend to use the 'yum' way.
/etc/lsm/lsm.conf
# searching path, socket path, allow run as root, debug level and etc.
/etc/lsm/local.conf or /etc/lsm/plugin/local.conf
# allow-root-privilege = true

Hi Gris,

You had mentioned having a configuration file that "defining the paths
of plugins which are allowed to run as root". I was thinking you meant
something like allow-root-privilege=/usr/bin/local_lsmplugin, but what
you have works too.

Following yum, perhaps the directory should be:
/etc/lsm/pluginconf.d/

Regards,
Tony

[Gris Ge]

Post by Tony Asleson

Post by Gris Ge

Post by Tony Asleson
Any suggestions?

Hi Tony,
I am intend to use the 'yum' way.
/etc/lsm/lsm.conf
# searching path, socket path, allow run as root, debug level and etc.
/etc/lsm/local.conf or /etc/lsm/plugin/local.conf
# allow-root-privilege = true

Hi Gris,
You had mentioned having a configuration file that "defining the paths
of plugins which are allowed to run as root". I was thinking you meant
something like allow-root-privilege=/usr/bin/local_lsmplugin, but what
you have works too.
/etc/lsm/pluginconf.d/
Regards,
Tony

Hi Tony,

Since lsmd already take '--plugindir' for the searching folder, it's
easy to get the full path out from config file, example:

/etc/lsm/lsmd.conf
plugindir=/usr/bin
allow-plugin-root-privilege=true
/etc/lsm/pluginconf.d/local.conf
allow-root-privilege=true

# this indicate plugin '/usr/bin/local_lsmplugin' could be run as
# root.

This drop-in config layout allow third party plugin work out of box
for root privilege without user's intervention.

User can explicitly disable root privilege globally in lsmd.conf or
locally in <plugin_name>.conf.

Comments?
Best regards.