[PATCH] implement in-kernel keys & keyring management

[PATCH] implement in-kernel keys & keyring management

[David Howells]

Hi Linus, Andrew,

I've made available a patch that does a better job of key and keyring
management for authentication, cryptography, etc.. I've added a good bit of
documentation and I've commented the code more thoroughly.

The patch can be found at:

	http://people.redhat.com/~dhowells/keys/keys-268rc2.diff.bz2

	Signed-Off-By: David Howells <dhowells@redhat.com>

The documentation is patched into Documentation/keys.txt.


The feature set the patch includes:

 - Key attributes:
   - Key type
   - Description (by which a key of a particular type can be selected)
   - Payload
   - UID, GID and permissions mask
   - Expiry time
 - Keyrings (just a type of key that holds links to other keys)
 - User-defined keys
 - Key revokation
 - Access controls
 - Per user key-count and key-memory consumption quota
 - Three std keyrings per task: per-thread, per-process, session
 - Two std keyrings per user: per-user and default-user-session
 - prctl() functions for key and keyring creation and management
 - Kernel interfaces for filesystem, blockdev, net stack access
 - JIT key creation by usermode helper

There are also two utility programs available:

 (*) http://people.redhat.com/~dhowells/keys/keyctl.c

     A comprehensive key management tool, permitting all the interfaces
     available to userspace to be exercised.

 (*) http://people.redhat.com/~dhowells/keys/request-key

     An example shell script (to be installed in /sbin) for instantiating a
     key.

David

Re: [PATCH] implement in-kernel keys & keyring management

[Andrew Morton]

David Howells <dhowells@redhat.com> wrote:
>
> I've made available a patch that does a better job of key and keyring
>  management for authentication, cryptography, etc.. I've added a good bit of
>  documentation and I've commented the code more thoroughly.

Nicely presented patch, thanks.

- Why have a separate key_user structure, rather than adding stuff to
  struct user_struct?  This appears to be feasible.

- I had a hard time working out the locking which protects key->flags. 
  It looks racy.

- Are the various system-wide locks going to end up hurting on big SMP?

- user_describe_key() appears to leak a key ref if kmalloc() fails. 
  user_describe_key() appears to leak a key ref if copy_to_user() faults. 
  The one-return-statement-per-function rule rules.

- Various people are likely to get upset about this:

  asmlinkage long sys_keyctl(int option, unsigned long arg2, unsigned long arg3,
		   unsigned long arg4, unsigned long arg5)

  I guess the pure way to do it is to add 13 new syscalls....

- I really hesitate to stick my nose into this perennial, but
  keyring_hash() is a bit lame.  I once read that

	hash = hash * 33 + *p++;

  works as well as pretty much anything else.

- All that keyring tree management (keyring_detect_cycle) looks tricky. 
  Do we really need such complexity?

- __key_link() does

	/* dispose of the old keyring list */
	if (klist)
		kfree(klist);

  but elsewhere you freely do kfree(0)

- Why does the proc code do spin_lock_irq(&key_user_lock)?  Other
  process-context code just does spin_lock().

- In key_create_or_update():

	mode = 0100;
	if (ktype == &key_type_keyring)
		mode = 0700;
	else if (ktype == &key_type_user)
		mode = 0700;
	else if (ktype->update)
		mode |= 0300;

  you've used the stat.h symbols elsewhere.

- How come install_process_keyring() and friends take task_lock() around
  the key_put()?  It's not done elsewhere.

  Please update the what-it-locks comment over task_lock()

- Documentation/CodingStyle doesn't mention

		if (clone_flags & CLONE_THREAD) {
			atomic_inc(&tsk->process_keyring->usage);
		}
		else {

- join_session_keyring() leaks the memory at `name'.  Trivial DoS hole. 
  Please audit the whole patch.

- request_key_construction() leaks `key' if __call_request_key() fails. 
  Multiple return statements again...

- request_key() appears to leak a ref to the key if key_user_lookup()
  fails.  Multiple return statements.

- In user_instantiate():

	if (!key->payload.data) {
		key_put(key->payload.data);

  the key_put() can be removed.


It's a lot of code, isn't it?

[PATCH] implement in-kernel keys & keyring management [try #2]

[David Howells]

I've put a revised patch at:

	http://people.redhat.com/~dhowells/keys/keys-268rc2-2.diff.bz2
	Signed-Off-By: David Howells <dhowells@redhat.com>

Andrew Morton <akpm@osdl.org> wrote:
> - Why have a separate key_user structure, rather than adding stuff to
>   struct user_struct?  This appears to be feasible.

It is not. user_struct pins two keyrings; these two keyrings pin the tracking
structure, which if it was merged into user_struct would cause the keyrings to
pin it. Even if those two keyrings themselves are marked such they don't pin
the user_struct, they may contain a key somewhere down the tree that does.

I thought long and hard about that, and unfortunately, I think it's necessary
- unless we delete every key or keyring a user has caused to exist at the time
the user_struct goes away. Alternatively, we could just discard the quota
tracking, but this would mean a user could potentially gain more quota by
logging out.

> - I had a hard time working out the locking which protects key->flags. 
>   It looks racy.

Hmmm... Thinking about it, you're probably right. I've made it use a write
lock on the key's lock anytime the flags are changed if the key can be
accessed (eg: key_alloc and key_cleanup don't need locks).

> - Are the various system-wide locks going to end up hurting on big SMP?

They shouldn't. They aren't used exclusively very much.

 (*) key_serial_lock is only used for key allocation, destruction and
     procfs. Key creation shouldn't be a particularly common event.

 (*) key_user_lock is used more or less the same, except it's used during call
     back to userspace too.

 (*) The key types sem is read-locked during key construction, but only locked
     exclusively when a type is being added or removed (which should be a very
     rare event).

 (*) The key session sem should only be locked when a process joins a new
     session. This should be very rare too.

 (*) The keyring name lock is only called during keyring creation, destruction
     and lookup. The first too shouldn't be particularly common, and the last
     should only be done at userspace's behest (usually joining a named
     session), and so should be very rare.

     One problem though: currently each task has the pointer to the process
     keyring in the task structure, which means that the process keyring must
     exist before CLONE_THREAD happens so that it can be shared. Currently, I
     create a new process keyring during fork if necessary and exec.

     Maybe what I should do is move some of the thread-group specific stuff
     out of the task_struct into a separate structure that's shared between
     CLONE_THREAD tasks; this would permit me to only create the process
     keyring when necessary.

     Alternatively, I could just not create a process keyring until it's
     required or CLONE_THREAD happens.

 (*) The key construction lock. This is only locked as often as keys are
     instantiated or requested from userspace, so it shouldn't be a common
     event on a system.

But basically, in answer to you question, this shouldn't be a problem - almost
all the operations you're likely to do - searching for and using keys - are
done with per-key shared spinlocks.

The construction queue locks could be made per-user, I suppose.

> - user_describe_key() appears to leak a key ref if kmalloc() fails. 
>   user_describe_key() appears to leak a key ref if copy_to_user() faults. 

Fixed.

> The one-return-statement-per-function rule rules.

Not necessarily. Do you ever investigate the asm produced by gcc?

For example, if you look as sys_keyctl() where each case statement is along
the lines of:

	case KEYCTL_NEW:
		return add_user_key((key_serial_t) arg2,
				    (const char __user *) arg3,
				    (const char __user *) arg4,
				    (const void __user *) arg5);

If you change these case statements to:

	case KEYCTL_NEW:
		ret = add_user_key((key_serial_t) arg2,
				    (const char __user *) arg3,
				    (const char __user *) arg4,
				    (const void __user *) arg5);
		break;

And just return ret after the end of the switch statement, then the compiler
will behave differently.

In the first case, the call will be rendered as a JUMP, in the second you'll
get a CALL, a BRANCH and a RETURN. In that case
one-return-statement-per-function sucks.

Similarly with exec_keys() where I'd like to end on a tail call:

	/* newly exec'd tasks get a fresh process keyring */
	return install_process_keyring(tsk);

But can only do so if there's no error. This is now:

	/* newly exec'd tasks get a fresh process keyring */
	ret = install_process_keyring(tsk);
 error:
	return ret;

Meaning gcc won't generate a JUMP, but rather CALL and RETURN.

Besides that, insisting on one return statement per function is slightly
tricky to make efficient when returning a pointer that either points to a
structure with an elevated refcount or has an error cast into it; especially
so when locking is involved.

Also it requires extra variables and gotos and indentation in cases where
they're not really necessary. However, since you insisted, every function
barring sys_keyctl() should now have a single return statement.

> - Various people are likely to get upset about this:
> ...
>   I guess the pure way to do it is to add 13 new syscalls....

I don't really want to add any syscalls, though I wouldn't be too upset to add
just one:-/

What're other people's thoughts on this?

> - I really hesitate to stick my nose into this perennial, but
>   keyring_hash() is a bit lame.  I once read that
> 
> 	hash = hash * 33 + *p++;
> 
>   works as well as pretty much anything else.

It makes no difference if you then do "hash & 0x1f" to pick your bucket (it's
equivalent to "hash += (hash << 5) + *p++"). I suppose you'd have to fold the
hash value down somehow.

> - All that keyring tree management (keyring_detect_cycle) looks tricky. 
>   Do we really need such complexity?

Absolutely. A keyring search is a tree search (we supported keyring nesting),
so we have to lock each level as we down, and unlock as we come back up, lest
the tree change beneath us.

If we don't prevent cycles in the tree, then we can end up deadlocking
ourselves if someone is waiting to writelock something we've got locked.

Besides, since a keyring pins all the keys it points to, you can end up with a
set of resources mutually pinning one another in memory.

Think of hardlinking directories on a filesystem.

> - __key_link() does
>   but elsewhere you freely do kfree(0)

Fixed. Isn't it more efficient, though, to check beforehand if we know the
value might well be 0?

> - Why does the proc code do spin_lock_irq(&key_user_lock)?  Other
>   process-context code just does spin_lock().

Fixed. key_put() can be called from interrupt context. It used to grab those
locks there, and so anything locking those locks had to disable irqs. I've
since changed things so that the actual key destruction is deferred to process
context, so I don't have to disable irqs when touching those locks.

> - In key_create_or_update():
> ...
>   you've used the stat.h symbols elsewhere.

Fixed. I thought I'd caught them all.

> - How come install_process_keyring() and friends take task_lock() around
>   the key_put()?  It's not done elsewhere.

It's not the key_put() that's locked, it's the xchg():

	task_lock(tsk);
	key_put(xchg(&tsk->process_keyring, keyring));
	task_unlock(tsk);

I've changed these to not use xchg() as there's no point, and to put the
key_put() calls outside of the critical section.

The reason task_lock() is must be held on change of keyring is that
/proc/pid/status displays the serial numbers of those keyrings, and that can
be used from another task.

Since a task can only change its own keyring subscriptions, it does not need a
lock meerly to access those keyrings. If it did, then accessing current->mm,
etc. would require a lock too.

>   Please update the what-it-locks comment over task_lock()

Done.

> - join_session_keyring() leaks the memory at `name'.  Trivial DoS hole. 
>   Please audit the whole patch.

Done. I still can't guarantee I've caught every last one, though.

> - request_key_construction() leaks `key' if __call_request_key() fails. 

>   Multiple return statements again...

Same answer again...

> - request_key() appears to leak a ref to the key if key_user_lookup()
>   fails. Multiple return statements.

I don't see it. We only get a key if search_process_keyrings() succeeds, and
if it did we returned immediately. If we didn't return, "key" was holding an
error.

I don't see how not having multiple return statements will make that easier
for you to understand, since it's the condition on the if-statement you've
misread. Please explain.

> - In user_instantiate():
>   the key_put() can be removed.

Done.

> It's a lot of code, isn't it?

	warthog>wc security/keys/*.c | sort -n
	  189   586  4584 security/keys/user_defined.c
	  214   591  5077 security/keys/proc.c
	  282   890  7090 security/keys/request_key.c
	  642  1678 14571 security/keys/process_keys.c
	  835  2489 18457 security/keys/keyctl.c
	  847  2390 19148 security/keys/keyring.c
	  925  2557 21606 security/keys/key.c
	 3934 11181 90533 total

I suppose so.


----

Anyway, I've been through and audited it again, and I've put in your suggested
changes (there're a lot fewer return statements than there used to be, and a
lot more gotos:-)

Also, what are people's thoughts on adding more error codes?

	ENOKEY		- Couldn't find or obtain a key
	EKEYREVOKED	- Key was revoked
	EKEYEXPIRED	- Key expired

And, maybe:

	ENOTKEYRING	- Key is not a keyring

Otherwise you might find, for example, open() return ENOENT because it
couldn't get a key...

David

Re: [PATCH] implement in-kernel keys & keyring management [try #3]

[David Howells]

Typical... I put up a patch and then immediately find a bug in it:-(

I've put version #3 up now (I fixed a bug in one of the error paths).

	http://people.redhat.com/~dhowells/keys/keys-268rc2-3.diff.bz2
	Signed-Off-By: David Howells <dhowells@redhat.com>

David

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[James Morris]

On Sat, 7 Aug 2004, David Howells wrote:

> >   I guess the pure way to do it is to add 13 new syscalls....
> 
> I don't really want to add any syscalls, though I wouldn't be too upset to add
> just one:-/
> 
> What're other people's thoughts on this?

Implement a filesystem interface, e.g. /proc/<pid>/keys

>From here you can have:

  /create

  /<keyid>/update
          /revoke
          /chown
          /chmod
          ...

Rather than syscalls/prctls for each of these.

For keyrings, you could have:

  /proc/<pid>/keyring/thread
                     /session
                     /process
                     ...

Instead of having /proc/keys and associated locking/seqfile overhead in 
the kernel, a userspace library could instead traverse /proc to build a 
global list of keys.

In general, I think you may be able to move logic out of the kernel this 
way, e.g. userspace searching for keys.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[David Howells]

James Morris <jmorris@redhat.com> wrote:
> Implement a filesystem interface, e.g. /proc/<pid>/keys
> 
> From here you can have:
> 
>   /create

How would this work? Remember you've got to get some data back, and you've got
to simultaneously attach your key to a keyring, otherwise it'll just be erased
immediately.

>   /<keyid>/update
>           /revoke
>           /chown
>           /chmod
>           ...

I could. You're going a little far: chown and chmod can be done by other
methods on the <keyid>/ directory.

> Rather than syscalls/prctls for each of these.

It'd require a lot more code and memory to do it with a filesystem, I think.

> For keyrings, you could have:
> 
>   /proc/<pid>/keyring/thread
>                      /session
>                      /process
>                      ...

What are these? Files containing keyring ID numbers? If so, better to just
have one file from whence you can read all the IDs, and since /proc/pid/status
has to grab the requisite lock anyway...

> Instead of having /proc/keys and associated locking/seqfile overhead in 
> the kernel, a userspace library could instead traverse /proc to build a 
> global list of keys.

And incur even more locking overhead? Besides, why? Looking at all the keys on
the system isn't something that should be done very often, and what you're
suggesting would be more expensive in the long run.

> In general, I think you may be able to move logic out of the kernel this 
> way, e.g. userspace searching for keys.

But you need to lock the keyrings as you search. Access controls also need
to be enforced, but that could probably be done in userspace (you can't scan
the contents of a keyring you don't own).

Besides, the search _has_ to be available in kernel space. A filesystem such
as AFS or NFS would need to be able to call it during file->open(), and maybe
at other times. Would you suggest that it should call out to userspace to do
the keyring search? Or would you, say, suggest a new open() syscall that takes
a key ID in addition?

And since there has to be a kernel space search routine, why not make it
available to userspace too?

David

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[James Morris]

On Mon, 9 Aug 2004, David Howells wrote:

> James Morris <jmorris@redhat.com> wrote:
> > Implement a filesystem interface, e.g. /proc/<pid>/keys
> > 
> > From here you can have:
> > 
> >   /create
> 
> How would this work? Remember you've got to get some data back, and you've got
> to simultaneously attach your key to a keyring, otherwise it'll just be erased
> immediately.

I figured you would write to the file (a keyring id?) and it would return 
a key id.

> > For keyrings, you could have:
> > 
> >   /proc/<pid>/keyring/thread
> >                      /session
> >                      /process
> >                      ...
> 
> What are these? Files containing keyring ID numbers? If so, better to just
> have one file from whence you can read all the IDs, and since /proc/pid/status
> has to grab the requisite lock anyway...

They would contain symlinks to keyring IDs.

> Besides, the search _has_ to be available in kernel space. A filesystem such
> as AFS or NFS would need to be able to call it during file->open(), and maybe
> at other times. Would you suggest that it should call out to userspace to do
> the keyring search?

No.  The reason for suggesting this was because with a filesystem API, the 
information is already available in userspace, and it would be quite 
simple to enumerate it.  As you said, it's not something that would happen 
all the time, so it's not performance critical.  But if you need a kernel 
API for the same thing, it's a moot point.

> Or would you, say, suggest a new open() syscall that takes a key ID in
> addition?

No, that would require changes to userspace code.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[David Howells]

> > How would this work? Remember you've got to get some data back, and you've
> > got to simultaneously attach your key to a keyring, otherwise it'll just
> > be erased immediately.
> 
> I figured you would write to the file (a keyring id?) and it would return 
> a key id.

What do you mean by return? You can't pass it back as write()'s return
value. I suppose you could then arrange for the fd to be read():-/

> > What are these? Files containing keyring ID numbers? If so, better to just
> > have one file from whence you can read all the IDs, and since
> > /proc/pid/status has to grab the requisite lock anyway...
> 
> They would contain symlinks to keyring IDs.

Yes, but given that the targets of the symlinks would not be in /proc, how do
you concoct the symlink. I suppose you could assume "/sysfs/keys/<keyid>" as
the target, but that's not very nice.

> > Besides, the search _has_ to be available in kernel space. A filesystem
> > such as AFS or NFS would need to be able to call it during file->open(),
> > and maybe at other times. Would you suggest that it should call out to
> > userspace to do the keyring search?
> 
> No.  The reason for suggesting this was because with a filesystem API, the 
> information is already available in userspace, and it would be quite 
> simple to enumerate it.  As you said, it's not something that would happen 
> all the time, so it's not performance critical.  But if you need a kernel 
> API for the same thing, it's a moot point.

It would be quite an involved process to do this in userspace. You'd have to
walk through a nest of keyrings, and you'd have to do a lot of file opening,
and closing (possibly 2 per key: type and description, though these could be
available through the same file) and reading of dirs and symlinks.

Maintaining access controls would be fun though... How does the kernel then
distinguish between a read and a search?

David

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[James Morris]

On Mon, 9 Aug 2004, David Howells wrote:

> > I figured you would write to the file (a keyring id?) and it would return 
> > a key id.
> 
> What do you mean by return? You can't pass it back as write()'s return
> value. I suppose you could then arrange for the fd to be read():-/

You write and then read, like the transaction based IO methods in 
selinuxfs (TA_write, TA_read).

> > > What are these? Files containing keyring ID numbers? If so, better to just
> > > have one file from whence you can read all the IDs, and since
> > > /proc/pid/status has to grab the requisite lock anyway...
> > 
> > They would contain symlinks to keyring IDs.
> 
> Yes, but given that the targets of the symlinks would not be in /proc, how do
> you concoct the symlink. I suppose you could assume "/sysfs/keys/<keyid>" as
> the target, but that's not very nice.

Not sure how to do this.

> > > Besides, the search _has_ to be available in kernel space. A filesystem
> > > such as AFS or NFS would need to be able to call it during file->open(),
> > > and maybe at other times. Would you suggest that it should call out to
> > > userspace to do the keyring search?
> > 
> > No.  The reason for suggesting this was because with a filesystem API, the 
> > information is already available in userspace, and it would be quite 
> > simple to enumerate it.  As you said, it's not something that would happen 
> > all the time, so it's not performance critical.  But if you need a kernel 
> > API for the same thing, it's a moot point.
> 
> It would be quite an involved process to do this in userspace. You'd have to
> walk through a nest of keyrings, and you'd have to do a lot of file opening,
> and closing (possibly 2 per key: type and description, though these could be
> available through the same file) and reading of dirs and symlinks.

It might be possible to do clever things with symlinks so that you can 
access the keys simply in a variety of common ways (e.g. per-session 
directory, all keys directory etc.)

> Maintaining access controls would be fun though... How does the kernel then
> distinguish between a read and a search?

I guess you'd use normal filsystem access controls instead of storing them
with the keys themselves.  i.e. keyrings are directories, and keys are
files (or directories with files including metadata and key data).  Then
xattrs could be used to integrate with things like SELinux.  A user could
then use chmod to specify whether a kerying can be searched, for example.

The main point is that a filesystem API provides several useful framework
components by default:access control, creat/open/read/write etc. syscalls,
hierarchical structure and xattrs.  Using a filesystem API means not
inventing yet another class of API.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH] implement in-kernel keys & keyring management [try #2]

[David Howells]

James Morris <jmorris@redhat.com> wrote:
> You write and then read, like the transaction based IO methods in 
> selinuxfs (TA_write, TA_read).

Hmmm... You need to consider the fact that you need three or four pieces of
information to create a key: type, description, payload (if there's a payload
- there may not be) and destination keyring ID; and the payload is a binary
blob of data.

I could say that you must do this as a single write of a buffer containing
three NUL terminated strings, one after the other, followed, if present, by a
binary blob...

Maybe I should say you have to use an ioctl()... :-)

> > Maintaining access controls would be fun though... How does the kernel then
> > distinguish between a read and a search?
> 
> I guess you'd use normal filsystem access controls instead of storing them
> with the keys themselves.  i.e. keyrings are directories, and keys are
> files (or directories with files including metadata and key data).  Then
> xattrs could be used to integrate with things like SELinux.  A user could
> then use chmod to specify whether a kerying can be searched, for example.

Keyring permissions don't map exactly to file or directory attributes. If
you, for instance, place the key permissions mask on the directory
representing the key/keyring, then search permission controls whether you can
access any file in that directory, which isn't practical.

I think I'd have to keep the key search mask in a separate file under that key
directory, and have you read or write to it rather than using chmod.

chmod() isn't an option, really: key permissions don't really map well to file
permissions. I suppose also key permissions really ought to have more than RWX
permission can handle:

	{ read, write, search, subscribe } x { UID, GID, Other }

Currently, I permit subscription (link) to any key for which you have either
read or execute permission.

I also don't really want you to be able to _see_ any key for which you don't
have any permission, so readdir() on /keyfs/ should not report any directory
corresponding to a file you can't see. I should implement this on my
/proc/keys too.

> The main point is that a filesystem API provides several useful framework
> components by default:access control, creat/open/read/write etc. syscalls,
> hierarchical structure and xattrs.  Using a filesystem API means not
> inventing yet another class of API.

I think you're wrong:

 (1) Making a service like this accessible through a filesystem is as much
     creating a new class of API, as is creating new prctl() functions or new
     syscalls. It's just an API built on read(), write(), etc. rather than
     being on the syscall interface directly.

 (2) A filesystem API is _way_ less efficient, though that probably isn't too
     much of a problem in this case. You have to do several VFS syscalls to
     achieve many things you could just do with a single real syscall.

 (3) Hierarchy? So what? You can't really represent a set of nested keyrings
     as a set of nested directories since keys can have multiple
     parents. Besides, keys don't make a single tree.

     You'd have to represent links in a keyring as symlinks, but when creating
     them, how do you interpret the argument given to symlink()?

 (4) I suspect a filesystem API will be a lot heavier on code and memory.

 (5) Text parsers are required in a number of cases.

 (6) Using a filesystem API throws more locks into the works.

The downsides of adding a number of new syscalls are:

 (1) glibc, uclibc, etc

 (2) 32->64 bit syscall translation

 (3) Adding the new syscalls to all archs

However, using prctl(), I just about get all three for free:-)

But are they really greater than the downsides of trying to do it through the
filesystem API to which doesn't really map, and which must be abused?

David

Re: [PATCH] implement in-kernel keys & keyring management

[Greg KH]

On Sat, Aug 07, 2004 at 01:17:58AM -0700, Andrew Morton wrote:
> - Various people are likely to get upset about this:
> 
>   asmlinkage long sys_keyctl(int option, unsigned long arg2, unsigned long arg3,
> 		   unsigned long arg4, unsigned long arg5)
> 
>   I guess the pure way to do it is to add 13 new syscalls....

I think that if the /proc interface was moved over to sysfs (which is
where it should be), a number of these syscalls would go away.

thanks,

greg k-h

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

Greg KH <greg@kroah.com> wrote:
> I think that if the /proc interface was moved over to sysfs (which is
> where it should be), a number of these syscalls would go away.

Well, I could move these two files into /sysfs. But just doing that wouldn't
get rid of any of the system calls. To move these files into sysfs, should I
create a "keys" subsystem?

Can you elaborate as to what you envision? I wonder if you'd thinking that I
should make every key a kobject and fan-out them out in a directory in sysfs
somewhere. I really don't want to do that, though... kobject seems to add
quite a large overhead that I'd rather avoid (a directory in sysfs for
instance).

I could a keyfs filesystem, fan the keys out in there, but this would spawn
more code than just a few new syscalls or prctls. However, I can't just
pretend all keyrings are directories and all keys files and then use link()
and unlink(). I'd need to be able to link() and unlink() directories. I could
do it by representing two keyrings, as two adjacent directories, and then use
symlink() to create a link between them.

The main advantage of doing this, however, is that shell scripts would be able
to modify their own keyrings without a utility program such as keyctl.c that I
put up for download.

David

Re: [PATCH] implement in-kernel keys & keyring management

[Greg KH]

On Mon, Aug 09, 2004 at 10:23:20AM +0100, David Howells wrote:
> 
> Greg KH <greg@kroah.com> wrote:
> > I think that if the /proc interface was moved over to sysfs (which is
> > where it should be), a number of these syscalls would go away.
> 
> Well, I could move these two files into /sysfs. But just doing that wouldn't
> get rid of any of the system calls. To move these files into sysfs, should I
> create a "keys" subsystem?

Yes.  But then you would have to split the info in these files up into
many different files, as it's "one value per file" for sysfs files :)

> Can you elaborate as to what you envision? I wonder if you'd thinking that I
> should make every key a kobject and fan-out them out in a directory in sysfs
> somewhere. I really don't want to do that, though... kobject seems to add
> quite a large overhead that I'd rather avoid (a directory in sysfs for
> instance).

James has gone into the detail of a filesystem type interface for this
code much better than I can envision.

thanks,

greg k-h

Re: [PATCH] implement in-kernel keys & keyring management

[Trond Myklebust]

På fr , 06/08/2004 klokka 17:31, skreiv David Howells:
>  - Two std keyrings per user: per-user and default-user-session

So what *is* the reason for the "per-user" and "default-user-session"
keys?

In a strong authentication model, a setuid process should not normally
find itself automatically authenticated to a new set of keys.

Cheers,
  Trond

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

Trond Myklebust <trond.myklebust@fys.uio.no> wrote
> >  - Two std keyrings per user: per-user and default-user-session
> 
> So what *is* the reason for the "per-user" and "default-user-session"
> keys?

Well, Linus wants a user keyring. Originally I was going to just include this
in the "search path" for keyring search.

However, it has become obvious that it's necessary to be able to exclude the
user keyring from the search path under certain circumstances, so I thought
why not add it to the session key, so that it is accessible via keyring
nesting.

However, I think it's still necessary to show some separation between the
session and the user keyrings, and unless PAM or something is made use of, you
won't get that if each process uses the user keyring as its session by
default.

It also allows me to add a facility by which a process can duplicate its
session keyring and subscribe to the new one instead, whilst retaining a link
to the real user keyring.

Of course, this is open to negotiation. I'm not entirely convinced I need it,
but it seemed right at the time.

> In a strong authentication model, a setuid process should not normally
> find itself automatically authenticated to a new set of keys.

This only happens when a process subscribed to the root default-user-session
keyring sets the UID to something non-zero. Execution of a SUID binary does
not change the session keyring.

The idea was that non-root users will probably want their own session keyring,
not root's.

I can think of several other ways of dealing with this:

 (*) Let userspace (PAM) determine the session.

 (*) Start with no session keyring set on any process, and only attach a
     process to the default-user-session when someone tries to access their
     session keyring if there isn't one set, or when a setuid() is performed
     (or maybe setsid()?).

 (*) Have an init-specific session keyring as well.

David

Re: [PATCH] implement in-kernel keys & keyring management

[James Morris]

On Sat, 7 Aug 2004, David Howells wrote:

> I've made available a patch that does a better job of key and keyring
> management for authentication, cryptography, etc.. I've added a good bit of
> documentation and I've commented the code more thoroughly.

Here's some more feedback:

 typedef int32_t key_serial_t;

Why is this signed?  And does this really need to be a typedef? (Do you 
forsee it ever changing from 32-bit?).


For consistency, request_key(), validate_key() and lookup_key() should 
probably be of the form key_request() etc.  There are other similar 
cases throughout the code.


I would suggest that the /sbin/request-key interface be done via Netlink
messaging instead.  The kernel would generate key create and key update
messages, to which userpace daemons can respond (similar to e.g. pfkey
acquire).  I think these messages need to be tagged with the key 'type',
so that the userspace code knows what to generate keys for.


  #define sys_keyctl(o,b,c,d,e)          (-EINVAL)

This should probably be -ENOSYS.


-                   capable(CAP_SETGID))
+                   capable(CAP_SETGID)) {
                        new_egid = egid;
+               }

This looks superfluous.


We need to look at the implications for LSM, e.g. keys have Unix style
access control information attached, and LSM apps may want to extend this
to other security models.  Some of the user interface calls may also need
to be mediated via LSM.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH] implement in-kernel keys & keyring management

[Linus Torvalds]

On Sun, 8 Aug 2004, James Morris wrote:
> 
> I would suggest that the /sbin/request-key interface be done via Netlink
> messaging instead.  The kernel would generate key create and key update
> messages, to which userpace daemons can respond (similar to e.g. pfkey
> acquire).  I think these messages need to be tagged with the key 'type',
> so that the userspace code knows what to generate keys for.

I really disagree. 

If you want to use netlink, do so. But do it from the /sbin/request-key
script or binary.

I've never seen a good binary deamon listening for things. It's a horrible 
interface. It's undebuggable, it's inflexible, and it's just plain nasty. 

I'm just looking at how _well_ /sbin/hotplug has worked out. I believe 
that it would have been a disaster done with a binary messaging setup.

		Linus

Re: [PATCH] implement in-kernel keys & keyring management

[James Morris]

On Sat, 7 Aug 2004, Linus Torvalds wrote:

> On Sun, 8 Aug 2004, James Morris wrote:
> > 
> > I would suggest that the /sbin/request-key interface be done via Netlink
> > messaging instead.  

> I really disagree. 
> 
> If you want to use netlink, do so. But do it from the /sbin/request-key
> script or binary.
> 
> I've never seen a good binary deamon listening for things. It's a horrible 
> interface. It's undebuggable, it's inflexible, and it's just plain nasty. 
> 
> I'm just looking at how _well_ /sbin/hotplug has worked out. I believe 
> that it would have been a disaster done with a binary messaging setup.

I'm not disagreeing with the above, but what about performance?  Part of
the reason I suggested Netlink is that it's likely to be more efficient to
send messages over a socket than to exec a program for each key request
from the kernel.

It's difficult to know if performance will actually be an issue without
understanding the potential workload more.  What if many thousands of
clients are connected to a fileserver?  Would calling /sbin/request-key
for each key request be likely to cause performance problems?


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH] implement in-kernel keys & keyring management

[Linus Torvalds]

On Sun, 8 Aug 2004, James Morris wrote:
> 
> I'm not disagreeing with the above, but what about performance?  Part of
> the reason I suggested Netlink is that it's likely to be more efficient
> to send messages over a socket than to exec a program for each key
> request from the kernel.

Yes. However, I don't see that the kernel really would ask for new keys 
very often.  Any normal operation is that you have the key already.

> It's difficult to know if performance will actually be an issue without
> understanding the potential workload more.  What if many thousands of
> clients are connected to a fileserver?  Would calling /sbin/request-key
> for each key request be likely to cause performance problems?

The fileserver generally is the one that _asks_ for keys, not the other 
way around.

So the "I don't have a key" case is more of an issue where somebody tries 
to mount an encrypted filesystem, and the filesystem says "you don't have 
a key".

It's not a thing like "you tried to open a file" that happens thousands of
times a second - that one would get an EACCES if you don't have a key. 

It would be more like "the mount binary needs a key to mount this volume, 
so let's request that key first".

David, have you actually coded up something that uses the user callback, 
maybe you can describe a realistic schenario...

But at least to me, the /sbin/request-key thing is more like loading a 
module. You might do it to mount a filesystem or read an encrypted volume, 
but you wouldn't do it in the "normal" workload. It's a major event.

		Linus

Re: [PATCH] implement in-kernel keys & keyring management

[bert hubert]

On Sun, Aug 08, 2004 at 09:27:15PM -0700, Linus Torvalds wrote:

> Yes. However, I don't see that the kernel really would ask for new keys 
> very often.  Any normal operation is that you have the key already.

Key might not be there though, leading to many repeated requests. Three
points:

1) A netlink binary "server" looks a hell of a lot like a nameserver, and
those are a roaring success in terms of stability and performance. When
considering nameservers other than bind, I'd also add security and leanness
to that list.

2) One can also send text over datagrams (think SIP)

3) Debugging netlink communications is actually not that hard as other
processes can listen in on netlink communications given certain settings,
think 'netlinkdump'. Especially easy when doing ASCII over netlink!

Bert.

-- 
http://www.PowerDNS.com      Open source, database driven DNS Software 
http://lartc.org           Linux Advanced Routing & Traffic Control HOWTO

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:
> So the "I don't have a key" case is more of an issue where somebody tries 
> to mount an encrypted filesystem, and the filesystem says "you don't have 
> a key".

Or in a network filesystem, such as AFS, CIFS or SMB where the key holds the
user identity mapping - perhaps a Kerberos ticket.

What I envision for AFS is that there'll be a TGT somewhere on the system,
perhaps in another key, and request-key will invoke a request to the kerberos
server to get an AFS ticket based on that TGT.

> It's not a thing like "you tried to open a file" that happens thousands of
> times a second - that one would get an EACCES if you don't have a key. 

Well, each open call would incur a key search, but I've half taken care of the
problem. Adding the concept of a limited-duration negative key would, I think,
take care of the rest.

> It would be more like "the mount binary needs a key to mount this volume, 
> so let's request that key first".
> 
> David, have you actually coded up something that uses the user callback, 
> maybe you can describe a realistic schenario...

Well:

	keyctl request <type> <description> <keyring>

Can be used to drive it at the moment, so that I can check that it works.

The scenario I'm mainly interested in involves kAFS/OpenAFS with Kerberos or
other authentication (see above). But I talked to a number of people at OLS
who also expressed an interest.

I need to work on my kAFS client at some point soon to get that to use it, but
that'll be a couple of weeks at least, I think.

> But at least to me, the /sbin/request-key thing is more like loading a 
> module. You might do it to mount a filesystem or read an encrypted volume, 
> but you wouldn't do it in the "normal" workload. It's a major event.

With AFS + Krb, you currently make the key available in advance (using the
aklog program) and any request for which you don't have a key fails with some
error or other.

You can't really do it at mount time in this case because you may have more
than one user on the system, and all of them may need to share the same
mountpoints (some people put /usr/ on AFS, for example).

David

Re: [PATCH] implement in-kernel keys & keyring management

[Alan Cox]

On Llu, 2004-08-09 at 05:27, Linus Torvalds wrote:
> But at least to me, the /sbin/request-key thing is more like loading a 
> module. You might do it to mount a filesystem or read an encrypted volume, 
> but you wouldn't do it in the "normal" workload. It's a major event.

The BSD networking PF_KEY does exactly this for IPsec sockets. Coupled
with a cache it seems to work rather well. In fact is there a reason for
not using PF_KEY - other than /sbin/hotplug being cleaner ?

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

James Morris <jmorris@redhat.com>
> I'm not disagreeing with the above, but what about performance?  Part of
> the reason I suggested Netlink is that it's likely to be more efficient to
> send messages over a socket than to exec a program for each key request
> from the kernel.
> 
> It's difficult to know if performance will actually be an issue without
> understanding the potential workload more.  What if many thousands of
> clients are connected to a fileserver?  Would calling /sbin/request-key
> for each key request be likely to cause performance problems?

I have put in one thing to mitigate this problem. There's a construction
queue. If a user has a key under userspace construction with a particular type
and description, then anyone else who wants the same key will have to wait for
the key construction to complete, and then repeat their search.

The assumption is that the constructor will instantiate the key and link it
into one of the calling process's keyrings (probably the session
keyring). This then acts as a cache, where subsequent key accesses should
hopefully find it.

However, there are two issues this.

 (1) Key lookup failure: I think that maybe I need to add the concept of a
     "negative" key.

     Key search would then proceed in this manner: the keyring tree is
     searched for a positive key, terminating the search when one is
     found. Any matching negative keys are noted in passing, but nothing is
     done about them until the search fails; in which case an error will be
     returned immediately rather than going to userspace.

 (2) What if the new key is not made available to the next process that wants
     it, perhaps because it's in a different session? Should it be possible
     for the constructor to say "don't use this key, use that one instead",
     and substitute an existing key from its cache?

David

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

Linus Torvalds <torvalds@osdl.org> wrote:
> If you want to use netlink, do so. But do it from the /sbin/request-key
> script or binary.

I've rewritten my example request-key program to get a key holding the
real key creation program from a key in the user's session key, so that, say,
a desktop such as KDE could supply a GUI front end.

	http://people.redhat.com/~dhowells/keys/request-key.c
	http://people.redhat.com/~dhowells/keys/request-key-dhowells.sh

So as me, I can do:

	keyctl add user request-key:create /tmp/request-key-dhowells.sh @s

And then:

	keyctl request user metal:copper

And the request-key program will change to my UID/GID, look around for the a
key telling it which program to run, and then run my default script.

David

[PATCH] implement in-kernel keys & keyring management [try #4]

[David Howells]

I've updated my patch and test programs such that the concept of negative keys
is now supported. Negative keys are produced when request-key fails. They time
out after a short amount of time. They can be updated to real keys later.

	http://people.redhat.com/~dhowells/keys/keys-268rc2-4.diff.bz2
	http://people.redhat.com/~dhowells/keys/keyctl.c
	http://people.redhat.com/~dhowells/keys/request-key.c
	http://people.redhat.com/~dhowells/keys/request-key-dhowells.sh


David

Re: [PATCH] implement in-kernel keys & keyring management [try #4]

[Kyle Moffett]

One feature that appears to be missing is a key handle or key reference,
essentially a structure for manipulating a key with an additional 
permissions
mask and the ability to be both cloned and revoked.  A clone should have
the same permission mask as the parent, reducible, of course.  When a 
key
reference is revoked it also revokes all cloned references, and their 
clones,
etc.  I guess a clone should be made when passed across a UNIX pipe, as
well as when passed to a child process.  That way I can give some daemon
a key handle to do work for me, then revoke the daemon's handle only, 
not
all the other handles I may have, when I'm done with it.

Cheers,
Kyle Moffett

-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a17 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$
L++++(+++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+
PGP+++ t+(+++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r  
!y?(-)
------END GEEK CODE BLOCK------

[PATCH] implement in-kernel keys & keyring management [try #5]

[David Howells]

I've modified my patch to avoid a locking error that Al Viro pointed out. If
you have two keyrings A and B, if one process tries to link A to B, whilst in
parallel another process tries to link B to A, it could end up creating a
cycle in the graph. I've added code to serialise link calls with respect to
one another to obviate that problem.

It can be found at:

	http://people.redhat.com/~dhowells/keys/keys-268rc2-5.diff.bz2

David.

Re: [PATCH] implement in-kernel keys & keyring management [try #5]

[Chris Wright]

Hi David,

* David Howells (dhowells@redhat.com) wrote:
> It can be found at:
> 
> 	http://people.redhat.com/~dhowells/keys/keys-268rc2-5.diff.bz2

Here's a few comments/questions from first pass-thru.  Looks good so
far.  I have yet to fully digest it all, or run your current
patchset/toolset to play with it.

it's still tough to walk away from the idea that this is really close to
a filesystem interface.
                                                                                
sys_keyctl prototype declaration in key.h?  maybe in syscalls.h?
also, the #else /* !CONFIG_KEYS */ for doesn't look right.  i think
the syscall should always be there, instead use cond_syscall.
                                                                                
prctl case statement.  are case ranges supported in all gcc versions
that are valid for 2.6 kernel compilation?
                                                                                
key_euid_changed/key_egid_changed will change the process_keyring even
if it's just a thread which did the setuid/gid.  this doesn't sound
right.
                                                                                
i'm a little confused by suid_keys().  it's sprinkled in various spots,
yet the function does nothing.  what's the intention there (esp. w.r.t.
key_euid_changed)?  e.g.  placement in compute_creds is before the actual
process uid updates are done.
                                                                                
why check pid ==1 and uid ==0 in exec_keys?
                                                                                
why not switch_uid_keyring for non root_session_keyrings?
                                                                                
in alloc_uid_keyring, keyring_alloc failure for session_keyring leaks
uid_keyring allocation.
                                                                                
key_user can't be squished into user_struct?  seems like the quota stuff
could become setable from userspace (rlimit-like).
                                                                                
/sbin/request-key should probably path configurable like /sbin/hotplug.

thanks,
-chris

Re: [PATCH] implement in-kernel keys & keyring management [try #5]

[David Howells]

Chris Wright <chrisw@osdl.org> wrote:
> Here's a few comments/questions from first pass-thru.  Looks good so
> far.  I have yet to fully digest it all, or run your current
> patchset/toolset to play with it.
> 
> it's still tough to walk away from the idea that this is really close to
> a filesystem interface.

It doesn't map especially well into a filesystem. I'm trying to implement a
keyfs, but there are some problems with doing so. Some of these problems can
be avoided by making keys into inodes in keyfs, but I want to keep memory
usage down. Inodes are way overkill. Basically doing that would make it into a
ramfs of sorts.

> sys_keyctl prototype declaration in key.h?  maybe in syscalls.h?
> also, the #else /* !CONFIG_KEYS */ for doesn't look right.  i think
> the syscall should always be there, instead use cond_syscall.

Currently sys_keyctl() is called from prctl() rather than having a syscall of
its own, or fanning the various functions out into separate syscalls. This
means I don't need to modify glibc, all the kernel arch dirs, and I don't need
to provide 32->64 bit conversion, since what's already done for prctl() is
sufficient.

If Andrew is shows a willingness to take the patch, I'd be willing to break
out the syscalls at that point if it's deemed necessary. I suppose I could do
that now, but only provide it on x86, and leave syscall addition to the
various arch maintainers.

Actually, doing that might be a good idea anyway... If I break key creation
out, at least, I can clean up its interface so that I don't have to store the
payload length in with the data.

> prctl case statement.  are case ranges supported in all gcc versions
> that are valid for 2.6 kernel compilation?

Shhh! :-)

I don't know, but Andrew hasn't complained yet, so I guess it must be:-)

> key_euid_changed/key_egid_changed will change the process_keyring even
> if it's just a thread which did the setuid/gid.  this doesn't sound
> right.

I know. If I don't do it, then a single-threaded process will lose access to
its process keyring if it changes process UID/GID; it could change its keyring
ownership manually, and it probably shouldn't be allowed to do that if it
doesn't have root privileges.

> i'm a little confused by suid_keys().  it's sprinkled in various spots,
> yet the function does nothing.  what's the intention there (esp. w.r.t.
> key_euid_changed)?  e.g.  placement in compute_creds is before the actual
> process uid updates are done.

It originally set up new keyrings for or cleared the thread, process and
session keyrings. However, it seems that this isn't a good idea. I've left the
hooks in in case someone comes up with a good strategy.

> why check pid ==1 and uid ==0 in exec_keys?

Well, the init process starts off without any keyrings attached. If it doesn't
have a session keyring attached and it's the init process exec'ing itself, it
automatically gets subscribed to the root keyring.

An alternative way of doing things that might work better, is to start off by
giving init no session keyring, and only force subscription to one when an
attempt is made to use it; and at that point subscribe the process to the
default session ring for its owning user.

> why not switch_uid_keyring for non root_session_keyrings?

I'd like login to pick up the user's keyring automatically if the user isn't
root and the process's current keyring is root's default without having to
bother PAM.

If I implement what I suggested above, this would get fixed too, I think.

> in alloc_uid_keyring, keyring_alloc failure for session_keyring leaks
> uid_keyring allocation.

Hmmm... you're right. I'd made the assumption that the caller would clean up,
but that isn't true, is it?

I've fixed that in my tree.

> key_user can't be squished into user_struct?  seems like the quota stuff
> could become setable from userspace (rlimit-like).

No:

 (1) A key has to pin the quota tracking struct in memory so that a user can't
     gain more quota (a key or keyring contributing to a user's quota could
     have been linked to by someone else).

 (2) The quota tracking information has to persist as long as any keys that
     contribute to it do.

 (3) user_struct pins two keyrings. These keyrings, and possibly keys they
     contain pin the quota tracking struct.

 (4) If user_struct was the quota tracking struct, you'd end up with a
     reference cycle, meaning that the user_struct and its associated baggage
     could never be discarded.

I could make it so that if a user_struct is discarded, then every key owned by
that user is withdrawn from the system, but that's not necessarily easy or
desirable.

It might make sense to split the user_struct into two: a statistics record and
a struct full of pointers that can pin things, including the statistics
record.

> /sbin/request-key should probably path configurable like /sbin/hotplug.

Yeah... That's an optimisation for later, I think. If Andrew or Linus takes
that patch, I'll fix that up.

David

[PATCH] implement in-kernel keys & keyring management [try #6]

[David Howells]

Hi Linus, Andrew,

> I've modified my patch to avoid a locking error that Al Viro pointed out. If
> you have two keyrings A and B, if one process tries to link A to B, whilst
> in parallel another process tries to link B to A, it could end up creating a
> cycle in the graph. I've added code to serialise link calls with respect to
> one another to obviate that problem.

I've fixed another looking bug that that introduced.

Following a reply from Chris Wright, I've also sorted out session keyring
management to what is probably a more sane approach. Processes now start off
without session keyrings being assigned. A process is automatically subscribed
to the user's default session if it doesn't have a session keyring when it
tries to access it. Processes inherit their parents session keyring or lack
thereof at fork time. Processes can still manually join a session.

The patch can be found at:

	http://people.redhat.com/~dhowells/keys/keys-268rc2-6.diff.bz2

David.

[PATCH] keys & keyring management: key filesystem

[David Howells]

Here's a preliminary look a patch I've concocted to make keys accessible
through yet another filesystem (TM). At the moment, it only gives a read-only
view onto the keys in the system.

The patch can be found at:

	http://people.redhat.com/~dhowells/keys/keyfs-268rc2.diff.bz2

It requires the keys-268rc2-6.diff.bz2 patch to supply the basic key services.


Currently, it makes a filesystem that looks like:

    /keyfs/
	thread		[symlink to process's thread key]
	process		[symlink to process's process key]
	session		[symlink to process's session key]
	user		[symlink to user key for process's UID]
	user-session	[symlink to def session key for process's UID]
	<key>/		[directory representing a key]
	    type	[file holding key type as a string]
	    description	[file holding key description as a string]
	    expiry	[file holding key expiry time as a decimal string]
	    perm	[file holding key access mask as an octal string]
	    payload	[file permitting reading/writing of key's payload data]
	<key>/		[directory representing a keyring]
	    type	[file holding key type as a string]
	    description	[file holding key description as a string]
	    expiry	[file holding key expiry time as a decimal string]
	    perm	[file holding key access mask as an octal string]
	    keyring/	[directory representing a keyring's contents]
		<key>	[symlink to linked keys]


I've encountered some problems:

 (1) readdir() on the root directory has to run in O(N^2) time because the key
     tree lock is a spinlock and the filldir call is permitted to sleep.

     I could solve this by making the spinlock into a semaphore, and just
     read-locking it for the duration of the whole readdir call. But this has
     other problems: it'll exclude any key changes for the duration,
     especially if calling filldir causes it to swap a lot.

     I don't think global enumeration is necessarily a good idea. /proc/keys
     is a debugging interface, to be made root readable only at some point. I
     don't think you should be able to _see_ any keys you don't have any
     rights to. Do people disagree with that?

 (2) It weakens security to some extent: it makes keys more accessible, more
     visible.

     I could, perhaps, deal with this by filtering the keys in readdir() and
     lookup() on the root directory.

 (3) The key permissions mask doesn't map easily to the usual RWX bits on a
     file with their strictly VFS meaning. Whilst on a key I'm using the 'X'
     bits to indicate search permission, that can't directly control execute
     permission on the key directory.

 (4) Accessing keys this way requires more syscall invocations, more locks to
     be dealt with, etc. than the syscall approach. It's a lot less efficient,
     particularly when it comes to producing a list of the same data seen in
     /proc/keys.

 (5) The kernel filesystem side of this is already much bigger than the
     syscall/prctl approach, even when it's read-only.

 (6) d_revalidate and getattr need to be supplied for any file or directory
     that relates to a particular key to pick up UID/GID/permissions changes.

     This is difficult to avoid. Each key has to be represented by several
     inodes, and they all need to be kept in sync.


I still haven't seen any really good arguments why I should get rid of all the
syscalls/prctls and drive it from userspace _entirely_ through a keyfs. We
seem to be going a little overboard with the "everything must be a filesystem
approach".

I could perhaps make all the key syscalls ioctls on a control file placed in
the root directory:-)

David

Re: [PATCH] implement in-kernel keys & keyring management

[David Howells]

James Morris <jmorris@redhat.com> wrote:
> Here's some more feedback:
> 
>  typedef int32_t key_serial_t;
> 
> Why is this signed?

So I can have special values that are negative. I suppose it doesn't really
matter - they could be small positive numbers or something, but then if I want
to add one later, you get the possibility of overlap on a userspace that
supports one running with a kernel that doesn't.

> And does this really need to be a typedef? (Do you forsee it ever changing
> from 32-bit?).

No... but then 640KB of memory is enough for anyone, right? :-)

> For consistency, request_key(), validate_key() and lookup_key() should 
> probably be of the form key_request() etc.  There are other similar 
> cases throughout the code.

Maybe. Though I think request_key() should follow the form of similar
functions inside the kernel, such as request_firmware().

> I would suggest that the /sbin/request-key interface be done via Netlink
> messaging instead.

Other people argued the exact opposite first.

> 
>   #define sys_keyctl(o,b,c,d,e)          (-EINVAL)
> 
> This should probably be -ENOSYS.

If it becomes a real syscall rather than being a subset of prctl(), then yes.

> -                   capable(CAP_SETGID))
> +                   capable(CAP_SETGID)) {
>                         new_egid = egid;
> +               }
> 
> This looks superfluous.

Yes. I had added an additional statement into there at one point.

> We need to look at the implications for LSM, e.g. keys have Unix style
> access control information attached, and LSM apps may want to extend this
> to other security models.  Some of the user interface calls may also need
> to be mediated via LSM.

True. I don't know much about LSM though.

David