[cups-pk-helper/f17] Fix a bunch of issues when getting/putting a file from cups
mkasik
mkasik at fedoraproject.org
Tue Oct 23 15:02:55 UTC 2012
commit 27e8ebc0078452778dd7b984663cf4887dcc213b
Author: Marek Kasik <mkasik at redhat.com>
Date: Tue Oct 23 17:01:51 2012 +0200
Fix a bunch of issues when getting/putting a file from cups
This fix changes behaviour of method FileGet, it needs to have the
target file created in advance (part of the security fix).
Resolves: #865815 (CVE-2012-4510)
...-of-issues-when-getting-putting-a-file-fr.patch | 350 ++++++++++++++++++++
...-supplementary-groups-when-changing-effec.patch | 172 ++++++++++
cups-pk-helper.spec | 12 +-
3 files changed, 533 insertions(+), 1 deletions(-)
---
diff --git a/0001-Fix-a-bunch-of-issues-when-getting-putting-a-file-fr.patch b/0001-Fix-a-bunch-of-issues-when-getting-putting-a-file-fr.patch
new file mode 100644
index 0000000..ecce316
--- /dev/null
+++ b/0001-Fix-a-bunch-of-issues-when-getting-putting-a-file-fr.patch
@@ -0,0 +1,350 @@
+From 6995d30816f0c76844dee4dc9022296a03258457 Mon Sep 17 00:00:00 2001
+From: Vincent Untz <vuntz at 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
+
+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 <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.12.1
+
diff --git a/0002-Also-change-supplementary-groups-when-changing-effec.patch b/0002-Also-change-supplementary-groups-when-changing-effec.patch
new file mode 100644
index 0000000..6b6c993
--- /dev/null
+++ b/0002-Also-change-supplementary-groups-when-changing-effec.patch
@@ -0,0 +1,172 @@
+From a397b90823af3e4e697b9118022aa88f91a80989 Mon Sep 17 00:00:00 2001
+From: Vincent Untz <vuntz at 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 at openwall.com> and Sebastian Krahmer
+<krahmer at suse.de> for catching this.
+
+Part of fix for CVE-2012-4510.
+---
+ src/cups.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++------
+ 1 file changed, 66 insertions(+), 7 deletions(-)
+
+diff --git a/src/cups.c b/src/cups.c
+index 6ff6cba..d45a315 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,31 +513,81 @@ _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)
+ {
+ struct passwd *password_entry;
++ int ngroups;
++ gid_t *groups;
++
++ /* 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 = -1;
++ *saved_groups = NULL;
++
++ ngroups = getgroups (0, NULL);
++ if (ngroups < 0)
++ return FALSE;
++
++ groups = g_new (gid_t, ngroups);
++ if (groups == NULL && ngroups > 0)
++ return FALSE;
++
++ if (getgroups (ngroups, groups) < 0) {
++ g_free (groups);
++
++ return FALSE;
++ }
+
+ password_entry = getpwuid ((uid_t) sender_uid);
+
+ if (password_entry == NULL ||
+- setegid (password_entry->pw_gid) != 0)
++ setegid (password_entry->pw_gid) != 0) {
++ g_free (groups);
++
++ return FALSE;
++ }
++
++ if (initgroups (password_entry->pw_name,
++ password_entry->pw_gid) != 0) {
++ if (getgid () != getegid ())
++ setegid (getgid ());
++
++ g_free (groups);
++
+ return FALSE;
++ }
++
+
+ if (seteuid (sender_uid) != 0) {
+ if (getgid () != getegid ())
+ setegid (getgid ());
+
++ setgroups (ngroups, groups);
++ g_free (groups);
++
+ return FALSE;
+ }
+
++ *saved_ngroups = ngroups;
++ *saved_groups = groups;
++
+ return TRUE;
+ }
+
+ 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,6 +1166,8 @@ cph_cups_file_get (CphCups *cups,
+ const char *filename,
+ unsigned int sender_uid)
+ {
++ int saved_ngroups = -1;
++ gid_t *saved_groups = NULL;
+ http_status_t status;
+ int fd;
+ struct stat file_stat;
+@@ -1127,7 +1180,8 @@ 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)) {
+ error = g_strdup_printf ("Cannot check if \"%s\" is "
+ "writable: %s",
+ filename, strerror (errno));
+@@ -1139,7 +1193,8 @@ 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);
++ g_free (saved_groups);
+
+ if (fd < 0) {
+ error = g_strdup_printf ("Cannot open \"%s\": %s",
+@@ -1200,6 +1255,8 @@ cph_cups_file_put (CphCups *cups,
+ const char *filename,
+ unsigned int sender_uid)
+ {
++ int saved_ngroups = -1;
++ gid_t *saved_groups = NULL;
+ http_status_t status;
+ int fd;
+ struct stat file_stat;
+@@ -1212,7 +1269,8 @@ 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)) {
+ error = g_strdup_printf ("Cannot check if \"%s\" is "
+ "readable: %s",
+ filename, strerror (errno));
+@@ -1224,7 +1282,8 @@ 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);
++ g_free (saved_groups);
+
+ if (fd < 0) {
+ error = g_strdup_printf ("Cannot open \"%s\": %s",
+--
+1.7.12.1
+
diff --git a/cups-pk-helper.spec b/cups-pk-helper.spec
index 9062a0f..21a3098 100644
--- a/cups-pk-helper.spec
+++ b/cups-pk-helper.spec
@@ -1,6 +1,6 @@
Name: cups-pk-helper
Version: 0.2.2
-Release: 1%{?dist}
+Release: 2%{?dist}
Summary: A helper that makes system-config-printer use PolicyKit
Group: System Environment/Base
@@ -9,6 +9,8 @@ URL: http://www.vuntz.net/download/cups-pk-helper/
Source0: http://cgit.freedesktop.org/cups-pk-helper/snapshot/cups-pk-helper-%{version}.tar.bz2
Patch0: polkit_result.patch
+Patch1: 0001-Fix-a-bunch-of-issues-when-getting-putting-a-file-fr.patch
+Patch2: 0002-Also-change-supplementary-groups-when-changing-effec.patch
BuildRequires: libtool >= 1.4.3
BuildRequires: cups-devel >= 1.2
@@ -38,6 +40,8 @@ interfaces available under control of PolicyKit.
%setup -q
%patch0 -p1 -b .polkit-result
+%patch1 -p1 -b .CVE-2012-4510
+%patch2 -p1 -b .CVE-2012-4510-2
%build
@@ -65,6 +69,12 @@ make install DESTDIR=$RPM_BUILD_ROOT
%changelog
+* Tue Oct 23 2012 Marek Kasik <mkasik at redhat.com> - 0.2.2-2
+- Fix a bunch of issues when getting/putting a file from cups
+- This fix changes behaviour of method FileGet, it needs to have the
+- target file created in advance (part of the security fix).
+- Resolves: #865815 (CVE-2012-4510)
+
* Thu Mar 22 2012 Marek Kasik <mkasik at redhat.com> - 0.2.2-1
- Update to 0.2.2
- Remove upstreamed patches
More information about the scm-commits
mailing list