Message-Id: <1196887108.16006.121.camel@moss-spartans.epoch.ncsc.mil>
Date:	Wed, 05 Dec 2007 15:38:28 -0500
From:	Stephen Smalley <sds@...ho.nsa.gov>
To:	casey@...aufler-ca.com
Cc:	David Howells <dhowells@...hat.com>, viro@....linux.org.uk,
	hch@...radead.org, Trond.Myklebust@...app.com,
	linux-kernel@...r.kernel.org, selinux@...ho.nsa.gov,
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH 4/7] KEYS: Add keyctl function to get a security label

On Wed, 2007-12-05 at 12:12 -0800, Casey Schaufler wrote:
> --- David Howells <dhowells@...hat.com> wrote:
> 
> > Add a keyctl() function to get the security label of a key.
> > 
> > The following is added to Documentation/keys.txt:
> > 
> >  (*) Get the LSM security context attached to a key.
> > 
> > 	long keyctl(KEYCTL_GET_SECURITY, key_serial_t key, char *buffer,
> > 		    size_t buflen)
> > 
> >      This function returns a string that represents the LSM security context
> >      attached to a key in the buffer provided.
> > 
> >      Unless there's an error, it always returns the amount of data it could
> >      produce, even if that's too big for the buffer, but it won't copy more
> >      than requested to userspace. If the buffer pointer is NULL then no copy
> >      will take place.
> > 
> >      A NUL character is included at the end of the string if the buffer is
> >      sufficiently big.  This is included in the returned count.  If no LSM is
> >      in force then an empty string will be returned.
> > 
> >      A process must have view permission on the key for this function to be
> >      successful.
> > 
> > Signed-off-by: David Howells <dhowells@...hat.com>
> > ---
> > 
> >  Documentation/keys.txt   |   21 +++++++++++++++
> >  include/linux/keyctl.h   |    1 +
> >  include/linux/security.h |   20 +++++++++++++-
> >  security/dummy.c         |    8 ++++++
> >  security/keys/compat.c   |    3 ++
> >  security/keys/keyctl.c   |   66
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  security/security.c      |    5 +++
> >  security/selinux/hooks.c |   21 +++++++++++++--
> >  8 files changed, 141 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/keys.txt b/Documentation/keys.txt
> > index b82d38d..be424b0 100644
> > --- a/Documentation/keys.txt
> > +++ b/Documentation/keys.txt
> > @@ -711,6 +711,27 @@ The keyctl syscall functions are:
> >       The assumed authoritative key is inherited across fork and exec.
> >  
> >  
> > + (*) Get the LSM security context attached to a key.
> > +
> > +	long keyctl(KEYCTL_GET_SECURITY, key_serial_t key, char *buffer,
> > +		    size_t buflen)
> > +
> > +     This function returns a string that represents the LSM security context
> > +     attached to a key in the buffer provided.
> > +
> > +     Unless there's an error, it always returns the amount of data it could
> > +     produce, even if that's too big for the buffer, but it won't copy more
> > +     than requested to userspace. If the buffer pointer is NULL then no copy
> > +     will take place.
> > +
> > +     A NUL character is included at the end of the string if the buffer is
> > +     sufficiently big.  This is included in the returned count.  If no LSM
> > is
> > +     in force then an empty string will be returned.
> > +
> > +     A process must have view permission on the key for this function to be
> > +     successful.
> > +
> > +
> >  ===============
> >  KERNEL SERVICES
> >  ===============
> > diff --git a/include/linux/keyctl.h b/include/linux/keyctl.h
> > index 3365945..656ee6b 100644
> > --- a/include/linux/keyctl.h
> > +++ b/include/linux/keyctl.h
> > @@ -49,5 +49,6 @@
> >  #define KEYCTL_SET_REQKEY_KEYRING	14	/* set default request-key keyring */
> >  #define KEYCTL_SET_TIMEOUT		15	/* set key timeout */
> >  #define KEYCTL_ASSUME_AUTHORITY		16	/* assume request_key() authorisation */
> > +#define KEYCTL_GET_SECURITY		17	/* get key security label */
> >  
> >  #endif /*  _LINUX_KEYCTL_H */
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index ac05083..8d9e946 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -959,6 +959,17 @@ struct request_sock;
> >   *	@perm describes the combination of permissions required of this key.
> >   *	Return 1 if permission granted, 0 if permission denied and -ve it the
> >   *      normal permissions model should be effected.
> > + * @key_getsecurity:
> 
> If I contradict what I said last time around, well, I've seen
> more of what's going on since then. (insert smiley here)
> 
> I think that this should be key_getsecctx(). That would get you
> what you need from SELinux and Smack. Unfortunately, there is no
> precedent for interfaces that return the secctx, while the are
> places in networking and audit (audit needs to be converted off
> of SELinux calls to LSM calls) that call getsecid() interfaces
> and later use security_secid_to_secctx() to get the "printable"
> version. So, key_getsecid() is the interface to introduce if you
> want to be consistant, and pass the result to
> security_secid_to_secctx() to get the data you're looking for.

inode_getsecurity and getprocattr directly return the strings.
Admittedly, the whole interface could be cleaned up and made far more
consistent, but I don't think he necessarily has to go through the
getsecid + secid_to_secctx sequence if he only wants the secctx.

> > + *	Get a textual representation of the security context attached to a key
> > + *	for the purposes of honouring KEYCTL_GETSECURITY.  This function
> > + *	allocates the storage for the NUL-terminated string and the caller
> > + *	should free it.
> > + *	@key points to the key to be queried.
> > + *	@_buffer points to a pointer that should be set to point to the
> > + *	 resulting string (if no label or an error occurs).
> > + *	Return the length of the string (including terminating NUL) or -ve if
> > + *      an error.
> > + *	May also return 0 (and a NULL buffer pointer) if there is no label.
> >   *
> >   * Security hooks affecting all System V IPC operations.
> >   *
> > @@ -1437,7 +1448,7 @@ struct security_operations {
> >  	int (*key_permission)(key_ref_t key_ref,
> >  			      struct task_struct *context,
> >  			      key_perm_t perm);
> > -
> > +	int (*key_getsecurity)(struct key *key, char **_buffer);
> >  #endif	/* CONFIG_KEYS */
> >  
> >  };
> > @@ -2567,6 +2578,7 @@ int security_key_alloc(struct key *key, struct
> > task_struct *tsk, unsigned long f
> >  void security_key_free(struct key *key);
> >  int security_key_permission(key_ref_t key_ref,
> >  			    struct task_struct *context, key_perm_t perm);
> > +int security_key_getsecurity(struct key *key, char **_buffer);
> >  
> >  #else
> >  
> > @@ -2588,6 +2600,12 @@ static inline int security_key_permission(key_ref_t
> > key_ref,
> >  	return 0;
> >  }
> >  
> > +static inline int security_key_getsecurity(struct key *key, char **_buffer)
> > +{
> > +	*_buffer = NULL;
> > +	return 0;
> > +}
> > +
> >  #endif
> >  #endif /* CONFIG_KEYS */
> >  
> > diff --git a/security/dummy.c b/security/dummy.c
> > index 6d895ad..7993b30 100644
> > --- a/security/dummy.c
> > +++ b/security/dummy.c
> > @@ -949,6 +949,13 @@ static inline int dummy_key_permission(key_ref_t
> > key_ref,
> >  {
> >  	return 0;
> >  }
> > +
> > +static int dummy_key_getsecurity(struct key *key, char **_buffer)
> > +{
> > +	*_buffer = NULL;
> > +	return 0;
> > +}
> > +
> >  #endif /* CONFIG_KEYS */
> >  
> >  struct security_operations dummy_security_ops;
> > @@ -1133,6 +1140,7 @@ void security_fixup_ops (struct security_operations
> > *ops)
> >  	set_to_dummy_if_null(ops, key_alloc);
> >  	set_to_dummy_if_null(ops, key_free);
> >  	set_to_dummy_if_null(ops, key_permission);
> > +	set_to_dummy_if_null(ops, key_getsecurity);
> >  #endif	/* CONFIG_KEYS */
> >  
> >  }
> > diff --git a/security/keys/compat.c b/security/keys/compat.c
> > index e10ec99..c766c68 100644
> > --- a/security/keys/compat.c
> > +++ b/security/keys/compat.c
> > @@ -79,6 +79,9 @@ asmlinkage long compat_sys_keyctl(u32 option,
> >  	case KEYCTL_ASSUME_AUTHORITY:
> >  		return keyctl_assume_authority(arg2);
> >  
> > +	case KEYCTL_GET_SECURITY:
> > +		return keyctl_get_security(arg2, compat_ptr(arg3), arg4);
> > +
> >  	default:
> >  		return -EOPNOTSUPP;
> >  	}
> > diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c
> > index 1698bf9..56e963b 100644
> > --- a/security/keys/keyctl.c
> > +++ b/security/keys/keyctl.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/string.h>
> >  #include <linux/err.h>
> >  #include <linux/vmalloc.h>
> > +#include <linux/security.h>
> >  #include <asm/uaccess.h>
> >  #include "internal.h"
> >  
> > @@ -1080,6 +1081,66 @@ error:
> >  
> >  } /* end keyctl_assume_authority() */
> >  
> > +/*
> > + * get the security label of a key
> > + * - the key must grant us view permission
> > + * - if there's a buffer, we place up to buflen bytes of data into it
> > + * - unless there's an error, we return the amount of information available,
> > + *   irrespective of how much we may have copied (including the terminal
> > NUL)
> > + * - implements keyctl(KEYCTL_GET_SECURITY)
> > + */
> > +long keyctl_get_security(key_serial_t keyid,
> > +			 char __user *buffer,
> > +			 size_t buflen)
> > +{
> > +	struct key *key, *instkey;
> > +	key_ref_t key_ref;
> > +	char *context;
> > +	long ret;
> > +
> > +	key_ref = lookup_user_key(NULL, keyid, 0, 1, KEY_VIEW);
> > +	if (IS_ERR(key_ref)) {
> > +		if (PTR_ERR(key_ref) != -EACCES)
> > +			return PTR_ERR(key_ref);
> > +
> > +		/* viewing a key under construction is also permitted if we
> > +		 * have the authorisation token handy */
> > +		instkey = key_get_instantiation_authkey(keyid);
> > +		if (IS_ERR(instkey))
> > +			return PTR_ERR(key_ref);
> > +		key_put(instkey);
> > +
> > +		key_ref = lookup_user_key(NULL, keyid, 0, 1, 0);
> > +		if (IS_ERR(key_ref))
> > +			return PTR_ERR(key_ref);
> > +	}
> > +
> > +	key = key_ref_to_ptr(key_ref);
> > +	ret = security_key_getsecurity(key, &context);
> > +	if (ret == 0) {
> > +		/* if no information was returned, give userspace an empty
> > +		 * string */
> > +		ret = 1;
> > +		if (buffer && buflen > 0 &&
> > +		    copy_to_user(buffer, "", 1) != 0)
> > +			ret = -EFAULT;
> > +	} else if (ret > 0) {
> > +		/* return as much data as there's room for */
> > +		if (buffer && buflen > 0) {
> > +			if (buflen > ret)
> > +				buflen = ret;
> > +
> > +			if (copy_to_user(buffer, context, buflen) != 0)
> > +				ret = -EFAULT;
> > +		}
> > +
> > +		kfree(context);
> > +	}
> > +
> > +	key_ref_put(key_ref);
> > +	return ret;
> > +}
> > +
> > 
> >
> /*****************************************************************************/
> >  /*
> >   * the key control system call
> > @@ -1160,6 +1221,11 @@ asmlinkage long sys_keyctl(int option, unsigned long
> > arg2, unsigned long arg3,
> >  	case KEYCTL_ASSUME_AUTHORITY:
> >  		return keyctl_assume_authority((key_serial_t) arg2);
> >  
> > +	case KEYCTL_GET_SECURITY:
> > +		return keyctl_get_security((key_serial_t) arg2,
> > +					   (char *) arg3,
> > +					   (size_t) arg4);
> > +
> >  	default:
> >  		return -EOPNOTSUPP;
> >  	}
> > diff --git a/security/security.c b/security/security.c
> > index 0e1f1f1..16213e3 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1079,4 +1079,9 @@ int security_key_permission(key_ref_t key_ref,
> >  	return security_ops->key_permission(key_ref, context, perm);
> >  }
> >  
> > +int security_key_getsecurity(struct key *key, char **_buffer)
> > +{
> > +	return security_ops->key_getsecurity(key, _buffer);
> > +}
> > +
> >  #endif	/* CONFIG_KEYS */
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 9f3124b..bd4cfab 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4768,6 +4768,20 @@ static int selinux_key_permission(key_ref_t key_ref,
> >  			    SECCLASS_KEY, perm, NULL);
> >  }
> >  
> > +static int selinux_key_getsecurity(struct key *key, char **_buffer)
> > +{
> > +	struct key_security_struct *ksec = key->security;
> > +	char *context = NULL;
> > +	unsigned len;
> > +	int rc;
> > +
> > +	rc = security_sid_to_context(ksec->sid, &context, &len);
> > +	if (!rc)
> > +		rc = len;
> > +	*_buffer = context;
> > +	return rc;
> > +}
> > +
> >  #endif
> >  
> >  static struct security_operations selinux_ops = {
> > @@ -4943,9 +4957,10 @@ static struct security_operations selinux_ops = {
> >  #endif
> >  
> >  #ifdef CONFIG_KEYS
> > -	.key_alloc =                    selinux_key_alloc,
> > -	.key_free =                     selinux_key_free,
> > -	.key_permission =               selinux_key_permission,
> > +	.key_alloc =			selinux_key_alloc,
> > +	.key_free =			selinux_key_free,
> > +	.key_permission =		selinux_key_permission,
> > +	.key_getsecurity =		selinux_key_getsecurity,
> >  #endif
> >  };
> >  
> > 
> > 
> > 
> 
> 
> Casey Schaufler
> casey@...aufler-ca.com
> 
> --
> This message was distributed to subscribers of the selinux mailing list.
> If you no longer wish to subscribe, send mail to majordomo@...ho.nsa.gov with
> the words "unsubscribe selinux" without quotes as the message.
-- 
Stephen Smalley
National Security Agency

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists