[PATCH v2 0/4] cifs: allow multiuser mounts with authtypes besides krb5

[PATCH v2 0/4] cifs: allow multiuser mounts with authtypes besides krb5

[Jeff Layton]

This set fixes a couple of problems pointed out by Shirish, and also
adds a patch to warn about deprecating the old MultiuserMount code
in 3.5.

When the (newer) multiuser mount code was initially introduced for cifs,
I limited it to sec=krb5 auth. When a new user walks into a mount, we
have no way to prompt for a username and password from the kernel, so
the only auth type we could support was krb5.

This patchset extends the code to allow other auth types to use
multiuser mounts. The idea here is for users to put their username and
password for a particular server or domain into the keyring. The kernel
can then look for that key and use those credentials to establish a
session on the user's behalf.

Because of the quirkiness of keyring permissions, this patchset adds a
new key type that does not allow the keys to be read from userspace.
That should prevent compromise of the credentials by someone walking up
to the user's machine while she is away at lunch.

This patchset requires some changes to cifs-utils as well, to make it
use the new key_type, description and payload format. I sent that set
to the linux-cifs list earlier today.

Comments and review of both sets is appreciated...

Jeff Layton (4):
  keys: add a "secret" key type
  cifs: sanitize username handling
  cifs: fetch credentials out of keyring for non-krb5 auth multiuser
    mounts
  cifs: warn about impending deprecation of legacy MultiuserMount code

 fs/cifs/cifs_debug.c         |   11 ++-
 fs/cifs/cifs_spnego.c        |   10 ++-
 fs/cifs/cifsencrypt.c        |   11 ++-
 fs/cifs/connect.c            |  194 ++++++++++++++++++++++++++++++++++++++----
 include/keys/user-type.h     |    3 +-
 security/keys/internal.h     |    1 +
 security/keys/key.c          |    1 +
 security/keys/user_defined.c |   17 ++++
 8 files changed, 223 insertions(+), 25 deletions(-)

-- 
1.7.7.4

[PATCH v2 1/4] keys: add a "secret" key type

[Jeff Layton]

For CIFS, we want to be able to store NTLM credentials (aka username
and password) in the keyring. We do not, however want to allow users
to fetch those keys back out of the keyring since that would be a
security risk.

Unfortunately, due to the nuances of key permission bits, it's not
possible to do this. We need to grant search permissions so the kernel
can find these keys, but that also implies permissions to read the
payload.

Resolve this by adding a new key_type. This key type is essentially
the same as key_type_user, but does not define a .read op. This
prevents the payload from ever being visible from userspace.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 include/keys/user-type.h     |    3 ++-
 security/keys/internal.h     |    1 +
 security/keys/key.c          |    1 +
 security/keys/user_defined.c |   17 +++++++++++++++++
 4 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/include/keys/user-type.h b/include/keys/user-type.h
index c37c342..41b5515 100644
--- a/include/keys/user-type.h
+++ b/include/keys/user-type.h
@@ -17,7 +17,7 @@
 
 /*****************************************************************************/
 /*
- * the payload for a key of type "user"
+ * the payload for a key of type "user" or "secret"
  * - once filled in and attached to a key:
  *   - the payload struct is invariant may not be changed, only replaced
  *   - the payload must be read with RCU procedures or with the key semaphore
@@ -33,6 +33,7 @@ struct user_key_payload {
 };
 
 extern struct key_type key_type_user;
+extern struct key_type key_type_secret;
 
 extern int user_instantiate(struct key *key, const void *data, size_t datalen);
 extern int user_update(struct key *key, const void *data, size_t datalen);
diff --git a/security/keys/internal.h b/security/keys/internal.h
index c7a7cae..2784e07 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -33,6 +33,7 @@
 
 extern struct key_type key_type_dead;
 extern struct key_type key_type_user;
+extern struct key_type key_type_secret;
 
 /*****************************************************************************/
 /*
diff --git a/security/keys/key.c b/security/keys/key.c
index 4414abd..3d1d79d 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -996,6 +996,7 @@ void __init key_init(void)
 	list_add_tail(&key_type_keyring.link, &key_types_list);
 	list_add_tail(&key_type_dead.link, &key_types_list);
 	list_add_tail(&key_type_user.link, &key_types_list);
+	list_add_tail(&key_type_secret.link, &key_types_list);
 
 	/* record the root user tracking */
 	rb_link_node(&root_key_user.node,
diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
index 69ff52c..e25782f 100644
--- a/security/keys/user_defined.c
+++ b/security/keys/user_defined.c
@@ -36,6 +36,23 @@ struct key_type key_type_user = {
 EXPORT_SYMBOL_GPL(key_type_user);
 
 /*
+ * This key type is essentially the same as key_type_user, but it does
+ * not define a .read op. This is suitable for storing information in
+ * the keyring that you do not want to be readable from userspace. For
+ * instance, passwords or secret encryption keys.
+ */
+struct key_type key_type_secret = {
+	.name		= "secret",
+	.instantiate	= user_instantiate,
+	.update		= user_update,
+	.match		= user_match,
+	.revoke		= user_revoke,
+	.destroy	= user_destroy,
+	.describe	= user_describe,
+};
+EXPORT_SYMBOL_GPL(key_type_secret);
+
+/*
  * instantiate a user defined key
  */
 int user_instantiate(struct key *key, const void *data, size_t datalen)
-- 
1.7.7.4

[PATCH v2 2/4] cifs: sanitize username handling

[Jeff Layton]

Currently, it's not very clear whether you're allowed to have a NULL
vol->username or ses->user_name. Some places check for it and some don't.

Make it clear that a NULL pointer is OK in these fields, and ensure that
all the callers check for that.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_spnego.c |   10 +++++++---
 fs/cifs/cifsencrypt.c |   11 ++++++++---
 fs/cifs/connect.c     |   19 ++++++++++++-------
 3 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/cifs_spnego.c b/fs/cifs/cifs_spnego.c
index 2272fd5..e622863 100644
--- a/fs/cifs/cifs_spnego.c
+++ b/fs/cifs/cifs_spnego.c
@@ -113,9 +113,11 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo)
 		   MAX_MECH_STR_LEN +
 		   UID_KEY_LEN + (sizeof(uid_t) * 2) +
 		   CREDUID_KEY_LEN + (sizeof(uid_t) * 2) +
-		   USER_KEY_LEN + strlen(sesInfo->user_name) +
 		   PID_KEY_LEN + (sizeof(pid_t) * 2) + 1;
 
+	if (sesInfo->user_name)
+		desc_len += USER_KEY_LEN + strlen(sesInfo->user_name);
+
 	spnego_key = ERR_PTR(-ENOMEM);
 	description = kzalloc(desc_len, GFP_KERNEL);
 	if (description == NULL)
@@ -152,8 +154,10 @@ cifs_get_spnego_key(struct cifs_ses *sesInfo)
 	dp = description + strlen(description);
 	sprintf(dp, ";creduid=0x%x", sesInfo->cred_uid);
 
-	dp = description + strlen(description);
-	sprintf(dp, ";user=%s", sesInfo->user_name);
+	if (sesInfo->user_name) {
+		dp = description + strlen(description);
+		sprintf(dp, ";user=%s", sesInfo->user_name);
+	}
 
 	dp = description + strlen(description);
 	sprintf(dp, ";pid=0x%x", current->pid);
diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c
index 5d9b9ac..bce99e6 100644
--- a/fs/cifs/cifsencrypt.c
+++ b/fs/cifs/cifsencrypt.c
@@ -420,15 +420,20 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash,
 	}
 
 	/* convert ses->user_name to unicode and uppercase */
-	len = strlen(ses->user_name);
+	len = ses->user_name ? strlen(ses->user_name) : 0;
 	user = kmalloc(2 + (len * 2), GFP_KERNEL);
 	if (user == NULL) {
 		cERROR(1, "calc_ntlmv2_hash: user mem alloc failure\n");
 		rc = -ENOMEM;
 		return rc;
 	}
-	len = cifs_strtoUCS((__le16 *)user, ses->user_name, len, nls_cp);
-	UniStrupr(user);
+
+	if (len) {
+		len = cifs_strtoUCS((__le16 *)user, ses->user_name, len, nls_cp);
+		UniStrupr(user);
+	} else {
+		memset(user, '\0', 2);
+	}
 
 	rc = crypto_shash_update(&ses->server->secmech.sdeschmacmd5->shash,
 				(char *)user, 2 * len);
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 8dbb6ff..5e96e60 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -1997,10 +1997,16 @@ static int match_session(struct cifs_ses *ses, struct smb_vol *vol)
 			return 0;
 		break;
 	default:
+		/* NULL username means anonymous session */
+		if (ses->user_name == NULL) {
+			if (!vol->nullauth)
+				return 0;
+			break;
+		}
+
 		/* anything else takes username/password */
-		if (ses->user_name == NULL)
-			return 0;
-		if (strncmp(ses->user_name, vol->username,
+		if (strncmp(ses->user_name,
+			    vol->username ? vol->username: "",
 			    MAX_USERNAME_SIZE))
 			return 0;
 		if (strlen(vol->username) != 0 &&
@@ -3152,10 +3158,9 @@ cifs_setup_volume_info(struct smb_vol *volume_info, char *mount_data,
 		return -EINVAL;
 
 	if (volume_info->nullauth) {
-		cFYI(1, "null user");
-		volume_info->username = kzalloc(1, GFP_KERNEL);
-		if (volume_info->username == NULL)
-			return -ENOMEM;
+		cFYI(1, "Anonymous login");
+		kfree(volume_info->username);
+		volume_info->username = NULL;
 	} else if (volume_info->username) {
 		/* BB fixme parse for domain name here */
 		cFYI(1, "Username: %s", volume_info->username);
-- 
1.7.7.4

[PATCH v2 3/4] cifs: fetch credentials out of keyring for non-krb5 auth multiuser mounts

[Jeff Layton]

Fix up multiuser mounts to set the secType and set the username and
password from the key payload in the vol info for non-krb5 auth types.

Look for a key of type "secret" with a description of
"cifs:a:<server address>" or "cifs:d:<domainname>". If that's found,
then scrape the username and password out of the key payload and use
that to create a new user session.

Finally, don't have the code enforce krb5 auth on multiuser mounts,
but do require a kernel with keys support.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |  175 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 165 insertions(+), 10 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 5e96e60..73535ef 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -38,6 +38,7 @@
 #include <asm/processor.h>
 #include <linux/inet.h>
 #include <linux/module.h>
+#include <keys/user-type.h>
 #include <net/ipv6.h>
 #include "cifspdu.h"
 #include "cifsglob.h"
@@ -1594,11 +1595,14 @@ cifs_parse_mount_options(const char *mountdata, const char *devname,
 		}
 	}
 
-	if (vol->multiuser && !(vol->secFlg & CIFSSEC_MAY_KRB5)) {
-		cERROR(1, "Multiuser mounts currently require krb5 "
-			  "authentication!");
+#ifndef CONFIG_KEYS
+	/* Muliuser mounts require CONFIG_KEYS support */
+	if (vol->multiuser) {
+		cERROR(1, "Multiuser mounts require kernels with "
+			  "CONFIG_KEYS enabled.");
 		goto cifs_parse_mount_err;
 	}
+#endif
 
 	if (vol->UNCip == NULL)
 		vol->UNCip = &vol->UNC[2];
@@ -2061,6 +2065,132 @@ cifs_put_smb_ses(struct cifs_ses *ses)
 	cifs_put_tcp_session(server);
 }
 
+#ifdef CONFIG_KEYS
+
+/* strlen("cifs:a:") + INET6_ADDRSTRLEN + 1 */
+#define CIFSCREDS_DESC_SIZE (7 + INET6_ADDRSTRLEN + 1)
+
+/* Populate username and pw fields from keyring if possible */
+static int
+cifs_set_cifscreds(struct smb_vol *vol, struct cifs_ses *ses)
+{
+	int rc = 0;
+	char *desc, *delim, *payload;
+	ssize_t len;
+	struct key *key;
+	struct TCP_Server_Info *server = ses->server;
+	struct sockaddr_in *sa;
+	struct sockaddr_in6 *sa6;
+	struct user_key_payload *upayload;
+
+	desc = kmalloc(CIFSCREDS_DESC_SIZE, GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	/* try to find an address key first */
+	switch (server->dstaddr.ss_family) {
+	case AF_INET:
+		sa = (struct sockaddr_in *)&server->dstaddr;
+		sprintf(desc, "cifs:a:%pI4", &sa->sin_addr.s_addr);
+		break;
+	case AF_INET6:
+		sa6 = (struct sockaddr_in6 *)&server->dstaddr;
+		sprintf(desc, "cifs:a:%pI6c", &sa6->sin6_addr.s6_addr);
+		break;
+	default:
+		cFYI(1, "Bad ss_family (%hu)", server->dstaddr.ss_family);
+		rc = -EINVAL;
+		goto out_err;
+	}
+
+	cFYI(1, "%s: desc=%s", __func__, desc);
+	key = request_key(&key_type_secret, desc, "");
+	if (IS_ERR(key)) {
+		if (!ses->domainName) {
+			cFYI(1, "domainName is NULL");
+			rc = PTR_ERR(key);
+			goto out_err;
+		}
+
+		/* didn't work, try to find a domain key */
+		sprintf(desc, "cifs:d:%s", ses->domainName);
+		cFYI(1, "%s: desc=%s", __func__, desc);
+		key = request_key(&key_type_secret, desc, "");
+		if (IS_ERR(key)) {
+			rc = PTR_ERR(key);
+			goto out_err;
+		}
+	}
+
+	down_read(&key->sem);
+	upayload = key->payload.data;
+	if (IS_ERR_OR_NULL(upayload)) {
+		rc = PTR_ERR(key);
+		goto out_key_put;
+	}
+
+	/* find first : in payload */
+	payload = (char *)upayload->data;
+	delim = strnchr(payload, upayload->datalen, ':');
+	cFYI(1, "payload=%s", payload);
+	if (!delim) {
+		cFYI(1, "Unable to find ':' in payload (datalen=%d)",
+				upayload->datalen);
+		rc = -EINVAL;
+		goto out_key_put;
+	}
+
+	len = delim - payload;
+	if (len > MAX_USERNAME_SIZE || len <= 0) {
+		cFYI(1, "Bad value from username search (len=%ld)", len);
+		rc = -EINVAL;
+		goto out_key_put;
+	}
+
+	vol->username = kstrndup(payload, len, GFP_KERNEL);
+	if (!vol->username) {
+		cFYI(1, "Unable to allocate %ld bytes for username", len);
+		rc = -ENOMEM;
+		goto out_key_put;
+	}
+	cFYI(1, "%s: username=%s", __func__, vol->username);
+
+	len = key->datalen - (len + 1);
+	if (len > MAX_PASSWORD_SIZE || len <= 0) {
+		cFYI(1, "Bad len for password search (len=%ld)", len);
+		rc = -EINVAL;
+		kfree(vol->username);
+		vol->username = NULL;
+		goto out_key_put;
+	}
+
+	++delim;
+	vol->password = kstrndup(delim, len, GFP_KERNEL);
+	if (!vol->password) {
+		cFYI(1, "Unable to allocate %ld bytes for password", len);
+		rc = -ENOMEM;
+		kfree(vol->username);
+		vol->username = NULL;
+		goto out_key_put;
+	}
+
+out_key_put:
+	up_read(&key->sem);
+	key_put(key);
+out_err:
+	kfree(desc);
+	cFYI(1, "%s: returning %d", __func__, rc);
+	return rc;
+}
+#else /* ! CONFIG_KEYS */
+static inline int
+cifs_set_cifscreds(struct smb_vol *vol __attribute__((unused)),
+		   struct cifs_ses *ses __attribute__((unused)))
+{
+	return -ENOSYS;
+}
+#endif /* CONFIG_KEYS */
+
 static bool warned_on_ntlm;  /* globals init to false automatically */
 
 static struct cifs_ses *
@@ -3678,16 +3808,38 @@ int cifs_setup_session(unsigned int xid, struct cifs_ses *ses,
 	return rc;
 }
 
+static int
+cifs_set_vol_auth(struct smb_vol *vol, struct cifs_ses *ses)
+{
+	switch(ses->server->secType) {
+	case Kerberos:
+		vol->secFlg = CIFSSEC_MUST_KRB5;
+		return 0;
+	case NTLMv2:
+		vol->secFlg = CIFSSEC_MUST_NTLMV2;
+		break;
+	case NTLM:
+		vol->secFlg = CIFSSEC_MUST_NTLM;
+		break;
+	case RawNTLMSSP:
+		vol->secFlg = CIFSSEC_MUST_NTLMSSP;
+		break;
+	case LANMAN:
+		vol->secFlg = CIFSSEC_MUST_LANMAN;
+		break;
+	}
+
+	return cifs_set_cifscreds(vol, ses);
+}
+
 static struct cifs_tcon *
 cifs_construct_tcon(struct cifs_sb_info *cifs_sb, uid_t fsuid)
 {
+	int rc;
 	struct cifs_tcon *master_tcon = cifs_sb_master_tcon(cifs_sb);
 	struct cifs_ses *ses;
 	struct cifs_tcon *tcon = NULL;
 	struct smb_vol *vol_info;
-	char username[28]; /* big enough for "krb50x" + hex of ULONG_MAX 6+16 */
-			   /* We used to have this as MAX_USERNAME which is   */
-			   /* way too big now (256 instead of 32) */
 
 	vol_info = kzalloc(sizeof(*vol_info), GFP_KERNEL);
 	if (vol_info == NULL) {
@@ -3695,8 +3847,6 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, uid_t fsuid)
 		goto out;
 	}
 
-	snprintf(username, sizeof(username), "krb50x%x", fsuid);
-	vol_info->username = username;
 	vol_info->local_nls = cifs_sb->local_nls;
 	vol_info->linux_uid = fsuid;
 	vol_info->cred_uid = fsuid;
@@ -3706,8 +3856,11 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, uid_t fsuid)
 	vol_info->local_lease = master_tcon->local_lease;
 	vol_info->no_linux_ext = !master_tcon->unix_ext;
 
-	/* FIXME: allow for other secFlg settings */
-	vol_info->secFlg = CIFSSEC_MUST_KRB5;
+	rc = cifs_set_vol_auth(vol_info, master_tcon->ses);
+	if (rc) {
+		tcon = ERR_PTR(rc);
+		goto out;
+	}
 
 	/* get a reference for the same TCP session */
 	spin_lock(&cifs_tcp_ses_lock);
@@ -3730,6 +3883,8 @@ cifs_construct_tcon(struct cifs_sb_info *cifs_sb, uid_t fsuid)
 	if (ses->capabilities & CAP_UNIX)
 		reset_cifs_unix_caps(0, tcon, NULL, vol_info);
 out:
+	kfree(vol_info->username);
+	kfree(vol_info->password);
 	kfree(vol_info);
 
 	return tcon;
-- 
1.7.7.4

[PATCH v2 4/4] cifs: warn about impending deprecation of legacy MultiuserMount code

[Jeff Layton]

We'll allow a grace period of 2 releases (3.3 and 3.4) and then remove
the legacy code in 3.5.

Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_debug.c |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index 84e8c07..24b3dfc 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -676,14 +676,23 @@ static ssize_t cifs_multiuser_mount_proc_write(struct file *file,
 {
 	char c;
 	int rc;
+	static bool warned;
 
 	rc = get_user(c, buffer);
 	if (rc)
 		return rc;
 	if (c == '0' || c == 'n' || c == 'N')
 		multiuser_mount = 0;
-	else if (c == '1' || c == 'y' || c == 'Y')
+	else if (c == '1' || c == 'y' || c == 'Y') {
 		multiuser_mount = 1;
+		if (!warned) {
+			warned = true;
+			printk(KERN_WARNING "CIFS VFS: The legacy multiuser "
+				"mount code is scheduled to be deprecated in "
+				"3.5. Please switch to using the multiuser "
+				"mount option.");
+		}
+	}
 
 	return count;
 }
-- 
1.7.7.4

Re: [PATCH v2 1/4] keys: add a "secret" key type

[Steve French]

This looks fine and plan to merge via the cifs git tree, unless
anyone has last minute objections.

On Fri, Jan 6, 2012 at 1:30 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> For CIFS, we want to be able to store NTLM credentials (aka username
> and password) in the keyring. We do not, however want to allow users
> to fetch those keys back out of the keyring since that would be a
> security risk.
>
> Unfortunately, due to the nuances of key permission bits, it's not
> possible to do this. We need to grant search permissions so the kernel
> can find these keys, but that also implies permissions to read the
> payload.
>
> Resolve this by adding a new key_type. This key type is essentially
> the same as key_type_user, but does not define a .read op. This
> prevents the payload from ever being visible from userspace.
>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  include/keys/user-type.h     |    3 ++-
>  security/keys/internal.h     |    1 +
>  security/keys/key.c          |    1 +
>  security/keys/user_defined.c |   17 +++++++++++++++++
>  4 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/include/keys/user-type.h b/include/keys/user-type.h
> index c37c342..41b5515 100644
> --- a/include/keys/user-type.h
> +++ b/include/keys/user-type.h
> @@ -17,7 +17,7 @@
>
>  /*****************************************************************************/
>  /*
> - * the payload for a key of type "user"
> + * the payload for a key of type "user" or "secret"
>  * - once filled in and attached to a key:
>  *   - the payload struct is invariant may not be changed, only replaced
>  *   - the payload must be read with RCU procedures or with the key semaphore
> @@ -33,6 +33,7 @@ struct user_key_payload {
>  };
>
>  extern struct key_type key_type_user;
> +extern struct key_type key_type_secret;
>
>  extern int user_instantiate(struct key *key, const void *data, size_t datalen);
>  extern int user_update(struct key *key, const void *data, size_t datalen);
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index c7a7cae..2784e07 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -33,6 +33,7 @@
>
>  extern struct key_type key_type_dead;
>  extern struct key_type key_type_user;
> +extern struct key_type key_type_secret;
>
>  /*****************************************************************************/
>  /*
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 4414abd..3d1d79d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -996,6 +996,7 @@ void __init key_init(void)
>        list_add_tail(&key_type_keyring.link, &key_types_list);
>        list_add_tail(&key_type_dead.link, &key_types_list);
>        list_add_tail(&key_type_user.link, &key_types_list);
> +       list_add_tail(&key_type_secret.link, &key_types_list);
>
>        /* record the root user tracking */
>        rb_link_node(&root_key_user.node,
> diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
> index 69ff52c..e25782f 100644
> --- a/security/keys/user_defined.c
> +++ b/security/keys/user_defined.c
> @@ -36,6 +36,23 @@ struct key_type key_type_user = {
>  EXPORT_SYMBOL_GPL(key_type_user);
>
>  /*
> + * This key type is essentially the same as key_type_user, but it does
> + * not define a .read op. This is suitable for storing information in
> + * the keyring that you do not want to be readable from userspace. For
> + * instance, passwords or secret encryption keys.
> + */
> +struct key_type key_type_secret = {
> +       .name           = "secret",
> +       .instantiate    = user_instantiate,
> +       .update         = user_update,
> +       .match          = user_match,
> +       .revoke         = user_revoke,
> +       .destroy        = user_destroy,
> +       .describe       = user_describe,
> +};
> +EXPORT_SYMBOL_GPL(key_type_secret);
> +
> +/*
>  * instantiate a user defined key
>  */
>  int user_instantiate(struct key *key, const void *data, size_t datalen)
> --
> 1.7.7.4
>



-- 
Thanks,

Steve

Re: [PATCH v2 1/4] keys: add a "secret" key type

[Steve French]

This looks fine and plan to merge via the cifs git tree, unless
anyone has last minute objections.

On Fri, Jan 6, 2012 at 1:30 PM, Jeff Layton <jlayton@redhat.com> wrote:
> For CIFS, we want to be able to store NTLM credentials (aka username
> and password) in the keyring. We do not, however want to allow users
> to fetch those keys back out of the keyring since that would be a
> security risk.
>
> Unfortunately, due to the nuances of key permission bits, it's not
> possible to do this. We need to grant search permissions so the kernel
> can find these keys, but that also implies permissions to read the
> payload.
>
> Resolve this by adding a new key_type. This key type is essentially
> the same as key_type_user, but does not define a .read op. This
> prevents the payload from ever being visible from userspace.
>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  include/keys/user-type.h     |    3 ++-
>  security/keys/internal.h     |    1 +
>  security/keys/key.c          |    1 +
>  security/keys/user_defined.c |   17 +++++++++++++++++
>  4 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/include/keys/user-type.h b/include/keys/user-type.h
> index c37c342..41b5515 100644
> --- a/include/keys/user-type.h
> +++ b/include/keys/user-type.h
> @@ -17,7 +17,7 @@
>
>  /*****************************************************************************/
>  /*
> - * the payload for a key of type "user"
> + * the payload for a key of type "user" or "secret"
>  * - once filled in and attached to a key:
>  *   - the payload struct is invariant may not be changed, only replaced
>  *   - the payload must be read with RCU procedures or with the key semaphore
> @@ -33,6 +33,7 @@ struct user_key_payload {
>  };
>
>  extern struct key_type key_type_user;
> +extern struct key_type key_type_secret;
>
>  extern int user_instantiate(struct key *key, const void *data, size_t datalen);
>  extern int user_update(struct key *key, const void *data, size_t datalen);
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index c7a7cae..2784e07 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -33,6 +33,7 @@
>
>  extern struct key_type key_type_dead;
>  extern struct key_type key_type_user;
> +extern struct key_type key_type_secret;
>
>  /*****************************************************************************/
>  /*
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 4414abd..3d1d79d 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -996,6 +996,7 @@ void __init key_init(void)
>        list_add_tail(&key_type_keyring.link, &key_types_list);
>        list_add_tail(&key_type_dead.link, &key_types_list);
>        list_add_tail(&key_type_user.link, &key_types_list);
> +       list_add_tail(&key_type_secret.link, &key_types_list);
>
>        /* record the root user tracking */
>        rb_link_node(&root_key_user.node,
> diff --git a/security/keys/user_defined.c b/security/keys/user_defined.c
> index 69ff52c..e25782f 100644
> --- a/security/keys/user_defined.c
> +++ b/security/keys/user_defined.c
> @@ -36,6 +36,23 @@ struct key_type key_type_user = {
>  EXPORT_SYMBOL_GPL(key_type_user);
>
>  /*
> + * This key type is essentially the same as key_type_user, but it does
> + * not define a .read op. This is suitable for storing information in
> + * the keyring that you do not want to be readable from userspace. For
> + * instance, passwords or secret encryption keys.
> + */
> +struct key_type key_type_secret = {
> +       .name           = "secret",
> +       .instantiate    = user_instantiate,
> +       .update         = user_update,
> +       .match          = user_match,
> +       .revoke         = user_revoke,
> +       .destroy        = user_destroy,
> +       .describe       = user_describe,
> +};
> +EXPORT_SYMBOL_GPL(key_type_secret);
> +
> +/*
>  * instantiate a user defined key
>  */
>  int user_instantiate(struct key *key, const void *data, size_t datalen)
> --
> 1.7.7.4
>



-- 
Thanks,

Steve

Re: [PATCH v2 1/4] keys: add a "secret" key type

[David Howells]

Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> For CIFS, we want to be able to store NTLM credentials (aka username
> and password) in the keyring. We do not, however want to allow users
> to fetch those keys back out of the keyring since that would be a
> security risk.
> 
> Unfortunately, due to the nuances of key permission bits, it's not
> possible to do this. We need to grant search permissions so the kernel
> can find these keys, but that also implies permissions to read the
> payload.

That was, perhaps, a bad design decision.  I attempted to make the use of keys
symmetric between the kernel services and userspace tools.  The kernel only has
to be able to find a key to use it, whereas userspace has to be able to *read*
a key to be able to use it - unless it can ask a kernel service to use the key
on its behalf.  SELinux can then be used to restrict who *actually* gets
permission.

I think I need to consider one of a couple of options: (a) simply retracting
read permission on Search permission only or (b) ACLs, so that a use of a key
can be granted directly to a userspace tool.  There may be other options also.

> Resolve this by adding a new key_type. This key type is essentially
> the same as key_type_user, but does not define a .read op. This
> prevents the payload from ever being visible from userspace.

I'm not fond of the idea of adding a completely open key type with no
restrictions on it.  user-defined keys do have an honorary restriction on their
descriptions, as stated in the documentation, for instance in the add_key()
manual page it says:

       "user" Keys  of  the  user-defined key type may contain a blob of arbi-
              trary data, and the description may be any valid string,  though
              it  is  preferred that the description be prefixed with a string
              representing the service to which the key is of interest  and  a
              colon  (for  instance "afs:mykey").  The payload may be empty or
              NULL for keys of this type.

If there are no restrictions at all, then you have a problem with potential
collisions in description space when multiple users start to use it.
Furthermore, you also have no vetting of the payload and no suppression of
updates to it, which means that the kernel has to recheck the payload each
time it gets a lock on it with the intent to make use of it.

I would prefer to see one or more of the following:

 (1) A key type more specific to the data contained.

     This has a couple of practical benefits: it is easier to discard a whole
     class of keys and easier to write useful restrictions on the description
     and payload; and it makes for faster searching as keys of different types
     are detected by pointer comparison whereas keys of the same type have to
     go to the type's ->match() op and potentially do string comparisons.

     It could still be relatively generic, say 'foreign_id'

 (2) Restrictions on the description, particularly if it is a generic key type.

     There exists a ->vet_description() method in the key type.  This can be
     used to make sure the description is suitable for the purpose intended.
     At the very least, if the key type is generic, I would very much prefer to
     see the key type being prefixed with a subclass (say "foo:"), and the vet
     function can check that there's a non-empty length string followed by a
     colon.

 (3) Checks on the payload; perhaps even a parsing of the payload into structs
     that are then attached to the key.  This might obviate the need for checks
     each time the key is used.

And then Documentation/security/ needs some additions...  But that's fine to be
done in a follow-up patch.  If the key is a generic key I'll need an addition
for the add_key() manual page too.

David