Crash authentication_agent_new with invalid object path in RegisterAuthenticationAgent

[Philip Withnall]

Hey,

On Wed, 2015-06-03 at 11:01 -0400, Colin Walters wrote:
> On Wed, Jun 3, 2015, at 09:40 AM, Philip Withnall wrote:
> > 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

Re-reviewed.

> > A few things:
> >  1. Please use spaces instead of tabs.
> 
> Oof.  I just pushed:
> http://cgit.freedesktop.org/polkit/commit/?id=3c01914300a2120c5f6ff27ecb2c40eaa63a4bb7

Heh.

> >  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

Fair enough.

> >  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.

Yeah, I realise it will be hard, but I thought I should mention it
anyway. Possibly the easiest way to do this is to bypass D-Bus entirely
and split the polkitd code out into a libtool helper library, with a
thin shim over it which does the D-Bus service glue. Then unit tests can
poke the helper library.

> > > 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.

Great, thanks.

Philip
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/polkit-devel/attachments/20150603/60fb9983/attachment.sig>