Attachment 140419 Details for Bug 107085 – Suggested Patch

[patch] Suggested Patch

0001-user_change_icon_file_authorized_cb-fix-insufficient.patch (text/plain), 2.36 KB, created by Matthias Gerstner on 2018-07-02 10:46:57 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Matthias Gerstner

Created: 2018-07-02 10:46:57 UTC

Size: 2.36 KB

patch

obsolete

>From 380f4cd7b80176549e91795808d875c1e6e58ccb Mon Sep 17 00:00:00 2001
>From: Matthias Gerstner <matthias.gerstner@suse.de>
>Date: Mon, 2 Jul 2018 12:06:58 +0200
>Subject: [PATCH] user_change_icon_file_authorized_cb: fix insufficient path
> prefix check
>
>The path prefix check can be circumvented by regular users by passing
>relativ path component like so:
>
>$ dbus-send --system --print-reply --dest=org.freedesktop.Accounts \
>	/org/freedesktop/Accounts/User1000 \
>	org.freedesktop.Accounts.User.SetIconFile \
>	string:/usr/share/../../tmp/test
>
>This results in a user controlled path to be stored in the
>accountsservice. Clients of accountsservice may trust this path. The
>existing code actually invests quite some efforts to ensure this by
>copying the file away with dropped privileges.
>
>To fix this situation this commit canonicalized the input path for the
>prefix comparison.
>---
> src/user.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
>diff --git a/src/user.c b/src/user.c
>index c8a2942..e1b9a6f 100644
>--- a/src/user.c
>+++ b/src/user.c
>@@ -1306,6 +1306,7 @@ user_change_icon_file_authorized_cb (Daemon                *daemon,
> 
> {
>         g_autofree gchar *filename = NULL;
>+        g_autofree gchar *canon_filename = NULL;
>         g_autoptr(GFile) file = NULL;
>         g_autoptr(GFileInfo) info = NULL;
>         guint32 mode;
>@@ -1355,9 +1356,19 @@ user_change_icon_file_authorized_cb (Daemon                *daemon,
>                 return;
>         }
> 
>+        /* This will not resolve symlinks. But we only want to check for a
>+         * trusted prefix below, so this should be enough. The important bit
>+         * is that no user controlled path is stored in the user data. If
>+         * canon_filename is not trusted then filename will be copied into a
>+         * safe place and the path to the safe place will be stored in the
>+         * user data.
>+         */
>+        canon_filename = g_file_get_path(file);
>+
>         if ((mode & S_IROTH) == 0 ||
>-            (!g_str_has_prefix (filename, DATADIR) &&
>-             !g_str_has_prefix (filename, ICONDIR))) {
>+            !canon_filename ||
>+            (!g_str_has_prefix (canon_filename, DATADIR) &&
>+             !g_str_has_prefix (canon_filename, ICONDIR))) {
>                 g_autofree gchar *dest_path = NULL;
>                 g_autoptr(GFile) dest = NULL;
>                 const gchar *argv[3];
>-- 
>2.16.4
>

Actions: View | Splinter Review

Attachments on bug 107085: 140419

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.