PolicyKit: Branch 'master' - 3 commits
David Zeuthen
david at kemper.freedesktop.org
Tue Dec 15 10:52:04 PST 2009
src/programs/pkexec.c | 212 +++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 191 insertions(+), 21 deletions(-)
New commits:
commit 12e4ee339b38b8ff3742114d69154bb614c37748
Author: David Zeuthen <davidz at redhat.com>
Date: Tue Dec 15 13:48:37 2009 -0500
Make pkexec(1) use the syslogging facilities
Dec 15 13:48:05 localhost pkexec[29065]: davidz: Executing command [USER=root] [TTY=/dev/pts/8] [CWD=/root] [COMMAND=/usr/bin/pk-example-frobnicate]
Dec 15 13:49:30 localhost pkexec[29080]: davidz: The value for the SHELL variable was not found the /etc/shells file [USER=root] [TTY=/dev/pts/5] [CWD=/home/davidz] [COMMAND=/bin/bash]
Dec 15 13:49:45 localhost pkexec[29082]: davidz: The value for environment variable LC_ALL contains suscipious content [USER=root] [TTY=/dev/pts/5] [CWD=/home/davidz] [COMMAND=/bin/bash]
Dec 15 13:50:03 localhost pkexec[29086]: davidz: Error executing command as another user: Not authorized [USER=root] [TTY=/dev/pts/5] [CWD=/home/davidz] [COMMAND=/bin/bash]
Signed-off-by: David Zeuthen <davidz at redhat.com>
diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index 0a24e91..572c5fa 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -23,6 +23,8 @@
# include "config.h"
#endif
+#define _GNU_SOURCE
+
#include <string.h>
#include <stdlib.h>
#include <sys/types.h>
@@ -33,9 +35,15 @@
#include <pwd.h>
#include <errno.h>
#include <security/pam_appl.h>
+#include <syslog.h>
+#include <stdarg.h>
#include <polkit/polkit.h>
+static gchar *original_user_name = NULL;
+static gchar *command_line = NULL;
+static struct passwd *pw;
+
#ifndef HAVE_CLEARENV
extern char **environ;
@@ -60,6 +68,54 @@ usage (int argc, char *argv[])
/* ---------------------------------------------------------------------------------------------------- */
+static void
+log_message (gint level,
+ gboolean print_to_stderr,
+ const gchar *format,
+ ...)
+{
+ static gboolean is_log_open = FALSE;
+ va_list var_args;
+ gchar *s;
+ const gchar *cwd;
+ const gchar *tty;
+
+ if (!is_log_open)
+ {
+ openlog ("pkexec",
+ LOG_PID,
+ LOG_AUTHPRIV); /* security/authorization messages (private) */
+ is_log_open = TRUE;
+ }
+
+ va_start (var_args, format);
+ s = g_strdup_vprintf (format, var_args);
+ va_end (var_args);
+
+ cwd = get_current_dir_name ();
+ tty = ttyname (0);
+ if (tty == NULL)
+ tty = "unknown";
+
+ /* first complain to syslog */
+ syslog (level,
+ "%s: %s [USER=%s] [TTY=%s] [CWD=%s] [COMMAND=%s]",
+ original_user_name,
+ s,
+ pw->pw_name,
+ tty,
+ cwd,
+ command_line);
+
+ /* and then on stderr */
+ if (print_to_stderr)
+ g_printerr ("%s\n", s);
+
+ g_free (s);
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
+
static int
pam_conversation_function (int n,
const struct pam_message **msg,
@@ -275,9 +331,10 @@ validate_environment_variable (const gchar *key,
/* check if it's in /etc/shells */
if (!is_valid_shell (value))
{
- g_printerr ("The value for environment variable SHELL is not included in the\n"
- "/etc/shells file. This incident will be reported. Bailing out.\n");
- /* TODO: syslog */
+ log_message (LOG_CRIT, TRUE,
+ "The value for the SHELL variable was not found the /etc/shells file.");
+ g_printerr ("\n"
+ "This incident has been reported.\n");
goto out;
}
}
@@ -285,10 +342,11 @@ validate_environment_variable (const gchar *key,
strstr (value, "%") != NULL ||
strstr (value, "..") != NULL)
{
- g_printerr ("The value for environment variable %s contains suscipious content\n"
- "indicating an exploit. This incident will be reported. Bailing out.\n",
- key);
- /* TODO: syslog */
+ log_message (LOG_CRIT, TRUE,
+ "The value for environment variable %s contains suscipious content.",
+ key);
+ g_printerr ("\n"
+ "This incident has been reported.\n");
goto out;
}
@@ -314,11 +372,9 @@ main (int argc, char *argv[])
PolkitDetails *details;
GError *error;
gchar *action_id;
- gchar *command_line;
gchar **exec_argv;
gchar *path;
struct passwd pwstruct;
- struct passwd *pw;
gchar pwbuf[8192];
gchar *s;
const gchar *environment_variables_to_save[] = {
@@ -376,6 +432,8 @@ main (int argc, char *argv[])
goto out;
}
+ original_user_name = g_strdup (g_get_user_name ());
+
/* First process options and find the command-line to invoke. Avoid using fancy library routines
* that depend on environtment variables since we haven't cleared the environment just yet.
*/
@@ -459,6 +517,21 @@ main (int argc, char *argv[])
command_line = g_strjoinv (" ", argv + n);
exec_argv = argv + n;
+ /* Look up information about the user we care about - yes, the return
+ * value of this function is a bit funky
+ */
+ rc = getpwnam_r (opt_user, &pwstruct, pwbuf, sizeof pwbuf, &pw);
+ if (rc == 0 && pw == NULL)
+ {
+ g_printerr ("User `%s' does not exist.\n", opt_user);
+ goto out;
+ }
+ else if (pw == NULL)
+ {
+ g_printerr ("Error getting information for user `%s': %s\n", opt_user, g_strerror (rc));
+ goto out;
+ }
+
/* now save the environment variables we care about */
saved_env = g_ptr_array_new ();
for (n = 0; environment_variables_to_save[n] != NULL; n++)
@@ -490,21 +563,6 @@ main (int argc, char *argv[])
goto out;
}
- /* Look up information about the user we care about - yes, the return
- * value of this function is a bit funky
- */
- rc = getpwnam_r (opt_user, &pwstruct, pwbuf, sizeof pwbuf, &pw);
- if (rc == 0 && pw == NULL)
- {
- g_printerr ("User `%s' does not exist.\n", opt_user);
- goto out;
- }
- else if (pw == NULL)
- {
- g_printerr ("Error getting information for user `%s': %s\n", opt_user, g_strerror (rc));
- goto out;
- }
-
/* Initialize the GLib type system - this is needed to interact with the
* PolicyKit daemon
*/
@@ -583,14 +641,15 @@ main (int argc, char *argv[])
}
else if (polkit_authorization_result_get_is_challenge (result))
{
- g_printerr ("Authorization requires authentication but no authentication agent was found.\n");
- /* TODO: syslog */
+ g_printerr ("Error executing command as another user: No authentication agent was found.\n");
goto out;
}
else
{
- g_printerr ("Not authorized.\n");
- /* TODO: syslog */
+ log_message (LOG_WARNING, TRUE,
+ "Error executing command as another user: Not authorized");
+ g_printerr ("\n"
+ "This incident has been reported.\n");
goto out;
}
@@ -704,7 +763,8 @@ main (int argc, char *argv[])
goto out;
}
- /* TODO: syslog */
+ /* Log the fact that we're executing a command */
+ log_message (LOG_NOTICE, FALSE, "Executing command");
/* exec the program */
if (execv (path, exec_argv) != 0)
@@ -740,6 +800,7 @@ main (int argc, char *argv[])
g_free (path);
g_free (command_line);
g_free (opt_user);
+ g_free (original_user_name);
return ret;
}
commit 5a388b6ebb095de6dc7f315b651a84fc31d268d7
Author: David Zeuthen <davidz at redhat.com>
Date: Tue Dec 15 13:08:55 2009 -0500
Make pkexec(1) validate environment variables
Suggested here
http://lists.freedesktop.org/archives/polkit-devel/2009-December/000279.html
Signed-off-by: David Zeuthen <davidz at redhat.com>
diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index dcc618d..0a24e91 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -208,6 +208,97 @@ find_action_for_path (PolkitAuthority *authority,
return action_id;
}
+/* ---------------------------------------------------------------------------------------------------- */
+
+static gboolean
+is_valid_shell (const gchar *shell)
+{
+ gboolean ret;
+ gchar *contents;
+ gchar **shells;
+ GError *error;
+ guint n;
+
+ ret = FALSE;
+
+ contents = NULL;
+ shells = NULL;
+
+ error = NULL;
+ if (!g_file_get_contents ("/etc/shells",
+ &contents,
+ NULL, /* gsize *length */
+ &error))
+ {
+ g_printerr ("Error getting contents of /etc/shells: %s\n", error->message);
+ g_error_free (error);
+ goto out;
+ }
+
+ shells = g_strsplit (contents, "\n", 0);
+ for (n = 0; shells != NULL && shells[n] != NULL; n++)
+ {
+ if (g_strcmp0 (shell, shells[n]) == 0)
+ {
+ ret = TRUE;
+ goto out;
+ }
+ }
+
+ out:
+ g_free (contents);
+ g_strfreev (shells);
+ return ret;
+}
+
+static gboolean
+validate_environment_variable (const gchar *key,
+ const gchar *value)
+{
+ gboolean ret;
+
+ /* Generally we bail if any environment variable value contains
+ *
+ * - '/' characters
+ * - '%' characters
+ * - '..' substrings
+ */
+
+ g_return_val_if_fail (key != NULL, FALSE);
+ g_return_val_if_fail (value != NULL, FALSE);
+
+ ret = FALSE;
+
+ /* special case $SHELL */
+ if (g_strcmp0 (key, "SHELL") == 0)
+ {
+ /* check if it's in /etc/shells */
+ if (!is_valid_shell (value))
+ {
+ g_printerr ("The value for environment variable SHELL is not included in the\n"
+ "/etc/shells file. This incident will be reported. Bailing out.\n");
+ /* TODO: syslog */
+ goto out;
+ }
+ }
+ else if (strstr (value, "/") != NULL ||
+ strstr (value, "%") != NULL ||
+ strstr (value, "..") != NULL)
+ {
+ g_printerr ("The value for environment variable %s contains suscipious content\n"
+ "indicating an exploit. This incident will be reported. Bailing out.\n",
+ key);
+ /* TODO: syslog */
+ goto out;
+ }
+
+ ret = TRUE;
+
+ out:
+ return ret;
+}
+
+/* ---------------------------------------------------------------------------------------------------- */
int
main (int argc, char *argv[])
@@ -231,12 +322,19 @@ main (int argc, char *argv[])
gchar pwbuf[8192];
gchar *s;
const gchar *environment_variables_to_save[] = {
+ "SHELL",
"LANG",
+ "LINGUAS",
"LANGUAGE",
- "LC_ALL",
+ "LC_COLLATE",
+ "LC_CTYPE",
"LC_MESSAGES",
- "SHELL",
+ "LC_MONETARY",
+ "LC_NUMERIC",
+ "LC_TIME",
+ "LC_ALL",
"TERM",
+ "COLORTERM",
/* For now, avoiding pretend that running X11 apps as another user in the same session
* will ever work... See
@@ -372,6 +470,13 @@ main (int argc, char *argv[])
if (value == NULL)
continue;
+ /* To qualify for the paranoia goldstar - we validate the value of each
+ * environment variable passed through - this is to attempt to avoid
+ * exploits in (potentially broken) programs launched via pkexec(1).
+ */
+ if (!validate_environment_variable (key, value))
+ goto out;
+
g_ptr_array_add (saved_env, g_strdup (key));
g_ptr_array_add (saved_env, g_strdup (value));
}
@@ -479,11 +584,13 @@ main (int argc, char *argv[])
else if (polkit_authorization_result_get_is_challenge (result))
{
g_printerr ("Authorization requires authentication but no authentication agent was found.\n");
+ /* TODO: syslog */
goto out;
}
else
{
g_printerr ("Not authorized.\n");
+ /* TODO: syslog */
goto out;
}
@@ -597,6 +704,8 @@ main (int argc, char *argv[])
goto out;
}
+ /* TODO: syslog */
+
/* exec the program */
if (execv (path, exec_argv) != 0)
{
commit 079bc59310ec3ea4fc260a855f39ec3f08f6bdec
Author: David Zeuthen <davidz at redhat.com>
Date: Tue Dec 15 12:19:44 2009 -0500
Fix error message when no authentication agent is available
Signed-off-by: David Zeuthen <davidz at redhat.com>
diff --git a/src/programs/pkexec.c b/src/programs/pkexec.c
index ae0d842..dcc618d 100644
--- a/src/programs/pkexec.c
+++ b/src/programs/pkexec.c
@@ -478,7 +478,7 @@ main (int argc, char *argv[])
}
else if (polkit_authorization_result_get_is_challenge (result))
{
- g_printerr ("Authorization requires authentication but no authentication was found.\n");
+ g_printerr ("Authorization requires authentication but no authentication agent was found.\n");
goto out;
}
else
More information about the hal-commit
mailing list