From 6995d30816f0c76844dee4dc9022296a03258457 Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Wed, 3 Oct 2012 18:21:41 +0200 Subject: Fix a bunch of issues when getting/putting a file from cups There was basically no check for permissions. Now, we temporarily change our effective uid/gid to the one of the user to open the file for writing (when getting) or reading (when putting). We then only use operations that work on the file descriptor to avoid potential race conditions. Before that, people could: - overwrite any file with the content of a cups resource - put any file in a cups resource Part of fix for CVE-2012-4510. --- src/cups-pk-helper-mechanism.c | 32 +++++++- src/cups.c | 171 ++++++++++++++++++++++++++++++++++------- src/cups.h | 16 ++-- 3 files changed, 183 insertions(+), 36 deletions(-) diff --git a/src/cups-pk-helper-mechanism.c b/src/cups-pk-helper-mechanism.c index d456499..a468a2a 100644 --- a/src/cups-pk-helper-mechanism.c +++ b/src/cups-pk-helper-mechanism.c @@ -559,14 +559,28 @@ cph_mechanism_file_get (CphIfaceMechanism *object, const char *filename) { CphMechanism *mechanism = CPH_MECHANISM (object); + unsigned int sender_uid; gboolean ret; _cph_mechanism_emit_called (mechanism); + if (!_cph_mechanism_get_sender_uid (mechanism, context, &sender_uid)) { + GError *error; + + error = g_error_new (CPH_MECHANISM_ERROR, + CPH_MECHANISM_ERROR_GENERAL, + "Cannot determine sender UID"); + g_dbus_method_invocation_return_gerror (context, error); + g_error_free (error); + + return TRUE; + } + if (!_check_polkit_for_action (mechanism, context, "server-settings")) return TRUE; - ret = cph_cups_file_get (mechanism->priv->cups, resource, filename); + ret = cph_cups_file_get (mechanism->priv->cups, + resource, filename, sender_uid); cph_iface_mechanism_complete_file_get ( object, context, @@ -581,14 +595,28 @@ cph_mechanism_file_put (CphIfaceMechanism *object, const char *filename) { CphMechanism *mechanism = CPH_MECHANISM (object); + unsigned int sender_uid; gboolean ret; _cph_mechanism_emit_called (mechanism); + if (!_cph_mechanism_get_sender_uid (mechanism, context, &sender_uid)) { + GError *error; + + error = g_error_new (CPH_MECHANISM_ERROR, + CPH_MECHANISM_ERROR_GENERAL, + "Cannot determine sender UID"); + g_dbus_method_invocation_return_gerror (context, error); + g_error_free (error); + + return TRUE; + } + if (!_check_polkit_for_action (mechanism, context, "server-settings")) return TRUE; - ret = cph_cups_file_put (mechanism->priv->cups, resource, filename); + ret = cph_cups_file_put (mechanism->priv->cups, + resource, filename, sender_uid); cph_iface_mechanism_complete_file_put ( object, context, diff --git a/src/cups.c b/src/cups.c index 7d46c96..6ff6cba 100644 --- a/src/cups.c +++ b/src/cups.c @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -510,6 +511,34 @@ _CPH_CUPS_IS_VALID (filename, "filename", TRUE, CPH_STR_MAXLEN) * Helpers ******************************************************/ +static gboolean +_cph_cups_set_effective_id (unsigned int sender_uid) +{ + struct passwd *password_entry; + + password_entry = getpwuid ((uid_t) sender_uid); + + if (password_entry == NULL || + setegid (password_entry->pw_gid) != 0) + return FALSE; + + if (seteuid (sender_uid) != 0) { + if (getgid () != getegid ()) + setegid (getgid ()); + + return FALSE; + } + + return TRUE; +} + +static void +_cph_cups_reset_effective_id (void) +{ + seteuid (getuid ()); + setegid (getgid ()); +} + static void _cph_cups_add_printer_uri (ipp_t *request, const char *name) @@ -1081,14 +1110,15 @@ cph_cups_is_printer_local (CphCups *cups, } gboolean -cph_cups_file_get (CphCups *cups, - const char *resource, - const char *filename) +cph_cups_file_get (CphCups *cups, + const char *resource, + const char *filename, + unsigned int sender_uid) { http_status_t status; + int fd; struct stat file_stat; - uid_t uid; - gid_t gid; + char *error; g_return_val_if_fail (CPH_IS_CUPS (cups), FALSE); @@ -1097,44 +1127,83 @@ cph_cups_file_get (CphCups *cups, if (!_cph_cups_is_filename_valid (cups, filename)) return FALSE; - stat (filename, &file_stat); - uid = file_stat.st_uid; - gid = file_stat.st_gid; + if (!_cph_cups_set_effective_id (sender_uid)) { + error = g_strdup_printf ("Cannot check if \"%s\" is " + "writable: %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + return FALSE; + } + + fd = open (filename, O_WRONLY | O_NOFOLLOW | O_TRUNC); + + _cph_cups_reset_effective_id (); + + if (fd < 0) { + error = g_strdup_printf ("Cannot open \"%s\": %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + return FALSE; + } + + + if (fstat (fd, &file_stat) != 0) { + error = g_strdup_printf ("Cannot write to \"%s\": %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + close (fd); + + return FALSE; + } + + if (!S_ISREG (file_stat.st_mode)) { + /* hrm, this looks suspicious... we won't help */ + error = g_strdup_printf ("File \"%s\" is not a regular file.", + filename); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + close (fd); + + return FALSE; + } /* reset the internal status: we'll use the http status */ _cph_cups_set_internal_status (cups, NULL); - status = cupsGetFile (cups->priv->connection, resource, filename); + status = cupsGetFd (cups->priv->connection, resource, fd); /* FIXME: There's a bug where the cups connection can fail with EPIPE. - * We're work-arounding it here until it's fixed in cups. */ + * We're working around it here until it's fixed in cups. */ if (status != HTTP_OK) { - if (cph_cups_reconnect (cups)) { - int fd; - - /* if cupsGetFile fail, then filename is unlinked */ - fd = open (filename, O_CREAT, S_IRUSR | S_IWUSR); - close (fd); - chown (filename, uid, gid); - - _cph_cups_set_internal_status (cups, NULL); - - status = cupsGetFile (cups->priv->connection, - resource, filename); - } + if (cph_cups_reconnect (cups)) + status = cupsGetFd (cups->priv->connection, + resource, fd); } + close (fd); + _cph_cups_set_internal_status_from_http (cups, status); return (status == HTTP_OK); } gboolean -cph_cups_file_put (CphCups *cups, - const char *resource, - const char *filename) +cph_cups_file_put (CphCups *cups, + const char *resource, + const char *filename, + unsigned int sender_uid) { http_status_t status; + int fd; + struct stat file_stat; + char *error; g_return_val_if_fail (CPH_IS_CUPS (cups), FALSE); @@ -1143,10 +1212,58 @@ cph_cups_file_put (CphCups *cups, if (!_cph_cups_is_filename_valid (cups, filename)) return FALSE; + if (!_cph_cups_set_effective_id (sender_uid)) { + error = g_strdup_printf ("Cannot check if \"%s\" is " + "readable: %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + return FALSE; + } + + fd = open (filename, O_RDONLY); + + _cph_cups_reset_effective_id (); + + if (fd < 0) { + error = g_strdup_printf ("Cannot open \"%s\": %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + return FALSE; + } + + if (fstat (fd, &file_stat) != 0) { + error = g_strdup_printf ("Cannot read \"%s\": %s", + filename, strerror (errno)); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + close (fd); + + return FALSE; + } + + if (!S_ISREG (file_stat.st_mode)) { + /* hrm, this looks suspicious... we won't help */ + error = g_strdup_printf ("File \"%s\" is not a regular file.", + filename); + _cph_cups_set_internal_status (cups, error); + g_free (error); + + close (fd); + + return FALSE; + } + /* reset the internal status: we'll use the http status */ _cph_cups_set_internal_status (cups, NULL); - status = cupsPutFile (cups->priv->connection, resource, filename); + status = cupsPutFd (cups->priv->connection, resource, fd); + + close (fd); _cph_cups_set_internal_status_from_http (cups, status); diff --git a/src/cups.h b/src/cups.h index 2832533..3017792 100644 --- a/src/cups.h +++ b/src/cups.h @@ -70,13 +70,15 @@ char *cph_cups_printer_get_uri (CphCups *cups, gboolean cph_cups_is_printer_local (CphCups *cups, const char *printer_name); -gboolean cph_cups_file_get (CphCups *cups, - const char *resource, - const char *filename); - -gboolean cph_cups_file_put (CphCups *cups, - const char *resource, - const char *filename); +gboolean cph_cups_file_get (CphCups *cups, + const char *resource, + const char *filename, + unsigned int sender_uid); + +gboolean cph_cups_file_put (CphCups *cups, + const char *resource, + const char *filename, + unsigned int sender_uid); gboolean cph_cups_server_get_settings (CphCups *cups, GVariant **settings); -- cgit v1.2.3