Greg Kroah-Hartman [off-list ref] writes:
The way capabilities are checked in this patch make me very nervous.
We are not checking permissions at open time. Every other location
of calling capable on file like objects has been show to be suceptible
to file descriptor pass attacks.
See Documentation/kdbus.txt for more details.
Signed-off-by: Daniel Mack <redacted>
Signed-off-by: Greg Kroah-Hartman <redacted>
---
quoted hunk
diff --git a/drivers/misc/kdbus/bus.c b/drivers/misc/kdbus/bus.c
new file mode 100644
index 000000000000..6dcaf22f5d59
--- /dev/null
+++ b/drivers/misc/kdbus/bus.c
+/**
+ * kdbus_bus_cred_is_privileged() - check whether the given credentials in
+ * combination with the capabilities of the
+ * current thead are privileged on the bus
+ * @bus: The bus to check
+ * @cred: The credentials to match
+ *
+ * Return: true if the credentials are privileged, otherwise false.
+ */
+bool kdbus_bus_cred_is_privileged(const struct kdbus_bus *bus,
+ const struct cred *cred)
+{
+ /* Capabilities are *ALWAYS* tested against the current thread, they're
+ * never remembered from conn-credentials. */
+ if (ns_capable(&init_user_ns, CAP_IPC_OWNER))
+ return true;
+
+ return uid_eq(bus->uid_owner, cred->fsuid);
+}
+
+/**
+ * kdbus_bus_uid_is_privileged() - check whether the current user is a
+ * priviledged bus user
+ * @bus: The bus to check
+ *
+ * Return: true if the current user has CAP_IPC_OWNER capabilities, or
+ * if it has the same UID as the user that created the bus. Otherwise,
+ * false is returned.
+ */
+bool kdbus_bus_uid_is_privileged(const struct kdbus_bus *bus)
+{
+ return kdbus_bus_cred_is_privileged(bus, current_cred());
+}
+/**
+ * kdbus_bus_new() - create a new bus
+ * @domain: The domain to work on
+ * @make: Pointer to a struct kdbus_cmd_make containing the
+ * details for the bus creation
+ * @name: Name of the bus
+ * @bloom: Bloom parameters for this bus
+ * @mode: The access mode for the device node
+ * @uid: The uid of the device node
+ * @gid: The gid of the device node
+ * @bus: Pointer to a reference where the new bus is stored
+ *
+ * This function will allocate a new kdbus_bus and link it to the given
+ * domain.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int kdbus_bus_new(struct kdbus_domain *domain,
+ const struct kdbus_cmd_make *make,
+ const char *name,
+ const struct kdbus_bloom_parameter *bloom,
+ umode_t mode, kuid_t uid, kgid_t gid,
+ struct kdbus_bus **bus)
+{
[snip]
+
+ if (!capable(CAP_IPC_OWNER) &&
+ atomic_inc_return(&b->user->buses) > KDBUS_USER_MAX_BUSES) {
+ atomic_dec(&b->user->buses);
+ ret = -EMFILE;
+ goto exit_unref_user_unlock;
+ }
+
On Wed, Oct 29, 2014 at 08:59:44PM -0700, Eric W. Biederman wrote:
Greg Kroah-Hartman [off-list ref] writes:
The way capabilities are checked in this patch make me very nervous.
We are not checking permissions at open time. Every other location
of calling capable on file like objects has been show to be suceptible
to file descriptor pass attacks.
Yes, I do understand the concern, this is valid for some cases! but we
can't apply it on the ioctl API ?! please see below:
All (perhaps not all) the current ioctl do not check for fd passing
attacks! if a privileged do arbitrary ioctl on untrusted fds we are
already owned... the dumb privileged process is the one to blame, right?
Example:
1) fs/ext4/ioctl.c:ext4_ioctl()
they have:
inode_owner_or_capable() + capable() checks
for all the restricted ioctl()
2) fs/xfs/xfs_ioctl.c:xfs_file_ioctl()
they have:
capable() checks
3) fs/btrfs/ioctl.c:btrfs_ioctl()
they have capable() + inode_owner_or_capable()
... long list
These are sensible API and they do not care at all about fd passing,
so I don't think we should care either ?! or perhaps I'm missing
something ?
The capable() is done as it is, and for the inode_owner_or_capable() you
will notice that we followed the same logic and did use it in our
kdbus_bus_uid_is_privileged() to stay safe and follow what other API are
doing.
Thank you for the comments!
quoted
See Documentation/kdbus.txt for more details.
Signed-off-by: Daniel Mack <daniel@zonque.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
quoted
diff --git a/drivers/misc/kdbus/bus.c b/drivers/misc/kdbus/bus.c
new file mode 100644
index 000000000000..6dcaf22f5d59
--- /dev/null
+++ b/drivers/misc/kdbus/bus.c
quoted
+/**
+ * kdbus_bus_cred_is_privileged() - check whether the given credentials in
+ * combination with the capabilities of the
+ * current thead are privileged on the bus
+ * @bus: The bus to check
+ * @cred: The credentials to match
+ *
+ * Return: true if the credentials are privileged, otherwise false.
+ */
+bool kdbus_bus_cred_is_privileged(const struct kdbus_bus *bus,
+ const struct cred *cred)
+{
+ /* Capabilities are *ALWAYS* tested against the current thread, they're
+ * never remembered from conn-credentials. */
+ if (ns_capable(&init_user_ns, CAP_IPC_OWNER))
+ return true;
+
+ return uid_eq(bus->uid_owner, cred->fsuid);
+}
+
+/**
+ * kdbus_bus_uid_is_privileged() - check whether the current user is a
+ * priviledged bus user
+ * @bus: The bus to check
+ *
+ * Return: true if the current user has CAP_IPC_OWNER capabilities, or
+ * if it has the same UID as the user that created the bus. Otherwise,
+ * false is returned.
+ */
+bool kdbus_bus_uid_is_privileged(const struct kdbus_bus *bus)
+{
+ return kdbus_bus_cred_is_privileged(bus, current_cred());
+}
quoted
+/**
+ * kdbus_bus_new() - create a new bus
+ * @domain: The domain to work on
+ * @make: Pointer to a struct kdbus_cmd_make containing the
+ * details for the bus creation
+ * @name: Name of the bus
+ * @bloom: Bloom parameters for this bus
+ * @mode: The access mode for the device node
+ * @uid: The uid of the device node
+ * @gid: The gid of the device node
+ * @bus: Pointer to a reference where the new bus is stored
+ *
+ * This function will allocate a new kdbus_bus and link it to the given
+ * domain.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int kdbus_bus_new(struct kdbus_domain *domain,
+ const struct kdbus_cmd_make *make,
+ const char *name,
+ const struct kdbus_bloom_parameter *bloom,
+ umode_t mode, kuid_t uid, kgid_t gid,
+ struct kdbus_bus **bus)
+{
[snip]
quoted
+
+ if (!capable(CAP_IPC_OWNER) &&
+ atomic_inc_return(&b->user->buses) > KDBUS_USER_MAX_BUSES) {
+ atomic_dec(&b->user->buses);
+ ret = -EMFILE;
+ goto exit_unref_user_unlock;
+ }
+
--
Djalal Harouni
http://opendz.org