From 07b101ecc84ced454200a21bdcbacd450568753d Mon Sep 17 00:00:00 2001 From: David Zeuthen Date: Mon, 20 Aug 2007 23:01:19 -0400 Subject: [PATCH] gracefully handle bad config/policy files, drop polkit-reload-config, syslog - don't abort/malfunction if the /etc/PolicyKit/PolicyKit.conf configuration file is malformed; simply just continue as normal but return 'no' to every question asked. Also use syslog(3) to report this to the system log - if a .policy file is malformed, simply skip it and still include other well-formed .policy files. Use syslog(3) to report if indeed a .policy file is malformed. - drop /var/lib/PolicyKit/reload and rely on inotify to detect changes to - /etc/PolicyKit/PolicyKit.conf - Policy files in /usr/share/PolicyKit/policy - privileges in /var/lib/PolicyKit and /var/run/PolicyKit As a result, changes made to /etc/PolicyKit/PolicyKit.conf (typically an admin edits this file) and .policy files (typically these can change on package upgrades) in /usr/share/PolicyKit/policy are instantly picked up. --- doc/TODO | 6 -- doc/man/Makefile.am | 2 +- doc/man/PolicyKit.conf.5.in | 12 ++-- doc/man/polkit-reload-config.1.in | 36 ---------- polkit-grant/polkit-grant-helper.c | 10 ++- polkit/Makefile.am | 5 -- polkit/polkit-context.c | 140 +++++++++++++++++++++++++------------ polkit/polkit-policy-cache.c | 12 +++- tools/Makefile.am | 7 -- tools/polkit-reload-config.in | 2 - 10 files changed, 120 insertions(+), 112 deletions(-) delete mode 100644 doc/man/polkit-reload-config.1.in delete mode 100644 tools/polkit-reload-config.in diff --git a/doc/TODO b/doc/TODO index 311a44b..38698b0 100644 --- a/doc/TODO +++ b/doc/TODO @@ -69,11 +69,5 @@ The is a bit like Objects mentioned in the spec (and what we used to have as PolKitResource) but a bit more blurry. It may just work. - - Be more forgiving about bad policy files and bad config files. Hard - problem; it's potentially security sensitive. At least we probably - need to spam the syslog - - - policy descriptions and localization - - Go to 1.0 soon diff --git a/doc/man/Makefile.am b/doc/man/Makefile.am index 5938601..5771b67 100644 --- a/doc/man/Makefile.am +++ b/doc/man/Makefile.am @@ -1,7 +1,7 @@ if MAN_PAGES_ENABLED -MAN_IN_FILES = polkit-check-caller.1.in polkit-check-session.1.in polkit-policy-file-validate.1.in PolicyKit.8.in PolicyKit.conf.5.in polkit-reload-config.1.in polkit-list-actions.1.in +MAN_IN_FILES = polkit-check-caller.1.in polkit-check-session.1.in polkit-policy-file-validate.1.in PolicyKit.8.in PolicyKit.conf.5.in polkit-list-actions.1.in man_MANS = $(MAN_IN_FILES:.in=) diff --git a/doc/man/PolicyKit.conf.5.in b/doc/man/PolicyKit.conf.5.in index 32d4307..79435ef 100644 --- a/doc/man/PolicyKit.conf.5.in +++ b/doc/man/PolicyKit.conf.5.in @@ -17,12 +17,16 @@ to determine whether a caller is privileged to use the mechanism in the way spec .I action identifier. -Changes to this configuration file are not immediately propagated; the -utility \&\fIpolkit-reload-config\fR\|(1) can be used to notify -running processes of the changes to the configuration file. +Changes to this configuration file are immediately propagated to +running processes using the \&\fIlibpolkit\fR\|(3) library. If the +configuration file is invalid, processes using this library will log +this fact to the system logger and the library will only only return +.B no +as the answer to processes using it. .B TODO: -we need to have a tool to verify the PolicyKit.conf file. +we need to have a tool to verify the PolicyKit.conf file is well +formed. For more information about the big picture refer to the \fIPolicyKit spec\fP which can be found in diff --git a/doc/man/polkit-reload-config.1.in b/doc/man/polkit-reload-config.1.in deleted file mode 100644 index 304a358..0000000 --- a/doc/man/polkit-reload-config.1.in +++ /dev/null @@ -1,36 +0,0 @@ -.\" -.\" polkit-reload-config manual page. -.\" Copyright (C) 2007 David Zeuthen -.\" -.TH POLKIT-RELOAD-CONFIG 1 -.SH NAME -polkit-reload-config \- reload configuration -.SH SYNOPSIS -.PP -.B polkit-reload-config - -.SH DESCRIPTION - -\fIpolkit-reload-config\fP can be used to signal all processes using -libpolkit to reload their configuration. For more information about -the big picture refer to the \fIPolicyKit spec\fP which can be found -in -.I "@docdir@/spec/polkit-spec.html" -depending on the distribution. Only the super user can invoke this -tool. - -.SH BUGS -.PP -Please send bug reports to either the distribution or the HAL -mailing list, see -.I "http://lists.freedesktop.org/mailman/listinfo/hal" -on how to subscribe. - -.SH SEE ALSO -.PP -\&\fIPolicyKit\fR\|(8) - -.SH AUTHOR -Written by David Zeuthen with a lot of help from many -others. - diff --git a/polkit-grant/polkit-grant-helper.c b/polkit-grant/polkit-grant-helper.c index bcbcf16..ffcd407 100644 --- a/polkit-grant/polkit-grant-helper.c +++ b/polkit-grant/polkit-grant-helper.c @@ -338,6 +338,10 @@ verify_with_polkit (pid_t caller_pid, const char *admin_auth_data; pk_config = polkit_context_get_config (pol_ctx); + /* if the configuration file is malformed, bail out */ + if (pk_config == NULL) + goto error; + if (polkit_config_determine_admin_auth_type (pk_config, action, caller, @@ -762,12 +766,6 @@ main (int argc, char *argv[]) goto out; } - /* touch the /var/lib/PolicyKit/reload file */ - if (utime (PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit/reload", NULL) != 0) { - fprintf (stderr, "polkit-grant-helper: cannot touch " PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit/reload: %s\n", strerror (errno)); - } - - ret = 0; out: #ifdef PGH_DEBUG diff --git a/polkit/Makefile.am b/polkit/Makefile.am index 155367a..9927b53 100644 --- a/polkit/Makefile.am +++ b/polkit/Makefile.am @@ -64,8 +64,6 @@ libpolkit_la_LDFLAGS = -version-info $(LT_CURRENT):$(LT_REVISION):$(LT_AGE) clean-local : rm -f *~ $(BUILT_SOURCES) -# Create /var/lib/PolicyKit/reload file; this is being watched by libpolkit -# for config file changes. install-data-local: -mkdir -p $(DESTDIR)$(localstatedir)/lib/PolicyKit -mkdir -p $(DESTDIR)$(localstatedir)/run/PolicyKit @@ -73,7 +71,4 @@ install-data-local: -chown $(POLKIT_USER):$(POLKIT_GROUP) $(DESTDIR)$(localstatedir)/run/PolicyKit -chmod 775 $(DESTDIR)$(localstatedir)/lib/PolicyKit -chmod 775 $(DESTDIR)$(localstatedir)/run/PolicyKit - -touch $(DESTDIR)$(localstatedir)/lib/PolicyKit/reload - -chown :$(POLKIT_GROUP) $(DESTDIR)$(localstatedir)/lib/PolicyKit/reload - -chmod 664 $(DESTDIR)$(localstatedir)/lib/PolicyKit/reload diff --git a/polkit/polkit-context.c b/polkit/polkit-context.c index 073b306..d2eb86d 100644 --- a/polkit/polkit-context.c +++ b/polkit/polkit-context.c @@ -36,6 +36,7 @@ #include #include #include +#include #include #include "polkit-config.h" @@ -100,7 +101,10 @@ struct PolKitContext int inotify_fd; int inotify_fd_watch_id; - int inotify_reload_wd; + int inotify_config_wd; + int inotify_policy_wd; + int inotify_grant_perm_wd; + int inotify_grant_temp_wd; }; /** @@ -118,6 +122,23 @@ polkit_context_new (void) pk_context->refcount = 1; return pk_context; } + +static void +_load_config_file (PolKitContext *pk_context) +{ + PolKitError *pk_error; + + pk_context->config = polkit_config_new (&pk_error); + /* if configuration file was bad, log it */ + if (pk_context->config == NULL) { + _pk_debug ("failed to load configuration file: %s", + polkit_error_get_error_message (pk_error)); + syslog (LOG_ALERT, "libpolkit: failed to load configuration file: %s", + polkit_error_get_error_message (pk_error)); + polkit_error_free (pk_error); + } +} + /** * polkit_context_init: * @pk_context: the context object @@ -145,14 +166,8 @@ polkit_context_init (PolKitContext *pk_context, PolKitError **error) /* NOTE: we don't populate the cache until it's needed.. */ /* Load configuration file */ - pk_context->config = polkit_config_new (error); - if (pk_context->config == NULL) { - _pk_debug ("failed to load configuration file: %s", strerror (errno)); - /* TODO: set error. TODO: should we error out if the config file is bad?!? Or recover and go without a config file? */ - goto error; - } + _load_config_file (pk_context); - /* if the client provided watch functions, use inotify to create a watch on /var/lib/PolicyKit/reload */ if (pk_context->io_add_watch_func != NULL) { pk_context->inotify_fd = inotify_init (); if (pk_context->inotify_fd < 0) { @@ -161,11 +176,45 @@ polkit_context_init (PolKitContext *pk_context, PolKitError **error) goto error; } - pk_context->inotify_reload_wd = inotify_add_watch (pk_context->inotify_fd, - PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit/reload", + /* Watch the /etc/PolicyKit/PolicyKit.conf file */ + pk_context->inotify_config_wd = inotify_add_watch (pk_context->inotify_fd, + PACKAGE_SYSCONF_DIR "/PolicyKit/PolicyKit.conf", IN_MODIFY | IN_CREATE | IN_ATTRIB); - if (pk_context->inotify_reload_wd < 0) { - _pk_debug ("failed to add watch on file '" PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit/reload': %s", + if (pk_context->inotify_config_wd < 0) { + _pk_debug ("failed to add watch on file '" PACKAGE_SYSCONF_DIR "/PolicyKit/PolicyKit.conf': %s", + strerror (errno)); + /* TODO: set error */ + goto error; + } + + /* Watch the /usr/share/PolicyKit/policy directory */ + pk_context->inotify_policy_wd = inotify_add_watch (pk_context->inotify_fd, + PACKAGE_DATA_DIR "/PolicyKit/policy", + IN_MODIFY | IN_CREATE | IN_DELETE | IN_ATTRIB); + if (pk_context->inotify_policy_wd < 0) { + _pk_debug ("failed to add watch on directory '" PACKAGE_DATA_DIR "/PolicyKit/policy': %s", + strerror (errno)); + /* TODO: set error */ + goto error; + } + + /* Watch the /var/lib/PolicyKit directory */ + pk_context->inotify_grant_perm_wd = inotify_add_watch (pk_context->inotify_fd, + PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit", + IN_MODIFY | IN_CREATE | IN_DELETE| IN_ATTRIB); + if (pk_context->inotify_grant_perm_wd < 0) { + _pk_debug ("failed to add watch on directory '" PACKAGE_LOCALSTATE_DIR "/lib/PolicyKit': %s", + strerror (errno)); + /* TODO: set error */ + goto error; + } + + /* Watch the /var/run/PolicyKit directory */ + pk_context->inotify_grant_temp_wd = inotify_add_watch (pk_context->inotify_fd, + PACKAGE_LOCALSTATE_DIR "/run/PolicyKit", + IN_MODIFY | IN_CREATE | IN_DELETE| IN_ATTRIB); + if (pk_context->inotify_grant_temp_wd < 0) { + _pk_debug ("failed to add watch on directory '" PACKAGE_LOCALSTATE_DIR "/run/PolicyKit': %s", strerror (errno)); /* TODO: set error */ goto error; @@ -267,10 +316,14 @@ polkit_context_set_config_changed (PolKitContext *pk_context, void polkit_context_io_func (PolKitContext *pk_context, int fd) { + gboolean config_changed; + g_return_if_fail (pk_context != NULL); _pk_debug ("polkit_context_io_func: data on fd %d", fd); + config_changed = FALSE; + if (fd == pk_context->inotify_fd) { /* size of the event structure, not counting name */ #define EVENT_SIZE (sizeof (struct inotify_event)) @@ -296,39 +349,32 @@ again: _pk_debug ("wd=%d mask=%u cookie=%u len=%u", event->wd, event->mask, event->cookie, event->len); - if (event->wd == pk_context->inotify_reload_wd) { - PolKitConfig *new_config; - - _pk_debug ("config changed!"); - - /* purge existing policy files */ - _pk_debug ("purging policy files"); - if (pk_context->priv_cache == NULL) { - polkit_policy_cache_unref (pk_context->priv_cache); - pk_context->priv_cache = NULL; - } - - /* Reload configuration file */ - _pk_debug ("reload configuration file"); - new_config = polkit_config_new (NULL); - if (pk_context->config == NULL) { - _pk_debug ("failed to reload configuration file: %s", strerror (errno)); - /* TODO: set error. TODO: should we error out if the config file is bad?!? Or recover and go without a config file? */ - } else { - if (pk_context->config != NULL) - polkit_config_unref (pk_context->config); - pk_context->config = new_config; - } - - if (pk_context->config_changed_cb != NULL) { - pk_context->config_changed_cb (pk_context, - pk_context->config_changed_user_data); - } - } + _pk_debug ("config changed!"); + config_changed = TRUE; i += EVENT_SIZE + event->len; } } + + if (config_changed) { + /* purge existing policy files */ + _pk_debug ("purging policy files"); + if (pk_context->priv_cache != NULL) { + polkit_policy_cache_unref (pk_context->priv_cache); + pk_context->priv_cache = NULL; + } + + /* Purge old config and reload configuration file */ + _pk_debug ("reloading configuration file"); + if (pk_context->config != NULL) + polkit_config_unref (pk_context->config); + _load_config_file (pk_context); + + if (pk_context->config_changed_cb != NULL) { + pk_context->config_changed_cb (pk_context, + pk_context->config_changed_user_data); + } + } } /** @@ -428,6 +474,10 @@ polkit_context_can_session_do_action (PolKitContext *pk_context, result = POLKIT_RESULT_NO; g_return_val_if_fail (pk_context != NULL, result); + /* if the configuration file is malformed, always say no */ + if (pk_context->config == NULL) + goto out; + if (action == NULL || session == NULL) goto out; @@ -506,6 +556,10 @@ polkit_context_can_caller_do_action (PolKitContext *pk_context, result = POLKIT_RESULT_NO; g_return_val_if_fail (pk_context != NULL, result); + /* if the configuration file is malformed, always say no */ + if (pk_context->config == NULL) + goto out; + if (action == NULL || caller == NULL) goto out; @@ -574,12 +628,12 @@ out: * using PolicyKit should never use this method; it's only here for * integration with other PolicyKit components. * - * Returns: A #PolKitConfig object + * Returns: A #PolKitConfig object or NULL if the configuration file + * is malformed. */ PolKitConfig * polkit_context_get_config (PolKitContext *pk_context) { - g_return_val_if_fail (pk_context != NULL, NULL); return pk_context->config; } diff --git a/polkit/polkit-policy-cache.c b/polkit/polkit-policy-cache.c index 461583d..a196bf9 100644 --- a/polkit/polkit-policy-cache.c +++ b/polkit/polkit-policy-cache.c @@ -35,6 +35,7 @@ #include #include #include +#include #include #include "polkit-debug.h" @@ -100,6 +101,7 @@ _polkit_policy_cache_new (const char *dirname, polkit_bool_t load_descriptions, while ((file = g_dir_read_name (dir)) != NULL) { char *path; PolKitPolicyFile *pf; + PolKitError *pk_error; if (!g_str_has_suffix (file, ".policy")) continue; @@ -110,11 +112,17 @@ _polkit_policy_cache_new (const char *dirname, polkit_bool_t load_descriptions, path = g_strdup_printf ("%s/%s", dirname, file); _pk_debug ("Loading %s", path); - pf = polkit_policy_file_new (path, load_descriptions, error); + pk_error = NULL; + pf = polkit_policy_file_new (path, load_descriptions, &pk_error); g_free (path); if (pf == NULL) { - goto out; + _pk_debug ("libpolkit: ignoring malformed policy file: %s", + polkit_error_get_error_message (pk_error)); + syslog (LOG_ALERT, "libpolkit: ignoring malformed policy file: %s", + polkit_error_get_error_message (pk_error)); + polkit_error_free (pk_error); + continue; } /* steal entries */ diff --git a/tools/Makefile.am b/tools/Makefile.am index 0a71131..1c922c2 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -13,8 +13,6 @@ INCLUDES = \ bin_PROGRAMS = polkit-check-caller polkit-check-session polkit-policy-file-validate polkit-grant polkit-list-actions -bin_SCRIPTS = polkit-reload-config - polkit_check_caller_SOURCES = polkit-check-caller.c polkit_check_caller_LDADD = @GLIB_LIBS@ @DBUS_LIBS@ $(top_builddir)/polkit/libpolkit.la $(top_builddir)/polkit-dbus/libpolkit-dbus.la @@ -30,11 +28,6 @@ polkit_grant_LDADD = @GLIB_LIBS@ @DBUS_LIBS@ $(top_builddir)/polkit/libpolkit.la polkit_list_actions_SOURCES = polkit-list-actions.c polkit_list_actions_LDADD = $(GLIB) $(top_builddir)/polkit/libpolkit.la -polkit-reload-config: polkit-reload-config.in Makefile - $(edit) $< >$@ - -EXTRA_DIST = polkit-reload-config.in - edit = sed \ -e 's|@docdir[@]|$(docdir)|g' \ -e 's|@sbindir[@]|$(sbindir)|g' \ diff --git a/tools/polkit-reload-config.in b/tools/polkit-reload-config.in deleted file mode 100644 index f4739f5..0000000 --- a/tools/polkit-reload-config.in +++ /dev/null @@ -1,2 +0,0 @@ -#!/bin/sh -touch @localstatedir@/lib/PolicyKit/reload -- 2.7.4