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

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

[Arjan van de Ven]

On Tue, 2004-10-19 at 10:58, Linux Kernel Mailing List wrote:
> ChangeSet 1.2260, 2004/10/19 07:58:51-07:00, dhowells@redhat.com

I thought there was a goal to not add multiplexer syscalls anymore,
especially not without thinking about 32/64 bit emulation first...
sounds like it's best to back out this changeset until that bit is
resolved (I thought the agreement was that this one was stuck in -mm
until this got sorted out in the first place)

[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?

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

[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:
> 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

[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

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