Bug 69538 - Post-CVE-2013-4288 cleanups
Summary: Post-CVE-2013-4288 cleanups
Status: RESOLVED FIXED
Alias: None
Product: PolicyKit
Classification: Unclassified
Component: libpolkit (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: David Zeuthen (not reading bugmail)
QA Contact: David Zeuthen (not reading bugmail)
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-09-18 17:24 UTC by Miloslav Trmac
Modified: 2013-11-12 00:02 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
0003-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch (6.01 KB, patch)
2013-09-18 17:24 UTC, Miloslav Trmac
Details | Splinter Review
0004-sessionmonitor-systemd-cleanup-Deduplicate-code-path.patch (3.56 KB, patch)
2013-09-18 17:24 UTC, Miloslav Trmac
Details | Splinter Review
0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch (7.13 KB, patch)
2013-11-07 22:28 UTC, Colin Walters
Details | Splinter Review
0002-sessionmonitor-systemd-Deduplicate-code-paths.patch (3.83 KB, patch)
2013-11-07 22:28 UTC, Colin Walters
Details | Splinter Review
0003-examples-cancel-Fix-to-securely-lookup-subject.patch (948 bytes, patch)
2013-11-07 22:28 UTC, Colin Walters
Details | Splinter Review
0004-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch (5.34 KB, patch)
2013-11-07 22:28 UTC, Colin Walters
Details | Splinter Review
0001-PolkitSystemBusName-Retrieve-both-pid-and-uid.patch (7.75 KB, patch)
2013-11-09 16:07 UTC, Colin Walters
Details | Splinter Review
0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch (2.31 KB, patch)
2013-11-09 19:08 UTC, Colin Walters
Details | Splinter Review
0002-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch (3.05 KB, patch)
2013-11-09 19:09 UTC, Colin Walters
Details | Splinter Review
0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch (2.34 KB, patch)
2013-11-11 14:09 UTC, Colin Walters
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Miloslav Trmac 2013-09-18 17:24:00 UTC
Created attachment 86078 [details] [review]
0003-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch

Still need to apply the following patches by Colin, and look into avoiding the newly deprecated polkit_unix_process_new* calls within the polkit codebase.
Comment 1 Miloslav Trmac 2013-09-18 17:24:19 UTC
Created attachment 86079 [details] [review]
0004-sessionmonitor-systemd-cleanup-Deduplicate-code-path.patch
Comment 2 Colin Walters 2013-11-07 22:26:36 UTC
Ok, I came back to this, and fixed up those two above patches, and I have two more.
Comment 3 Colin Walters 2013-11-07 22:28:02 UTC
Created attachment 88855 [details] [review]
0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch
Comment 4 Colin Walters 2013-11-07 22:28:15 UTC
Created attachment 88856 [details] [review]
0002-sessionmonitor-systemd-Deduplicate-code-paths.patch
Comment 5 Colin Walters 2013-11-07 22:28:26 UTC
Created attachment 88857 [details] [review]
0003-examples-cancel-Fix-to-securely-lookup-subject.patch
Comment 6 Colin Walters 2013-11-07 22:28:38 UTC
Created attachment 88858 [details] [review]
0004-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch
Comment 7 Miloslav Trmac 2013-11-09 06:58:34 UTC
Comment on attachment 88855 [details] [review]
0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch

Review of attachment 88855 [details] [review]:
-----------------------------------------------------------------

Looks good.
Comment 8 Miloslav Trmac 2013-11-09 06:58:47 UTC
Comment on attachment 88857 [details] [review]
0003-examples-cancel-Fix-to-securely-lookup-subject.patch

Review of attachment 88857 [details] [review]:
-----------------------------------------------------------------

OK.
Comment 9 Miloslav Trmac 2013-11-09 07:10:59 UTC
Comment on attachment 88858 [details] [review]
0004-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch

Review of attachment 88858 [details] [review]:
-----------------------------------------------------------------

::: src/polkit/polkitpermission.c
@@ +125,2 @@
>    if (permission->subject == NULL)
>      permission->subject = polkit_unix_process_new (getpid ());

In this case we know precisely the attributes of the process; wouldn't it be cleaner to call 

polkit_unix_process_new_for_owner (getpid(), 0 /* look up, which is safe */, getuid());

?  That would amount to the same thing, be marginally faster because it avoids _polkit_unix_process_get_owner(), and most importantly not suggest that the code is somehow problematic.

::: src/polkit/polkitsystembusname.c
@@ +393,2 @@
>    ret = polkit_unix_process_new (pid);
> +  G_GNUC_END_IGNORE_DEPRECATIONS

We actually can get the correct UID from D-Bus here; wouldn't it make sense to provide the correct information?  (The startup time would remain unreliable.)

OTOH at least the in-tree users only use the PID, so this would be adding one more lookup without any tangible benefit.

I'm not sure.

::: src/programs/pkcheck.c
@@ +397,3 @@
>            if (sscanf (argv[n], "%i,%" G_GUINT64_FORMAT ",%u", &pid, &pid_start_time, &uid) == 3)
>              {
>                subject = polkit_unix_process_new_for_owner (pid, pid_start_time, uid);

Being a pedant, I'd prefer the IGNORE_DEPRECATIONS marker only around the 2 polkit_unix_process_* calls instead of also including the sscanfs, and especially the one non-deprecated call.  It would also be consistent with the polkitsubject hunk above.

::: test/polkitbackend/test-polkitbackendjsauthority.c
@@ +78,3 @@
>    caller = polkit_unix_process_new (getpid ());
>    subject = polkit_unix_process_new (getpid ());
> +  G_GNUC_END_IGNORE_DEPRECATIONS

Here I'd also prefer calling the non-deprecated API instead.

@@ +346,3 @@
>    caller = polkit_unix_process_new (getpid ());
>    subject = polkit_unix_process_new (getpid ());
> +  G_GNUC_END_IGNORE_DEPRECATIONS

Here I'd also prefer calling the non-deprecated API instead.
Comment 10 Miloslav Trmac 2013-11-09 07:14:51 UTC
Comment on attachment 88856 [details] [review]
0002-sessionmonitor-systemd-Deduplicate-code-paths.patch

Review of attachment 88856 [details] [review]:
-----------------------------------------------------------------

Yes, the pid->session duplication removal is a good thing.

OTOH see comment on patch #4 - it might make sense polkit_system_bus_name_get_process_sync() to look up both PID and UID (I really don't know), and in that case the UID lookup is wasteful.

Perhaps we might end up with an internal-only polkit_system_bus_name_get_unix_pid() that avoids the UID lookup?  Or then again, it might not be worth worrying about.


A similar cleanup can be done in sessionmonitor.c, but I don't have an easy way to test it, so it's probably best left alone.
Comment 11 Colin Walters 2013-11-09 13:56:42 UTC
(In reply to comment #10)
>
> OTOH see comment on patch #4 - it might make sense
> polkit_system_bus_name_get_process_sync() to look up both PID and UID (I
> really don't know), and in that case the UID lookup is wasteful.

That's a very good point; I think we should do this even if it's not actually used internally, because it's clearly the correct thing to do, and who knows - maybe a later refactoring will make it important.  Or maybe it'll be useful for external users.

I think it'll be fine in terms of efficiency if we make two async calls, then block on them both returning.

https://bugs.freedesktop.org/show_bug.cgi?id=54445 is an in-progress bug to make exactly this more efficient.

If it's OK I'd like to do it as a separate patch on top, as it more clearly separates pure code deduplication from an improvement.
Comment 12 Colin Walters 2013-11-09 16:06:14 UTC
Comment on attachment 88855 [details] [review]
0001-PolkitSystemBusName-Add-public-API-to-retrieve-Unix-.patch

0001 committed.
Comment 13 Colin Walters 2013-11-09 16:06:27 UTC
Comment on attachment 88857 [details] [review]
0003-examples-cancel-Fix-to-securely-lookup-subject.patch

0003 committed.
Comment 14 Colin Walters 2013-11-09 16:07:57 UTC
Created attachment 88934 [details] [review]
0001-PolkitSystemBusName-Retrieve-both-pid-and-uid.patch

Additional patch to follow 0002, tested and works on top of gnome-continuous buildmaster.
Comment 15 Colin Walters 2013-11-09 19:08:50 UTC
Created attachment 88944 [details] [review]
0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch
Comment 16 Colin Walters 2013-11-09 19:09:03 UTC
Created attachment 88945 [details] [review]
0002-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch
Comment 17 Colin Walters 2013-11-09 19:10:35 UTC
(In reply to comment #9)

> In this case we know precisely the attributes of the process; wouldn't it be
> cleaner to call 
> 
> polkit_unix_process_new_for_owner (getpid(), 0 /* look up, which is safe */,
> getuid());

Indeed, I should have looked more carefully for calls which could be updated.  I split this into two patches - the updated calls, and the ones we must ignore.

Thanks for the careful review!
Comment 18 Colin Walters 2013-11-11 14:09:32 UTC
Created attachment 89028 [details] [review]
0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch

This one compiles, forgot to amend this commit before filing.
Comment 19 Miloslav Trmac 2013-11-11 23:14:40 UTC
Comment on attachment 88934 [details] [review]
0001-PolkitSystemBusName-Retrieve-both-pid-and-uid.patch

Review of attachment 88934 [details] [review]:
-----------------------------------------------------------------

::: src/polkit/polkitsystembusname.c
@@ +380,5 @@
> +	  g_assert (!data->retrieved_pid);
> +	  data->retrieved_pid = TRUE;
> +	  data->pid = value;
> +	}
> +    }

Is it actually documented that GDbus will call the callbacks in the same order as the requests have been issued?  I agree that it makes sense given the underlying implementation, still...  Perhaps I just worry too much.

@@ +406,5 @@
> +  g_main_context_push_thread_default (tmp_context);
> +
> +  /* Do two async calls as it's basically as fast as one sync call.
> +   */
> +  g_dbus_connection_call (connection, 

Adds trailing whitespace

@@ +418,5 @@
> +			  -1,
> +			  cancellable,
> +			  on_retrieved_unix_uid_pid,
> +			  &data);
> +  g_dbus_connection_call (connection, 

Adds trailing whitespace

@@ +467,4 @@
>                                           GCancellable         *cancellable,
>                                           GError              **error)
>  {
> +  PolkitSubject *ret = NULL;

"connection" can be removed entirely.

@@ +504,5 @@
>  				      GCancellable         *cancellable,
>  				      GError              **error)
>  {
> +  PolkitUnixUser *ret = NULL;
> +  GDBusConnection *connection = NULL;

"connection" can be removed entirely.

@@ +512,5 @@
>    g_return_val_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable), NULL);
>    g_return_val_if_fail (error == NULL || *error == NULL, NULL);
>  
> +  if (!polkit_system_bus_name_get_creds_sync (system_bus_name, &uid, NULL,
> +					      cancellable, error))

(There's no direct benefit to changin this, but this patch makes the code easier to adapt for bug 54445 (or to add caching), which makes sense as well.  So I'm fine with this, just highlighting.)
Comment 20 Miloslav Trmac 2013-11-11 23:15:28 UTC
Comment on attachment 88945 [details] [review]
0002-Use-G_GNUC_BEGIN_IGNORE_DEPRECATIONS-to-avoid-warnin.patch

Review of attachment 88945 [details] [review]:
-----------------------------------------------------------------

ACK, and thanks for the update.
Comment 21 Miloslav Trmac 2013-11-11 23:15:32 UTC
Comment on attachment 89028 [details] [review]
0001-Port-internals-non-deprecated-PolkitProcess-API-wher.patch

Review of attachment 89028 [details] [review]:
-----------------------------------------------------------------

ACK, and thanks for the update.
Comment 22 Colin Walters 2013-11-12 00:01:13 UTC
(In reply to comment #19)

> Is it actually documented that GDbus will call the callbacks in the same
> order as the requests have been issued?  I agree that it makes sense given
> the underlying implementation, still...  Perhaps I just worry too much.

Yep, I can't offhand find this as part of the spec, but see:
http://lists.freedesktop.org/archives/dbus/2009-November/011927.html

Fixed up for all of the other comments, thanks!


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.