On Wed, Nov 5, 2014 at 6:34 AM, Daniel Mack <[email protected]> wrote:
> On 10/29/2014 11:19 PM, Andy Lutomirski wrote:
>> I think that each piece of trustable metadata needs to be explicitly
>> opted-in to by the sender at the time of capture. Otherwise you're
>> asking for lots of information leaks and privilege escalations. This
>> is especially important given that some of the items in the current
>> list could be rather sensitive.
>
> Alright, the above seems to pretty much sum up that end of our
> discussion. To address this, We've now added the following functionality
> for v2:
>
> * The attach_flags in kdbus_cmd_hello was split into two parts,
> attach_flags_send and attach_flags_recv, so each peer may chose what
> exactly it want to transmit or receive.
>
> * Metadata will only be attached to the final message in the
> receiver's pool if both the sender's attach_flags_send and the
> receiver's attach_flags_recv bit are set.
>
> * Consequently, the existing KDBUS_ITEM_ATTACH_FLAGS item type is
> split into KDBUS_ITEM_ATTACH_FLAGS_SEND and
> KDBUS_ITEM_ATTACH_FLAGS_RECV, so that both connection details can be
> separately updated through KDBUS_CMD_CONN_UPDATE.
>
> * To allow for use cases that require certain metadata to be attached
> on each message, we've added a negotiation mechanism to the HELLO
> ioctl: An optional metadata mask can be passed during the creation
> of buses, so bus owners may require certain bits in
> attach_flags_send to be set. That way, the creator of the bus will
> specify which metadata is required to fulfill the requirements of
> the specification of the role of the bus.
>
>
Thanks!
Here are some early thoughts based on reading what I think is the v2
documentation online.
My general impression is that the semantics of and use cases for
passing fds around (including through execve) are not well thought
out. I think that the kdbus designers need to decide whether kdbus
fds will ever be passed between processes (via execve, SCM_RIGHTS, or
kdbus). If yes, there are several issues:
The docs for metadata and namespaces suggest that passing fds between
namespaces turns off metadata. I don't want to reopen this particular
discussion until Eric is back, and I don't think that the code matches
the docs, but to me this suggests that no one has really considered
how this case is supposed to behave.
I think that KDBUS_MESSAGE_SEND needs to specify metadata items to
send. Otherwise you'll introduce security problems the first time you
ever have a process that accepts a kdbus fd from a process that it
should not fully trust. (Note that this is not just my overly
paranoid imagination. I've found real security bugs in netlink and
procfs based on this. And SCM_CREDENTIALS is broken for similar
reasons.) Also, the userspace libraries will need to be rather
careful about setting those bits.
I don't see a mechanism to prevent resource exhaustion issues in which
many fds are recursively shoved into kdbus. (You still may need to
address this to some extent if you disallow the transfer of kdbus fds,
but it's probably easier to handle.)
If no:
KDBUS_MESSAGE_SEND should not send metadata items at all, because they
will never be interesting.
What privilege do you need to create a custom endpoint? What
privilege do you need to set its policy? Why do custom endpoints have
names at all?
Metadata:
Please justify each and every metadata with an actual use case or
remove it. The justification should include an explanation of why the
same thing can't be achieved with policy.
Off the top of my head:
- KDBUS_ATTACH_TIMESTAMP is fine.
- KDBUS_ATTACH_CREDS should be just uid and gid
- I tend to think that pid and tid should be separate. They're
really their own thing, and, as noted in all the perfectly valid
dislike directed at SO_PEERCRED, they have extremely limited value.
- starttime should have a justification or be removed.
- KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
feel like it's only useful for implementing strange types of policies.
- KDBUS_ATTACH_COMM, KDBUS_ATTACH_CMDLINE, and KDBUS_ATTACH_EXE: Just
remove them. There is no point whatsoever in having the kernel try to
validate comm and cmdline, and exe is barely better [1]. You realize
that there's this thing called PR_SET_NAME, right? Removing these may
benefit your arguments about the whole metadata mechanism, too --
these three items are wonderful straw men to poke at, because any
concept of trusting them is absurd even ignoring the framework in
which they're used. But you can't tell people to shut up and stop
attacking straw men, because you've actually proposed these particular
straw men. :)
- KDBUS_ATTACH_CGROUP: This sounds legitimately useful.
- KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
even mean on a non-selinux system? I tend to think that this
shouldn't exist as a metadata item and that all selinux integration
should go through the LSM hooks. Otherwise we'll end up with two
separate selinux policy databases -- the normal one and whatever dbus
tries to do, and the result will be even more unwieldy than the
current state of affairs. Also, turning off selinux enforcing mode
really ought to continue turning it all the way off, and I don't think
that dbus should be allowed to interfere with that.
- KDBUS_ATTACH_AUDIT: Please define what an audit label is. I'm
serious: audit doesn't have a great track record for interface
stability. Please consider disallowing anyone without
CAP_AUDIT_CONTROL from *receiving* this item. (Also, the docs here
are full of typos.)
Are caps missing from this list? I swear they're in the code but not
in either version of the docs... If they're still in the code, I
really don't like it. The Linux capability system sucks, and to avoid
having that suckage spread, those bits should IMO not gain any new
powers like this. One of the great things about polkit and dbus is
that you don't *need* caps to ask system software to do things for
you. This means that you can, for example, have the ability to ask
for a graceful reboot but not to call reboot(2). This is a good
thing, and actually using caps as a metadata item risks undoing that.
(Also, keep in mind that actually granting caps to uid != 0 processes
is a royal PITA, and every attempt to fix it gets shot down.
Furthermore, since I fully expect Eric to eventually nak you hard
enough that you get kdbus namespacing to work well, I think you'll
find that namespacing caps will be even less pleasant that the rest of
it.)
[1] I say that exe is barely better than comm because of two factors.
The first is ptrace. The second is that, if you fix namespacing,
you'll have a high probability of getting mostly-attacker-controlled
garbage out of it. (It will be 100% garbage, and attackers will have
lots of influence on that garbage.)
--Andy
On 05/11/14 15:56, Andy Lutomirski wrote:
> - I tend to think that pid and tid should be separate. They're
> really their own thing, and, as noted in all the perfectly valid
> dislike directed at SO_PEERCRED, they have extremely limited value.
Traditional D-Bus has GetConnectionUnixProcessID(), which is used by
several applications:
<http://codesearch.debian.net/search?q=GetConnectionUnixProcessID>,
<http://codesearch.debian.net/search?q=servicePid> (the latter is the Qt
binding).
I don't know what those applications use it for, or whether they're
doing it safely. CVE-2013-4288, CVE-2014-5033 seem potentially relevant.
In the same way that kernel people don't want to break userland, I don't
want to break existing D-Bus users; it would be a shame if kdbus omits
things that would let it replace traditional D-Bus.
> - starttime should have a justification or be removed.
I think its justification is "detect pid reuse", although AIUI it
doesn't detect pid "reuse" via exec().
> - KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
> feel like it's only useful for implementing strange types of policies.
I think the intention is that it enables things like "processes with
group netdev may tell NetworkManager to reconfigure networking".
Traditional D-Bus half-supports this, but interacts poorly with things
like pam_groups that assign groups to processes, not uids.
> - KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
> even mean on a non-selinux system?
As far as I understand it, sockets have a generic mechanism for storing
one arbitrary security label alongside the uid, and the active LSM gets
to define its syntax and what it means. This is the equivalent of that.
Traditional D-Bus has GetConnectionSELinuxSecurityContext() which only
returns the SELinux context, and never any other LSM's
context/label/profile/whatever.
<http://codesearch.debian.net/search?q=GetConnectionSELinuxSecurityContext>
suggests that it has users; I don't know what they do with it, or
whether they're correct.
The intention was that each LSM with code in dbus-daemon will eventually
contribute a key/value pair to GetConnectionCredentials() rather than
having their own separate methods, with
GetConnectionSELinuxSecurityContext() deprecated, but that's blocked by
people who understand the LSMs contributing the necessary code and
documentation. (Incidentally, if anyone reading this can contribute
proper documentation of the SELinux context to the D-Bus Specification -
what is the preferred jargon term? what do a couple of typical values
look like? is it restricted to some limited character set? etc. - I
would be grateful for a patch.)
Of course, if kdbus ends up being what everyone uses for D-Bus on Linux,
then there will no longer be much point in adding Linux-specific
features to dbus-daemon.
> Otherwise we'll end up with two
> separate selinux policy databases -- the normal one and whatever dbus
> tries to do
Traditional D-Bus already has this problem: dbus-daemon has to work out
"what would SELinux do?" in userland, including the decision whether to
enforce or just complain, and do the same. My understanding is that one
of the more minor upsides of doing (this part of) D-Bus in the kernel is
that it would remove that intermediary, moving the security decisions to
a location where LSMs can allow/deny things directly.
S
On Wed, Nov 5, 2014 at 9:02 AM, Simon McVittie
<[email protected]> wrote:
> On 05/11/14 15:56, Andy Lutomirski wrote:
>> - I tend to think that pid and tid should be separate. They're
>> really their own thing, and, as noted in all the perfectly valid
>> dislike directed at SO_PEERCRED, they have extremely limited value.
>
> Traditional D-Bus has GetConnectionUnixProcessID(), which is used by
> several applications:
> <http://codesearch.debian.net/search?q=GetConnectionUnixProcessID>,
> <http://codesearch.debian.net/search?q=servicePid> (the latter is the Qt
> binding).
>
> I don't know what those applications use it for, or whether they're
> doing it safely. CVE-2013-4288, CVE-2014-5033 seem potentially relevant.
>
> In the same way that kernel people don't want to break userland, I don't
> want to break existing D-Bus users; it would be a shame if kdbus omits
> things that would let it replace traditional D-Bus.
That's why I suggested separating it, not removing it. If it were
separate, then short-lived processes (like dbus-send, perhaps, in a
mode where it doesn't wait for a reply) would just skip sending it,
because the recipient can't use it in a reliable manner.
Although, TBH, it might be better to just return -1 from
GetConnectionUnixProcessID when kdbus is being used, since anything
that breaks might be breaking because it's vulnerable...
>
>> - starttime should have a justification or be removed.
>
> I think its justification is "detect pid reuse", although AIUI it
> doesn't detect pid "reuse" via exec().
Ugh. I really don't think the kernel should add a new feature to make
a broken, racy concept slightly less racy. I'm fully in favor of
making it completely non-racy, but this isn't the way to do it.
(Also, anything that actually relies on this is unlikely to survive
checkpoint/restore.)
>
>> - KDBUS_ATTACH_AUXGROUPS: I'm not sure what to think about this. I
>> feel like it's only useful for implementing strange types of policies.
>
> I think the intention is that it enables things like "processes with
> group netdev may tell NetworkManager to reconfigure networking".
> Traditional D-Bus half-supports this, but interacts poorly with things
> like pam_groups that assign groups to processes, not uids.
That's what I guessed, too. I'm not sure whether I like it.
>
>> - KDBUS_ATTACH_SECLABEL: The docs talk about selinux. What does this
>> even mean on a non-selinux system?
>
> As far as I understand it, sockets have a generic mechanism for storing
> one arbitrary security label alongside the uid, and the active LSM gets
> to define its syntax and what it means. This is the equivalent of that.
The UNIX socket credential mechanism is not a good role model, and
"something else does this too" is not a sufficient justification for
new code. I want to make sure that someone actually understands what
this thing does and whether it's a good idea.
Unfortunately, I suspect that most or all kdbus developers either
don't use LSMs at all or use selinux, which means that may not be
anyone who has considered the big picture here.
[...]
>
>> Otherwise we'll end up with two
>> separate selinux policy databases -- the normal one and whatever dbus
>> tries to do
>
> Traditional D-Bus already has this problem: dbus-daemon has to work out
> "what would SELinux do?" in userland, including the decision whether to
> enforce or just complain, and do the same. My understanding is that one
> of the more minor upsides of doing (this part of) D-Bus in the kernel is
> that it would remove that intermediary, moving the security decisions to
> a location where LSMs can allow/deny things directly.
I absolutely agree. It seems that the logical conclusion is that the
security label should *not* be passed to userspace, then -- once LSMs
can make decisions directly, userspace should let them make those
decisions and consider only the result of the decision, not the labels
that influenced the decision.
--Andy