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
signature.asc
Description: This is a digitally signed message part
_______________________________________________ polkit-devel mailing list polkit-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/polkit-devel
