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 <[email protected]>. Here
is a patch that fixes both issues.
Signed-off-by: Arun Raghavan <[email protected]>
Acked-by: Satyam Sharma <[email protected]>
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;
Arun Raghavan <[email protected]> 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