Attachment 624105 Details for Bug 864515 – Relevant patch from Vincent Untz

[patch] Relevant patch from Vincent Untz

0001-Fix-a-bunch-of-issues-when-getting-putting-a-file-fr.patch (text/plain), 11.88 KB, created by Jan Lieskovsky on 2012-10-09 13:36:20 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jan Lieskovsky

Created: 2012-10-09 13:36:20 UTC

Size: 11.88 KB

patch

obsolete

>From 03916ec94e396a9264ac4f76453c301a54120ebd Mon Sep 17 00:00:00 2001
>From: Vincent Untz <vuntz@suse.com>
>Date: Wed, 3 Oct 2012 18:21:41 +0200
>Subject: [PATCH] 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
>---
> 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 <fcntl.h>
> #include <errno.h>
> #include <ctype.h>
>+#include <pwd.h>
> #include <sys/wait.h>
> #include <sys/types.h>
> #include <sys/stat.h>
>@@ -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);
>-- 
>1.7.10.4
>

Actions: View | Diff

Attachments on bug 864515: 624105 | 624699 | 624821