Re: [GIT PULL] kdbus for 4.1-rc1

[Andy Lutomirski]

On Mon, Apr 13, 2015 at 1:45 PM, Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Apr 13, 2015 at 01:13:26PM -0700, Andy Lutomirski wrote:
>> On Mon, Apr 13, 2015 at 12:03 PM, Greg Kroah-Hartman
>> <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>> > The following changes since commit 9eccca0843205f87c00404b663188b88eb248051:
>> >
>> >   Linux 4.0-rc3 (2015-03-08 16:09:09 -0700)
>> >
>> > are available in the git repository at:
>> >
>> >   git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/ tags/kdbus-4.1-rc1
>> >
>> > for you to fetch changes up to 9fb9cd0f4434a23487b6ef3237e733afae90e336:
>> >
>> >   kdbus: avoid the use of struct timespec (2015-04-10 14:34:53 +0200)
>> >
>> > ----------------------------------------------------------------
>> > kdbus for 4.1-rc1
>> >
>> > Here's the kdbus pull request for 4.1-rc1.
>> >
>> > It's been under development for many years now, and been in linux-next
>> > for many months, and has undergone loads of testing a review and even a few
>> > good arguments.  It comes with full documentation and tests.
>> >
>> > There has been a few complaints about the code, notably from people who
>> > don't like the use of metadata in the bus messages.  That is actually
>> > one of the main features here, as we can get this data in a secure and
>> > reliable way, and it's something that userspace requires today.  So
>> > while it does look "odd" to people who are not familiar with dbus, this
>> > is something that finally fixes a number of almost unfixable races in
>> > the current dbus implementations.
>>
>> While I generally like the concept of having a better in-kernel IPC
>> mechanism, after some consideration I don't think this belongs in the
>> kernel in its current form.  Here's why.
>>
>> First, the naming is counterintuitive.  There are "endpoints", but you
>> don't send messages to endpoints.  In fact, an basic kdbus setup will
>> have exactly one endpoint AFAICT.  Wtf?  This makes talking about it
>> awkward.
>
> Did you read the documentation?  We've been over this before, and it
> should all be addressed in the documentation based on this coming up.
>
>> A lot of the design seems to be to violate the concept of "mechanism,
>> not policy".  Kdbus is very much a port of userspace dbus to the
>> kernel, and it appears to be a port designed to preserve some
>> questionable design decisions instead of learning from them.
>>
>> For example, kdbus sticks a whole policy database in the kernel, but
>> that policy database (AFAICT -- holy crap it's overcomplicated) is
>> *not* a simple set of rules like "if A then allow B".  Instead it has
>> really weird dependencies not on what name you're sending to but on
>> what *other* names the thing you're sending to has.  Sorry, but this
>> way lies (a) the inability for a large set of developers to understand
>> what's going on and (b) security bugs.  Also, the result probably
>> can't be reused as part of a non-legacy-filled sensible design
>
> What policy database?  Matching messages to subscribers?  That's the
> same type of "database" that other ipc subsystems need/want, there's
> nothing radical here.

Let me quote from the latest version of the kdbus docs:

      Note that TALK access is checked against all names of a connection. For
      example, if a connection owns both <constant>'org.foo.bar'</constant> and
      <constant>'org.blah.baz'</constant>, and the policy database allows
      <constant>'org.blah.baz'</constant> to be talked to by WORLD, then this
      permission is also granted to <constant>'org.foo.bar'</constant>. That
      might sound illogical, but after all, we allow messages to be directed to
      either the ID or a well-known name, and policy is applied to the
      connection, not the name. In other words, the effective TALK policy for a
      connection is the most permissive of all names the connection owns.

In my humble opinion, this paragraph speaks for itself.  The design is
bad, full stop.

[...]

> And the benchmarks and source were posted by David previously, with full
> details, this is the first time I've heard you could not reproduce them
> using that code.

No it's not.  But I got bored and didn't try again.

>
>> The metadata thing is problematic.  It seems to be intended to serve
>> two purposes: data gathering for logging and authentication.
>> Unfortunately, it has issues.  There are no fewer than *three*
>> metadata capture points: creation of a bus, connection to a bus, and
>> sending of a message.  The kdbus authors like to point out that these
>> are all optional, but IMO that's bunk.  Someone will write a userspace
>> library that rejects messages from people who don't enable all of
>> them, then then we're screwed.
>
> Remember, you asked for it to be optional, it wasn't in the beginning :)
>
> So let's make it not optional, great.  And the capture points are in
> different places as it is different data and entry points.

Then I'll have to find a way to embolden my NACK further.  My point is
that capturing garbage like cmdline and capabilities (again, that
latter part is completely unacceptable under any circumstances
whatsoever) on behalf of *all* senders is a disaster.  If it's
optional, then I can at least hope that userspace will honor the
optionality and let everything turn it off.  If it's mandatory, then
kdbus is just unsafe to use to send messages to untrusted parties.

>
>> Why are we screwed?  Because any kdbus client *won't know which
>> metadata matters*.  That means that we automatically have the worst of
>> all worlds, not the best.  Also, the bus creation metadata is
>> completely worthless for anything other than logging, but someone will
>> use it for something other than logging, at which point it's
>> vulnerable to a DoS.  No one has explained to my satisfaction why this
>> isn't a problem.
>
> I don't think the creation data is worthless, I'm pretty sure the
> SELinux people are using it to validate things, but I could be wrong.
> Others on the cc: know more about that than I do and can provide
> details.

Does that code even exist in public form yet?

>
>> Also, the metadata code captures things that are, in my book
>> completely unacceptable, such as cmdline and (!) capabilities.  I bet
>> that the cmdline capture is extra special fscked up when cgroups and
>> such are in play because *it reads from the sender's VM*.  IOW it's
>> insecure and pointless.  (OK, it has a point: logging.  But I really
>> don't think that belongs in the kernel.)
>
> The sender's vm is what is wanted here.  And cmdline is something that
> userspace gets today, and does things with, as does SELinux, and
> auditing.  Same for capabilities, it's not insecure and pointless, it's
> the same thing that is provided to userspace, and userspace makes
> decisions on today, independent of kdbus/dbus.

Is there anything that userspace makes decisions on based on
capabilities?  If so, please tell me and I'll entertain myself by
writing exploits for them.

The fact that some existing userspace does awful things does *not*
justify adding new kernel mechanisms with which to repeat those
mistakes.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/