Bug 864613 - Authorization list comes out in wrong order in PolicyKit local authority
Authorization list comes out in wrong order in PolicyKit local authority
Status: CLOSED ERRATA
Product: Red Hat Enterprise Linux 6
Classification: Red Hat
Component: polkit (Show other bugs)
6.3
Unspecified Unspecified
medium Severity medium
: rc
: ---
Assigned To: Miloslav Trmač
Martin Žember
:
Depends On: 951756
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-09 14:32 EDT by jared jennings
Modified: 2015-07-13 00:14 EDT (History)
5 users (show)

See Also:
Fixed In Version: polkit-0.96-6.el6
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
: 951756 (view as bug list)
Environment:
Last Closed: 2014-10-14 03:27:39 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
add debugging statements to clarify behavior of polkit_backend_local_authorization_store_ensure function (3.33 KB, patch)
2012-10-09 14:32 EDT, jared jennings
no flags Details | Diff
reverse authorization list only after it is entirely built (759 bytes, patch)
2012-10-09 16:25 EDT, jared jennings
no flags Details | Diff
sort filenames before iterating over them (1.04 KB, patch)
2012-10-09 16:27 EDT, jared jennings
no flags Details | Diff
0001-Sort-filenames-before-iterating-over-them.patch applied in polkit-pkla-compat upstream (2.03 KB, patch)
2013-05-07 14:27 EDT, Miloslav Trmač
no flags Details | Diff

  None (edit)
Description jared jennings 2012-10-09 14:32:24 EDT
Created attachment 624219 [details]
add debugging statements to clarify behavior of polkit_backend_local_authorization_store_ensure function

Description of problem:
While trying to allow udisks mounts for one unix-group and deny for everyone else (see someone else's Bug #812684), I found that the order of authorizations is unpredictable, for two reasons: (a) because the list of authorizations read is reversed every time an entry is added; and (b) it doesn't appear that the list of PolicyKit local authority files in a given directory is sorted by name, so the files may be read in any order.

Version-Release number of selected component (if applicable):
polkit-0.96.2.el6.1


How reproducible:
1. Apply the attached add-debug patch to the polkit sources. It adds more g_debug calls that make it clear what is happening. Build polkit the same way the official .spec file does.

2. Place in /etc/polkit-1/localauthority/90-mandatory.d at least four PolicyKit config files. For example, I've got one to cause all NetworkManager actions to require admin_auth, one to cause all PackageKit actions to require admin_auth, one for udisks to require admin_auth, and another for udisks where Identity=unix-group:my-special-group that will allow mount and unmount actions. You need at least four, and they should not be created in lexical sort order: i.e., the output of ls -f should not equal the output of ls.

3. Run polkitd with --debug.

4. As a normal user, plug in a USB disk and try to mount it with udisks --mount. Watch the debug output of polkitd.

5. The debug messages shown as each file is read should make it clear that the files are not being processed in sorted order.

6. The debug messages should also make clear that every time a value is prepended to the list of authorizations, the list is reversed.

As a concrete example, for four files FA FD FB FC, each containing a heading (INI file section) A D B C respectively, I believe the intended behavior is that the files should be read in sorted order, FA FB FC FD. The section in each is prepended to the list of authorization entries. With no reversals, the resultant list would be in the order D C B A. The list is reversed, to yield A B C D, and authorizations are checked in this order. But the files are read in directory order, FA FD FB FC, and each time an authorization entry is prepended the list is reversed: (<> means "reverses to")

A <> A
D A <> A D
B A D <> D A B
C D A B <> B A D C

and the final order in which the authorization entries are processed is B A D C.

The reverse is in src/polkitbackend/polkitbackendlocalauthorizationstore.c, function polkit_backend_local_authorization_store_ensure, at line 641 in the patched source, 636 in the unpatched source.

* * *

To fix: 
(a) Move the call to g_list_reverse outside the for loop it is in.
(b) Sort the list of files before iterating over it.
Comment 2 jared jennings 2012-10-09 16:25:06 EDT
Created attachment 624255 [details]
reverse authorization list only after it is entirely built
Comment 3 jared jennings 2012-10-09 16:27:37 EDT
Created attachment 624256 [details]
sort filenames before iterating over them

I cribbed the sort function and its usage from one of the other source files in the same directory.
Comment 4 jared jennings 2012-10-09 16:29:18 EDT
I looked at http://cgit.freedesktop.org/polkit/tree/src/polkitbackend and it seems the upstream PolicyKit is now very different; I'm not sure these patches mean anything there.
Comment 5 jared jennings 2012-10-24 16:54:09 EDT
This looks like the same issue as https://bugs.freedesktop.org/show_bug.cgi?id=26131
Comment 6 jared jennings 2012-10-24 17:13:12 EDT
D'oh! I wrote the comment on the wrong ticket! Should have been in Bug 812684, sorry.
Comment 8 jared jennings 2013-03-14 13:36:51 EDT
Comment on attachment 624219 [details]
add debugging statements to clarify behavior of polkit_backend_local_authorization_store_ensure function

Noticed that the debugging statements patch attachment was not marked as a patch; marked it
Comment 9 Miloslav Trmač 2013-04-12 22:01:09 EDT
Thanks for your report.  My reading of pklocalauthority(8) is that it defines the ordering of subdirectories, but not the ordering of files within a directory, so this is not strictly speaking contrary to documentation.

However, it still makes good sense to make the ordering consistent to avoid surprisingly different behavior when the configuration is e.g. restored from a backup.

(Reading the patch without testing it, compare_filename_reverse seems to use a non-reversed order.)
Comment 10 Miloslav Trmač 2013-05-07 14:27:28 EDT
Created attachment 744858 [details]
0001-Sort-filenames-before-iterating-over-them.patch applied in polkit-pkla-compat upstream

(In reply to comment #9)
> (Reading the patch without testing it, compare_filename_reverse seems to use
> a non-reversed order.)

This updated patch avoids this confusion, and explicitly talks about basenames because that's what we are interested in.
Comment 11 RHEL Product and Program Management 2013-10-13 20:23:26 EDT
This request was evaluated by Red Hat Product Management for
inclusion in the current release of Red Hat Enterprise Linux.
Because the affected component is not scheduled to be updated
in the current release, Red Hat is unable to address this
request at this time.

Red Hat invites you to ask your support representative to
propose this request, if appropriate, in the next release of
Red Hat Enterprise Linux.
Comment 18 errata-xmlrpc 2014-10-14 03:27:39 EDT
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHBA-2014-1533.html

Note You need to log in before you can comment on or make changes to this bug.