[PATCH] keyring: Incorrect permissions checking in __keyring_search_one()

[PATCH] keyring: Incorrect permissions checking in __keyring_search_one()

[Arun Raghavan]

The __keyring_search_one() function currently has 2 issues with regards
to permissions:

 1. It does not check for KEY_SEARCH on the keyring before performing a
    search

 2. It accepts a "perm" parameter to check whether a given key in the
    keyring may be returned. This is incorrect, because it *must* check
    for KEY_SEARCH on the key before considering it a candidate for a
    match (and only KEY_SEARCH, since it is merely a search function).
    In fact, it's only caller, key_create_or_update() passes 0 as the
    permissions to check for (=> a key will be returned even if it
    doesn't have KEY_SEARCH set)

The second was discovered by Satyam Sharma <satyam@infradead.org>. Here
is a patch that fixes both issues.

Signed-off-by: Arun Raghavan <arunsr@cse.iitk.ac.in>
Acked-by: Satyam Sharma <satyam@infradead.org>

diff --git a/security/keys/internal.h b/security/keys/internal.h
index d36d693..94bffb8 100644
--- a/security/keys/internal.h
+++ b/security/keys/internal.h
@@ -83,8 +83,7 @@ extern int __key_link(struct key *keyring, struct key *key);
 
 extern key_ref_t __keyring_search_one(key_ref_t keyring_ref,
 				      const struct key_type *type,
-				      const char *description,
-				      key_perm_t perm);
+				      const char *description);
 
 extern struct key *keyring_search_instkey(struct key *keyring,
 					  key_serial_t target_id);
diff --git a/security/keys/key.c b/security/keys/key.c
index ca1d921..524249a 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -800,8 +800,7 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 	 * update that instead if possible
 	 */
 	if (ktype->update) {
-		key_ref = __keyring_search_one(keyring_ref, ktype, description,
-					       0);
+		key_ref = __keyring_search_one(keyring_ref, ktype, description);
 		if (!IS_ERR(key_ref))
 			goto found_matching_key;
 	}
diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 88292e3..c7f6fd2 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -446,17 +446,22 @@ EXPORT_SYMBOL(keyring_search);
  */
 key_ref_t __keyring_search_one(key_ref_t keyring_ref,
 			       const struct key_type *ktype,
-			       const char *description,
-			       key_perm_t perm)
+			       const char *description)
 {
 	struct keyring_list *klist;
 	unsigned long possessed;
 	struct key *keyring, *key;
 	int loop;
+	long err;
 
 	keyring = key_ref_to_ptr(keyring_ref);
 	possessed = is_key_possessed(keyring_ref);
 
+	/* the keyring must have search permission to begin the search */
+	err = key_permission(keyring_ref, KEY_SEARCH);
+	if (err < 0)
+		return ERR_PTR(err);
+
 	rcu_read_lock();
 
 	klist = rcu_dereference(keyring->payload.subscriptions);
@@ -468,7 +473,7 @@ key_ref_t __keyring_search_one(key_ref_t keyring_ref,
 			    (!key->type->match ||
 			     key->type->match(key, description)) &&
 			    key_permission(make_key_ref(key, possessed),
-					   perm) == 0 &&
+					   KEY_SEARCH) == 0 &&
 			    !test_bit(KEY_FLAG_REVOKED, &key->flags)
 			    )
 				goto found;

Re: [PATCH] keyring: Incorrect permissions checking in __keyring_search_one()

[David Howells]

Arun Raghavan <arunsr@cse.iitk.ac.in> wrote:

> The __keyring_search_one() function currently has 2 issues with regards
> to permissions:
> 
>  1. It does not check for KEY_SEARCH on the keyring before performing a
>     search

That is correct.  This is used by key_create_or_update() to check to see
whether there's a key in the current keyring that it can update rather than
adding a new key entirely.  key_create_or_update() mustn't be bound by
KEY_SEARCH permission, and similarly the target key doesn't require KEY_SEARCH
permission either; the control here is whether or not the target key has
KEY_WRITE permission.

>  2. It accepts a "perm" parameter to check whether a given key in the
>     keyring may be returned.

The "perm" parameter is superfluous given that nothing else now calls this
function.

David