linux-kernel - Re: kdbus: add code to gather metadata

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5451F242.4070106@zonque.org>
Date:	Thu, 30 Oct 2014 09:09:38 +0100
From:	Daniel Mack <daniel@...que.org>
To:	Andy Lutomirski <luto@...capital.net>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	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>,
	simon.mcvittie@...labora.co.uk, alban.crequy@...labora.co.uk,
	javier.martinez@...labora.co.uk, Tom Gundersen <teg@...m.no>
Subject: Re: kdbus: add code to gather metadata

On 10/29/2014 11:33 PM, Andy Lutomirski wrote:
> On Wed, Oct 29, 2014 at 3:00 PM, Greg Kroah-Hartman

>> +/**
>> + * kdbus_meta_new() - create new metadata object
>> + * @meta:              New metadata object
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int kdbus_meta_new(struct kdbus_meta **meta)
>> +{
>> +       struct kdbus_meta *m;
>> +
>> +       BUG_ON(*meta);
>> +
>> +       m = kzalloc(sizeof(*m), GFP_KERNEL);
>> +       if (!m)
>> +               return -ENOMEM;
>> +
>> +       /*
>> +        * Remember the PID and user namespaces our credentials belong to;
>> +        * we need to prevent leaking authorization and security-relevant
>> +        * data across different namespaces.
>> +        */
>> +       m->pid_namespace = get_pid_ns(task_active_pid_ns(current));
>> +       m->user_namespace = get_user_ns(current_user_ns());
>> +
> 
> This is unusual, and it could be very expensive (it will serialize
> essentially everyone on an exclusive cacheline).  What attack is it
> protecting against?

As mentioned before, we currently prevent metadata from crossing over
user and pid namespace boundaries. In order to detect such situations,
we need to pin the namespaces of the the task creating such a metadata
object, so we can compare them later, even when the original task is not
alive anymore. But I'm open for cheaper solutions for this, as I'm
admittedly not an expert in these APIs.

>> +static int kdbus_meta_append_cred(struct kdbus_meta *meta)
>> +{
>> +       struct kdbus_creds creds = {
>> +               .uid = from_kuid_munged(current_user_ns(), current_uid()),
>> +               .gid = from_kgid_munged(current_user_ns(), current_gid()),
>> +               .pid = task_pid_vnr(current),
>> +               .tid = task_tgid_vnr(current),
>> +               .starttime = current->start_time,
>> +       };
>> +
>> +       return kdbus_meta_append_data(meta, KDBUS_ITEM_CREDS,
>> +                                     &creds, sizeof(creds));
>> +}
> 
> This seems wrong to me.  Shouldn't this store kuid_t, etc. directly?

The metadata item's memory that is appended here is directly copied into
the final message in the receiver's pool later, so the information has
to be authoritative and translated at this point. This is currently not
a problem as in cases where we cross namespaces, the metadata will not
be added to the final message anyway.

But you're right, if we support translation between namespaces later, we
need to store the kuid_t here, and patch in the the translated version
later, when the message is installed by the receiving peer (which is
when we know which namespace to translate the kuid_t for).

> Also, why pid, tid, and starttime?

Because pid is also part of struct ucred, and starttime seemed to fit in
here as well. After all, an item has some overhead with its header, so
we tried to group information that will most probably be needed
together. Any strong reason not to store it here?

Thanks,
Daniel

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