Crash authentication_agent_new with invalid object path in RegisterAuthenticationAgent

[Colin Walters]

Hi,

On Wed, Jun 3, 2015, at 09:40 AM, Philip Withnall wrote:
> Hi Colin,
> 
> On Sat, 2015-05-30 at 09:36 -0400, an unknown sender wrote:
> > On Fri, May 29, 2015, at 02:08 PM, Tavis Ormandy wrote:
> > > Hello, I've noticed polkitd dumps core if you set an invalid object
> > > path when calling RegisterAuthenticationAgent. It looks like this code
> > > doesn't check if error was set before dereferencing it:
> > 
> > Indeed, thanks for the report.  Can someone review this patch?
> 
> The approach looks sound to me.

Thanks for review!  I pushed an updated version to
https://bugs.freedesktop.org/show_bug.cgi?id=90829

> A few things:
>  1. Please use spaces instead of tabs.

Oof.  I just pushed:
http://cgit.freedesktop.org/polkit/commit/?id=3c01914300a2120c5f6ff27ecb2c40eaa63a4bb7

>  2. The test case doesn’t unref the GDBusConnection.
>  3. There’s no need for the ‘out’ label in the test case — just check if
> (reply != NULL) instead.

I tend to always write code using "out:" even if it's not necessary, because
1) Then the code is more consistent
2) If the function needs to later evolve to be less trivial, you don't have to
   rewrite all of the code flow

>  4. Would it be possible to plumb the test case into the tests/
> directory?

I'll investigate this, though because those are all unit tests, we can't
easily do anything end-to-end like talk to polkitd over DBus.

> > I suppose this'll need a CVE, as local, authenticated users can
> > can DoS polkitd.
> 
> Looks like it.

I talked to Red Hat Security Response, and they allocated
CVE-2015-3218 for this.

Can you do one more pass on the patch?  There's no real
changes other than the untab-ifying and updating the commit
message for the CVE, but we might as well
consistently do it in Bugzilla.

I'll look at a test case as a followup.