Date:	Mon, 3 Nov 2014 09:05:14 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Simon McVittie <simon.mcvittie@...labora.co.uk>
Cc:	Daniel Mack <daniel@...que.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linux API <linux-api@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	John Stultz <john.stultz@...aro.org>,
	Arnd Bergmann <arnd@...db.de>, Tejun Heo <tj@...nel.org>,
	Marcel Holtmann <marcel@...tmann.org>,
	Ryan Lortie <desrt@...rt.ca>,
	Bastien Nocera <hadess@...ess.net>,
	David Herrmann <dh.herrmann@...il.com>,
	Djalal Harouni <tixxdz@...ndz.org>,
	"alban.crequy" <alban.crequy@...labora.co.uk>,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	Tom Gundersen <teg@...m.no>
Subject: Re: kdbus: add code to gather metadata

On Mon, Nov 3, 2014 at 4:00 AM, Simon McVittie
<simon.mcvittie@...labora.co.uk> wrote:
> On 01/11/14 16:19, Andy Lutomirski wrote:
>> You can't justify logging fundamentally unverifiable things like the
>> command line by saying that you want to know if someone tries to play
>> (impossible-to-reliably-detect) games to obscure their command line.
>
> I think kdbus might be mixing up two orthogonal things here.
>
> It has an easy, kernel-checked, race-free way to determine
> kernel-mediated credential-like information that cannot be faked or
> interfered with (uid, primary gid, other gids?, security label,
> capabilities) because these are usable for security decisions, but if
> they are *not* received in a kernel-checked, race-free way, then they
> are useless.
>
> One concrete example of using non-ucred credential-like information is
> that traditional D-Bus can only restrict sysadmin tasks to uid 0 (or a
> root-equivalent uid in group sudo/admin/whatever), whereas when systemd
> and systemd-logind are run on kdbus, many of their D-Bus methods require
> specific capabilities(7): KillUser requires CAP_KILL, PowerOff requires
> CAP_SYS_BOOT, and so on. If capabilities(7) are a good thing, then
> that's surely a good thing too. (On the other hand, if you think
> capabilities(7) are a waste of time, then so is this.)

I think that capabilities(7) is largely a disaster.  That aside, I
don't think that all of these capabilities should refer to the
*kernel* privileges.  For example, CAP_SYS_BOOT should be, and remain,
the ability to reboot the system *yourself*.  For example, if someone
wants to implement Windows 7 (or Visa?  Or 2003?  I forget.) style
reboot auditing, then userspace specifically does not want programs
that can ask for a reboot to hold CAP_SYS_BOOT.

>
> It also uses the same mechanism as an easy, race-free, but *not*
> kernel-checked way to determine bits and pieces that are valuable for
> debugging (dbus-monitor etc.), but unsuitable for security decisions,
> such as cmdline.
>
> In traditional D-Bus, you can get the uid and pid of a remote process,
> but in a debug log you would probably actually prefer to log the cmdline
> in addition; yes a malicious user could fake the cmdline, but when
> debugging a system problem, information that is known to be forgeable
> seems better than no information at all. After all, ps(1) shows the
> forgeable cmdline, not just the executable. You can get that by
> rummaging in /proc/$pid, but there is a race: if the remote process
> exits too soon (a "fire and forget" method call) then you'll never know
> who it was. kdbus solves that race, but does not make cmdline unforgeable.
>

I would love to fix this race.  That opens the door to a lot more
debugging (maps, status, etc) while not pretending to offer a kernel
check that doesn't, and can't, exist.

Materializing some sort of pid fd on the receiving end would be one
solution.  Another would be to give the receiver some token that can
be used to check for pid recycling.

Can we give each task a pid-namespace-local 64-bit (or even longer)
number that is mostly guaranteed not to be reused for the lifetime of
the namespace?

For example, give each task a per-pidns tid_unique, and give each
pidns a next_tid_unique.  Creating a task assigns it
next_tid_unique++.  (On overflow, clone fails.)  For CRIU's benefit,
if you have CAP_SYS_ADMIN over the pidns's userns and you are not in
the pidns, then you can change tid_unique and next_tid_unique.  Doing
that carelessly will introduce races, but that's fine -- it should
only be done on restore when everything's frozen.

> If client libraries wishing to attach their cmdline (or other debug
> info) to messages for debugging were required to add it as an
> out-of-band KDBUS_ITEM, or as a D-Bus message header inside the payload,
> then that would be duplicating work in client libraries that could have
> been done centrally, but would still solve the race.
>

I much prefer that approach.  If a kernel feature is being added to
just to avoid duplication of a debugging aid in user code, then let's
leave it to user code.

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

Powered by blists - more mailing lists