Attachment 624699 Details for Bug 864515 – Updated patch from Vincent Untz

[patch] Updated patch from Vincent Untz

cups-pk-helper-permission-flaw.patch (text/plain), 17.16 KB, created by Jan Lieskovsky on 2012-10-10 09:19:00 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jan Lieskovsky

Created: 2012-10-10 09:19:00 UTC

Size: 17.16 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 1/2] 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
>
>
>From f953594b7d32325857cc189db12c8d6fb4ca26e5 Mon Sep 17 00:00:00 2001
>From: Vincent Untz <vuntz@suse.com>
>Date: Wed, 10 Oct 2012 09:54:18 +0200
>Subject: [PATCH 2/2] Also change supplementary groups when changing effective
> uid/gid
>
>Thanks to Alexander Peslyak <solar@openwall.com> and Sebastian Krahmer
><krahmer@suse.de> for catching this.
>---
> src/cups.c |   49 +++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 43 insertions(+), 6 deletions(-)
>
>diff --git a/src/cups.c b/src/cups.c
>index 6ff6cba..e3f0143 100644
>--- a/src/cups.c
>+++ b/src/cups.c
>@@ -28,6 +28,7 @@
> #include <fcntl.h>
> #include <errno.h>
> #include <ctype.h>
>+#include <grp.h>
> #include <pwd.h>
> #include <sys/wait.h>
> #include <sys/types.h>
>@@ -512,20 +513,44 @@ _CPH_CUPS_IS_VALID (filename, "filename", TRUE, CPH_STR_MAXLEN)
>  ******************************************************/
> 
> static gboolean
>-_cph_cups_set_effective_id (unsigned int sender_uid)
>+_cph_cups_set_effective_id (unsigned int  sender_uid,
>+                            int          *saved_ngroups,
>+                            gid_t        *saved_groups,
>+                            int           saved_groups_len)
> {
>         struct passwd *password_entry;
> 
>+        /* avoid g_assert() because we don't want to crash here */
>+        if (saved_ngroups == NULL || saved_groups == NULL) {
>+                g_critical ("Internal error: cannot save supplementary groups.");
>+                return FALSE;
>+        }
>+
>+        *saved_ngroups = getgroups (saved_groups_len, saved_groups);
>+        if (*saved_ngroups < 0)
>+                return FALSE;
>+
>         password_entry = getpwuid ((uid_t) sender_uid);
> 
>         if (password_entry == NULL ||
>             setegid (password_entry->pw_gid) != 0)
>                 return FALSE;
> 
>+        if (initgroups (password_entry->pw_name,
>+                        password_entry->pw_gid) != 0) {
>+                if (getgid () != getegid ())
>+                        setegid (getgid ());
>+
>+                return FALSE;
>+        }
>+
>+
>         if (seteuid (sender_uid) != 0) {
>                 if (getgid () != getegid ())
>                         setegid (getgid ());
> 
>+                setgroups (*saved_ngroups, saved_groups);
>+
>                 return FALSE;
>         }
> 
>@@ -533,10 +558,13 @@ _cph_cups_set_effective_id (unsigned int sender_uid)
> }
> 
> static void
>-_cph_cups_reset_effective_id (void)
>+_cph_cups_reset_effective_id (int    saved_ngroups,
>+                              gid_t *saved_groups)
> {
>         seteuid (getuid ());
>         setegid (getgid ());
>+        if (saved_ngroups >= 0)
>+                setgroups (saved_ngroups, saved_groups);
> }
> 
> static void
>@@ -1115,11 +1143,14 @@ cph_cups_file_get (CphCups      *cups,
>                    const char   *filename,
>                    unsigned int  sender_uid)
> {
>+        int           saved_ngroups = -1;
>+        gid_t         saved_groups[NGROUPS_MAX];
>         http_status_t status;
>         int           fd;
>         struct stat   file_stat;
>         char         *error;
> 
>+
>         g_return_val_if_fail (CPH_IS_CUPS (cups), FALSE);
> 
>         if (!_cph_cups_is_resource_valid (cups, resource))
>@@ -1127,7 +1158,9 @@ cph_cups_file_get (CphCups      *cups,
>         if (!_cph_cups_is_filename_valid (cups, filename))
>                 return FALSE;
> 
>-        if (!_cph_cups_set_effective_id (sender_uid)) {
>+        if (!_cph_cups_set_effective_id (sender_uid,
>+                                         &saved_ngroups, saved_groups,
>+                                         sizeof (saved_groups))) {
>                 error = g_strdup_printf ("Cannot check if \"%s\" is "
>                                          "writable: %s",
>                                          filename, strerror (errno));
>@@ -1139,7 +1172,7 @@ cph_cups_file_get (CphCups      *cups,
> 
>         fd = open (filename, O_WRONLY | O_NOFOLLOW | O_TRUNC);
> 
>-        _cph_cups_reset_effective_id ();
>+        _cph_cups_reset_effective_id (saved_ngroups, saved_groups);
> 
>         if (fd < 0) {
>                 error = g_strdup_printf ("Cannot open \"%s\": %s",
>@@ -1200,6 +1233,8 @@ cph_cups_file_put (CphCups      *cups,
>                    const char   *filename,
>                    unsigned int  sender_uid)
> {
>+        int           saved_ngroups = -1;
>+        gid_t         saved_groups[NGROUPS_MAX];
>         http_status_t status;
>         int           fd;
>         struct stat   file_stat;
>@@ -1212,7 +1247,9 @@ cph_cups_file_put (CphCups      *cups,
>         if (!_cph_cups_is_filename_valid (cups, filename))
>                 return FALSE;
> 
>-        if (!_cph_cups_set_effective_id (sender_uid)) {
>+        if (!_cph_cups_set_effective_id (sender_uid,
>+                                         &saved_ngroups, saved_groups,
>+                                         sizeof (saved_groups))) {
>                 error = g_strdup_printf ("Cannot check if \"%s\" is "
>                                          "readable: %s",
>                                          filename, strerror (errno));
>@@ -1224,7 +1261,7 @@ cph_cups_file_put (CphCups      *cups,
> 
>         fd = open (filename, O_RDONLY);
> 
>-        _cph_cups_reset_effective_id ();
>+        _cph_cups_reset_effective_id (saved_ngroups, saved_groups);
> 
>         if (fd < 0) {
>                 error = g_strdup_printf ("Cannot open \"%s\": %s",
>-- 
>1.7.10.4
>

Actions: View | Diff

Attachments on bug 864515: 624105 | 624699 | 624821