Re: [PATCH] p9caps: add Plan9 capability devices
Am Mittwoch, 25. April 2018, 12:38:02 CEST schrieb Enrico Weigelt:
> >> + list_for_each_safe(pos, q, &(caphash_writers)) {
> >> + tmp = list_entry(pos, struct caphash_entry, list);
> >
> > list_for_each_entry.
>
> what's the exact difference ?
Whoops, I meant list_for_each_entry_safe().
You don't list_entry() then.
Thanks,
//richard
--
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y
Re: [PATCH] p9caps: add Plan9 capability devices
On 17.02.2018 23:11, Richard Weinberger wrote:
Hi,
+static LIST_HEAD(caphash_writers); >> +>> +static DEFINE_MUTEX(lock);>> +>> +struct crypto_ahash *hmac_tfm
= NULL;>> +>> +static int caphash_open(struct inode *inode, struct file
*filp)>> +{>> + struct caphash_writer *tmp = NULL;>> +
struct user_namespace *user_ns = current_user_ns();>> + int retval
= 0;>> + struct list_head *pos, *q;>> +>> + /* make sure
only one instance per namespace can be opened */>> +
mutex_lock(&lock);>> +>> + list_for_each_safe(pos, q,
&(caphash_writers)) {>> + tmp = list_entry(pos, struct
caphash_writer, list);>> + if (tmp->user_ns == user_ns)
{>> + pr_err("already locked in this namespace\n");>
So, evil guy opens the device but does not close it, therefore the
whole service is blocked in a namespace?
Yes, exactly as specified. There may be only one host factotum running,
which can create caps. It's an important security feature.
In general, I think we should open that device in
kernel_init_freeable() and hand over the fd to init/systemd.
That would require an customized init and factotum, and wouldn't be
Plan9 compatible.
+static int caphash_release(struct inode *inode, struct file *filp)
+{
+ int retval = 0;
+ struct user_namespace *user_ns = current_user_ns();
Why not obtaining the user namespace from the open file?
That way one can close a caphash file hande she never opened.
Think of open, followed by nsenter, ...
hmm, good point.
+ list_for_each_safe(pos, q, &(caphash_writers)) {
+ tmp = list_entry(pos, struct caphash_entry, list);
list_for_each_entry.
what's the exact difference ?
+static ssize_t capuse_write(struct file *filp, const char __user *buf,
+ size_t count, loff_t *f_pos)
+{
+ ssize_t retval = count;
+ char *rand_str, *src_uname, *dst_uname;
+ u8 hashval[SHA1_DIGEST_SIZE] = { 0 };
+ char *cmdbuf;
+
+ if (!(cmdbuf = kzalloc(count, GFP_KERNEL)))
count is unbound, please apply a limit check.
ok.
+ {
+ char *walk = cmdbuf;
cmdbuf is toxic, make sure it is at least nul-terminated.
ok.
+ if (hmac_tfm)
IS_ERR()? Otherwise you free a error value, if crypto_alloc_ahash() fails.
ok
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
i...@metux.net -- +49-151-27565287
Re: [PATCH] p9caps: add Plan9 capability devices
On Sun, Feb 11, 2018 at 10:50 PM, Enrico Weigelt, metux IT consult
wrote:
> From: "Enrico Weigelt, metux IT consult"
>
> This driver implements the Plan9 capability devices, used for
> switching user id via capability tokens.
>
> https://9p.io/sys/doc/auth.html
Please see some nit-picks below.
I'm still reading the plan9 docs to fully get the big picture.
> ---
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile| 1 +
> drivers/staging/p9caps/Kconfig | 11 ++
> drivers/staging/p9caps/Makefile | 1 +
> drivers/staging/p9caps/p9caps.c | 369
>
> 5 files changed, 384 insertions(+)
> create mode 100644 drivers/staging/p9caps/Kconfig
> create mode 100644 drivers/staging/p9caps/Makefile
> create mode 100644 drivers/staging/p9caps/p9caps.c
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 554683912cff..23f325339fe8 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -118,4 +118,6 @@ source "drivers/staging/vboxvideo/Kconfig"
>
> source "drivers/staging/pi433/Kconfig"
>
> +source "drivers/staging/p9caps/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 6e536020029a..eccdf4643453 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -3,6 +3,7 @@
>
> obj-y += media/
> obj-y += typec/
> +obj-$(CONFIG_PLAN9CAPS)+= p9caps/
> obj-$(CONFIG_IRDA) += irda/net/
> obj-$(CONFIG_IRDA) += irda/drivers/
> obj-$(CONFIG_PRISM2_USB) += wlan-ng/
> diff --git a/drivers/staging/p9caps/Kconfig b/drivers/staging/p9caps/Kconfig
> new file mode 100644
> index ..b909daaa79ce
> --- /dev/null
> +++ b/drivers/staging/p9caps/Kconfig
> @@ -0,0 +1,11 @@
> +config PLAN9CAPS
> + tristate "Plan 9 capability device"
> + default n
> + select CRYPTO_HMAC
> + select CRYPTO_SHA1
> + help
> + This module implements the Plan 9 capability devices
> + /dev/caphash and /dev/capuse
> +
> + To compile this driver as a module, choose
> + M here: the module will be called p9caps.
> diff --git a/drivers/staging/p9caps/Makefile b/drivers/staging/p9caps/Makefile
> new file mode 100644
> index ..67d38099a249
> --- /dev/null
> +++ b/drivers/staging/p9caps/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PLAN9CAPS)+= p9caps.o
> diff --git a/drivers/staging/p9caps/p9caps.c b/drivers/staging/p9caps/p9caps.c
> new file mode 100644
> index ..e46b09821c18
> --- /dev/null
> +++ b/drivers/staging/p9caps/p9caps.c
> @@ -0,0 +1,369 @@
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * Plan9 /dev/caphash and /dev/capuse device
> + *
> + * 2DO: - caphash should only allow one process (per userns)
> + * - support textual user names
> + * - invalidate old caps
> + */
> +
> +#define DEVICE_CAPUSE "/dev/capuse"
> +#define DEVICE_CAPHASH "/dev/caphash"
> +
> +struct caphash_entry {
> + struct list_head list;
> + struct user_namespace *user_ns;
> + char data[SHA1_DIGEST_SIZE];
> +};
> +
> +struct caphash_writer {
> + struct list_head list;
> + struct user_namespace *user_ns;
> +};
> +
> +static dev_t caphash_devid = 0;
> +static dev_t capuse_devid = 0;
Static vars are already 0, no need to initialize them.
> +static LIST_HEAD(caphash_entries);
> +static LIST_HEAD(caphash_writers);
> +
> +static DEFINE_MUTEX(lock);
> +
> +struct crypto_ahash *hmac_tfm = NULL;
> +
> +static int caphash_open(struct inode *inode, struct file *filp)
> +{
> + struct caphash_writer *tmp = NULL;
> + struct user_namespace *user_ns = current_user_ns();
> + int retval = 0;
> + struct list_head *pos, *q;
> +
> + /* make sure only one instance per namespace can be opened */
> + mutex_lock(&lock);
> +
> + list_for_each_safe(pos, q, &(caphash_writers)) {
> + tmp = list_entry(pos, struct caphash_writer, list);
> + if (tmp->user_ns == user_ns) {
> + pr_err("already locked in this namespace\n");
So, evil guy opens the device but does not close it, therefore the
whole service is blocked in a namespace?
In general, I think we should open that device in
kernel_init_freeable() and hand over the fd to init/systemd.
As the plan9 docs state "The write-only file /dev/caphash can be
opened only by the host owner, and only once. Factotum opens this file
immediately after booting."
> + retval = -EBUSY;
> + goto out;
> + }
> + }
> +
> + if (!(tmp = kzalloc(sizeof(struct caphash_writer), GFP_KERNEL))) {
> + retval = -ENOMEM;
Re: [PATCH] p9caps: add Plan9 capability devices
On 14.02.2018 15:56, Serge E. Hallyn wrote: If it's an out of tree module you'd have to do it this way, but if > it's in-tree, even as a module, adding a bit to the userns struct> would imo be ok. Assuming one doesn't try to load the module when the kernel image previously was built w/o it ;-) (well, could export some dummy symbol for protection ;-)). OTOH, that raises the question, where / how exactly the cap list destruction / expiry should be done. My original plan was adding a timer in the p9caps module that just scans for old entries. Should the userns code just call back on userns destruction ? (in that case it would be tricky to have it as a module) the whole thing might become a bit more complex when introducing plan9-like unprivileged mount operations. haven't sorted out how to do that yet. I hope you'll have a discussion here about that first. Yes, of course - that's why I'm here :p My current idea is introducing some special flag for disabling suid completely and switch into an private namespace, where now the unprivileged user can mount at will and create new mnt namespaces, just like on Plan9. I'll try some qnd hacks w/ a new syscall, lets see where it leads to, and then sort out how to do that in a more appropriate way. Now speaking practically, I love the caphash idea, but it does have issues with a modern login system. There are privileged things which login needs to do besides changing uid, including but not limited to: 1. setting limits 2. setting loginuid, 3. mounting things (polyinstantiated /tmp, decrypted homedir, etc) 4. setting selinux context For now, I don't think that's necessary for doing things the Plan9 way. Perhaps we later could extend the /dev/caphash interface w/ additional parameters for that. (and of course gplv3 as Al pointed out is a blocker) already fixed. --mtx -- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering i...@metux.net -- +49-151-27565287
Re: [PATCH] p9caps: add Plan9 capability devices
Quoting Enrico Weigelt, metux IT consult (me...@gmx.de): > On 13.02.2018 07:16, Serge E. Hallyn wrote: > >>+ /* make sure only one instance per namespace can be opened */ > > ... > >>at a time > yeah, right. > > >might be better to keep this state in the user_ns itself, would > >avoid kzalloc below. > > thought about, but hesitated to touch user_ns. might not be the best > idea when having p9caps as module (OTOH, doesn't need to be a module) If it's an out of tree module you'd have to do it this way, but if it's in-tree, even as a module, adding a bit to the userns struct would imo be ok. > the whole thing might become a bit more complex when introducing > plan9-like unprivileged mount operations. haven't sorted out how to > do that yet. I hope you'll have a discussion here about that first. > >Would it be worth doing any privilege checking here? > > Which ones should I check ? Well, granting privileges to another task is extra-special, so in the 2010 submission we created a new POSIX capability for it (CAP_GRANT_ID). Now speaking practically, I love the caphash idea, but it does have issues with a modern login system. There are privileged things which login needs to do besides changing uid, including but not limited to: 1. setting limits 2. setting loginuid, 3. mounting things (polyinstantiated /tmp, decrypted homedir, etc) 4. setting selinux context These are probably all solvable, and don't need to be solved before we solve this part, that would be silly, but they're worth thinking about. The ones which must be done on the current task are the hardest to solve. (and of course gplv3 as Al pointed out is a blocker) -serge
Re: [PATCH] p9caps: add Plan9 capability devices
On 13.02.2018 07:16, Serge E. Hallyn wrote: + /* make sure only one instance per namespace can be opened */ > > ... at a time yeah, right. might be better to keep this state in the user_ns itself, would avoid kzalloc below. thought about, but hesitated to touch user_ns. might not be the best idea when having p9caps as module (OTOH, doesn't need to be a module) the whole thing might become a bit more complex when introducing plan9-like unprivileged mount operations. haven't sorted out how to do that yet. Would it be worth doing any privilege checking here? Which ones should I check ? --mtx
Re: [PATCH] p9caps: add Plan9 capability devices
On Sun, Feb 11, 2018 at 09:50:28PM +, Enrico Weigelt, metux IT consult
wrote:
> From: "Enrico Weigelt, metux IT consult"
>
> This driver implements the Plan9 capability devices, used for
> switching user id via capability tokens.
>
> https://9p.io/sys/doc/auth.html
> ---
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile| 1 +
> drivers/staging/p9caps/Kconfig | 11 ++
> drivers/staging/p9caps/Makefile | 1 +
> drivers/staging/p9caps/p9caps.c | 369
>
> 5 files changed, 384 insertions(+)
> create mode 100644 drivers/staging/p9caps/Kconfig
> create mode 100644 drivers/staging/p9caps/Makefile
> create mode 100644 drivers/staging/p9caps/p9caps.c
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 554683912cff..23f325339fe8 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -118,4 +118,6 @@ source "drivers/staging/vboxvideo/Kconfig"
>
> source "drivers/staging/pi433/Kconfig"
>
> +source "drivers/staging/p9caps/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 6e536020029a..eccdf4643453 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -3,6 +3,7 @@
>
> obj-y+= media/
> obj-y+= typec/
> +obj-$(CONFIG_PLAN9CAPS) += p9caps/
> obj-$(CONFIG_IRDA) += irda/net/
> obj-$(CONFIG_IRDA) += irda/drivers/
> obj-$(CONFIG_PRISM2_USB) += wlan-ng/
> diff --git a/drivers/staging/p9caps/Kconfig b/drivers/staging/p9caps/Kconfig
> new file mode 100644
> index ..b909daaa79ce
> --- /dev/null
> +++ b/drivers/staging/p9caps/Kconfig
> @@ -0,0 +1,11 @@
> +config PLAN9CAPS
> + tristate "Plan 9 capability device"
> + default n
> + select CRYPTO_HMAC
> + select CRYPTO_SHA1
> + help
> + This module implements the Plan 9 capability devices
> + /dev/caphash and /dev/capuse
> +
> + To compile this driver as a module, choose
> + M here: the module will be called p9caps.
> diff --git a/drivers/staging/p9caps/Makefile b/drivers/staging/p9caps/Makefile
> new file mode 100644
> index ..67d38099a249
> --- /dev/null
> +++ b/drivers/staging/p9caps/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PLAN9CAPS) += p9caps.o
> diff --git a/drivers/staging/p9caps/p9caps.c b/drivers/staging/p9caps/p9caps.c
> new file mode 100644
> index ..e46b09821c18
> --- /dev/null
> +++ b/drivers/staging/p9caps/p9caps.c
> @@ -0,0 +1,369 @@
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * Plan9 /dev/caphash and /dev/capuse device
> + *
> + * 2DO: - caphash should only allow one process (per userns)
> + * - support textual user names
> + * - invalidate old caps
> + */
> +
> +#define DEVICE_CAPUSE"/dev/capuse"
> +#define DEVICE_CAPHASH "/dev/caphash"
> +
> +struct caphash_entry {
> + struct list_head list;
> + struct user_namespace *user_ns;
> + char data[SHA1_DIGEST_SIZE];
> +};
> +
> +struct caphash_writer {
> + struct list_head list;
> + struct user_namespace *user_ns;
> +};
> +
> +static dev_t caphash_devid = 0;
> +static dev_t capuse_devid = 0;
> +
> +static LIST_HEAD(caphash_entries);
> +static LIST_HEAD(caphash_writers);
> +
> +static DEFINE_MUTEX(lock);
> +
> +struct crypto_ahash *hmac_tfm = NULL;
> +
> +static int caphash_open(struct inode *inode, struct file *filp)
> +{
> + struct caphash_writer *tmp = NULL;
> + struct user_namespace *user_ns = current_user_ns();
> + int retval = 0;
> + struct list_head *pos, *q;
> +
> + /* make sure only one instance per namespace can be opened */
... at a time
might be better to keep this state in the user_ns itself, would
avoid kzalloc below.
Would it be worth doing any privilege checking here?
(incidentally, for historical reference, https://lkml.org/lkml/2010/4/20/404 :)
> + mutex_lock(&lock);
> +
> + list_for_each_safe(pos, q, &(caphash_writers)) {
> + tmp = list_entry(pos, struct caphash_writer, list);
> + if (tmp->user_ns == user_ns) {
> + pr_err("already locked in this namespace\n");
> + retval = -EBUSY;
> + goto out;
> + }
> + }
> +
> + if (!(tmp = kzalloc(sizeof(struct caphash_writer), GFP_KERNEL))) {
> + retval = -ENOMEM;
> + goto out;
> + }
> +
> + tmp->user_ns = get_user_ns(user_ns);
> + list_add(&(tmp->list), &caphash_writers);
> +
> +out:
> + mutex_unlock(&lock);
> + return retval;
> +}
Re: [PATCH] p9caps: add Plan9 capability devices
On Sat, Feb 10, 2018 at 04:58:45PM +, Enrico Weigelt, metux IT consult
wrote:
> From: "Enrico Weigelt, metux IT consult"
> +MODULE_LICENSE("GPLv3");
... which is incompatible with GPLv2. I'm not even going to look at that
thing - same as with proprietary code. You CAN'T combine any derivatives
of that code with the kernel. Not without GPL violation.
And yes, there is GPLv2-only code all over the place in Linux VFS (at
least), with at least some of the copyright holders not going to permit
violations of clause 6. Not going to happen.
If you want it in the kernel, you need a GPLv2-compatible license. GPLv3
does not qualify, and while it can be combined with GPLv2+ (yielding GPLv3
for combination), the kernel is not GPLv2+ - some parts are, but GPLv2-only
ones render the whole thing GPLv2-only.
Re: [PATCH] p9caps: add Plan9 capability devices
Hi,
On 02/10/2018 08:58 AM, Enrico Weigelt, metux IT consult wrote:
> From: "Enrico Weigelt, metux IT consult"
>
> This driver implements the Plan9 capability devices, used for
> switching user id via capability tokens.
>
> https://9p.io/sys/doc/auth.html
> ---
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile| 1 +
> drivers/staging/p9caps/Kconfig | 11 ++
> drivers/staging/p9caps/Makefile | 1 +
> drivers/staging/p9caps/p9caps.c | 371
>
> 5 files changed, 386 insertions(+)
> create mode 100644 drivers/staging/p9caps/Kconfig
> create mode 100644 drivers/staging/p9caps/Makefile
> create mode 100644 drivers/staging/p9caps/p9caps.c
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 554683912cff..23f325339fe8 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -118,4 +118,6 @@ source "drivers/staging/vboxvideo/Kconfig"
>
> source "drivers/staging/pi433/Kconfig"
>
> +source "drivers/staging/p9caps/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index 6e536020029a..eccdf4643453 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -3,6 +3,7 @@
>
> obj-y+= media/
> obj-y+= typec/
> +obj-$(CONFIG_PLAN9CAPS) += p9caps/
> obj-$(CONFIG_IRDA) += irda/net/
> obj-$(CONFIG_IRDA) += irda/drivers/
> obj-$(CONFIG_PRISM2_USB) += wlan-ng/
> diff --git a/drivers/staging/p9caps/Kconfig b/drivers/staging/p9caps/Kconfig
> new file mode 100644
> index ..455c3fa726ff
> --- /dev/null
> +++ b/drivers/staging/p9caps/Kconfig
> @@ -0,0 +1,11 @@
> +config PLAN9CAPS
> + tristate "Plan 9 capability device"
> + default n
> + select CRYPTO_HMAC
> + select CRYPTO_SHA1
> + help
> + This module implements the Plan 9 capability devices
> + /dev/caphash and /dev/capuse
> +
> + To compile this driver as a module, choose
> + M here: the module will be called p9auth.
Just below here (Makefile), it's called p9caps, not p9auth.
> diff --git a/drivers/staging/p9caps/Makefile b/drivers/staging/p9caps/Makefile
> new file mode 100644
> index ..67d38099a249
> --- /dev/null
> +++ b/drivers/staging/p9caps/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_PLAN9CAPS) += p9caps.o
> diff --git a/drivers/staging/p9caps/p9caps.c b/drivers/staging/p9caps/p9caps.c
> new file mode 100644
> index ..4c5c94dc1893
> --- /dev/null
> +++ b/drivers/staging/p9caps/p9caps.c
> @@ -0,0 +1,371 @@
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/*
> + * Plan9 /dev/caphash and /dev/capuse device
> + *
> + * 2DO: - caphash should only allow one process (per userns)
> + * - support textual user names
> + * - invalidate old caps
> + */
> +
> +#define DEVICE_CAPUSE"/dev/capuse"
> +#define DEVICE_CAPHASH "/dev/caphash"
> +
> +#define MODNAME "p9cap"
p9caps ?
> +
> +struct caphash_entry {
> + struct list_head list;
> + struct user_namespace *user_ns;
> + char data[SHA1_DIGEST_SIZE];
> +};
> +
> +struct caphash_writer {
> + struct list_head list;
> + struct user_namespace *user_ns;
> +};
> +
> +static dev_t caphash_devid = 0;
> +static dev_t capuse_devid = 0;
> +
> +static LIST_HEAD(caphash_entries);
> +static LIST_HEAD(caphash_writers);
> +
> +static DEFINE_MUTEX(p9cap_lock);
> +
> +struct crypto_ahash *p9cap_tfm = NULL;
> +
> +static int caphash_open(struct inode *inode, struct file *filp)
> +{
> + struct caphash_writer *tmp = NULL;
> + struct user_namespace *user_ns = current_user_ns();
> + int retval = 0;
> + struct list_head *pos, *q;
> +
> + /* make sure only one instance per namespace can be opened */
> + mutex_lock(&p9cap_lock);
> +
> + list_for_each_safe(pos, q, &(caphash_writers)) {
> + tmp = list_entry(pos, struct caphash_writer, list);
> + if (tmp->user_ns == user_ns) {
> + printk(KERN_ERR DEVICE_CAPHASH ": already locked in
> this namespace\n");
> + retval = -EBUSY;
> + goto out;
> + }
> + }
> +
> + if (!(tmp = kzalloc(sizeof(struct caphash_writer), GFP_KERNEL))) {
> + retval = -ENOMEM;
> + goto out;
> + }
> +
> + tmp->user_ns = get_user_ns(user_ns);
> + list_add(&(tmp->list), &caphash_writers);
> +
> +out:
> + mutex_unlock(&p9cap_lock);
> + return retval;
> +}
> +
> +static int caphash_release(struct inode *inode, struct file *filp)
> +{
> + int retval = 0;
> + struct user_namespace *user_ns = current_user_ns();
> + struct list_head *pos, *q;
> + stru
