From 1260f801b4e4ba7be200886b4a53d730de05ca19 Mon Sep 17 00:00:00 2001 From: David Howells Date: Thu, 4 Aug 2005 11:50:01 +0100 Subject: [PATCH] Keys: Fix key management syscall interface bugs This fixes five bugs in the key management syscall interface: (1) add_key() returns 0 rather than EINVAL if the key type is "". Checking the key type isn't "" should be left to lookup_user_key(). (2) request_key() returns ENOKEY rather than EPERM if the key type begins with a ".". lookup_user_key() can't do this because internal key types begin with a ".". (3) Key revocation always returns 0, even if it fails. (4) Key read can return EAGAIN rather than EACCES under some circumstances. A key is permitted to by read by a process if it doesn't grant read access, but it does grant search access and it is in the process's keyrings. That search returns EAGAIN if it fails, and this needs translating to EACCES. (5) request_key() never adds the new key to the destination keyring if one is supplied. The wrong macro was being used to test for an error condition: PTR_ERR() will always return true, whether or not there's an error; this should've been IS_ERR(). Signed-Off-By: David Howells Signed-Off-By: Linus Torvalds --- security/keys/keyctl.c | 11 +++++++---- security/keys/request_key.c | 2 +- 2 files changed, 8 insertions(+), 5 deletions(-) (limited to 'security/keys') diff --git a/security/keys/keyctl.c b/security/keys/keyctl.c index fea262860ea..a6516a64b29 100644 --- a/security/keys/keyctl.c +++ b/security/keys/keyctl.c @@ -49,9 +49,6 @@ asmlinkage long sys_add_key(const char __user *_type, goto error; type[31] = '\0'; - if (!type[0]) - goto error; - ret = -EPERM; if (type[0] == '.') goto error; @@ -144,6 +141,10 @@ asmlinkage long sys_request_key(const char __user *_type, goto error; type[31] = '\0'; + ret = -EPERM; + if (type[0] == '.') + goto error; + /* pull the description into kernel space */ ret = -EFAULT; dlen = strnlen_user(_description, PAGE_SIZE - 1); @@ -362,7 +363,7 @@ long keyctl_revoke_key(key_serial_t id) key_put(key); error: - return 0; + return ret; } /* end keyctl_revoke_key() */ @@ -685,6 +686,8 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen) goto can_read_key2; ret = PTR_ERR(skey); + if (ret == -EAGAIN) + ret = -EACCES; goto error2; } diff --git a/security/keys/request_key.c b/security/keys/request_key.c index dfcd983af1f..90c1506d007 100644 --- a/security/keys/request_key.c +++ b/security/keys/request_key.c @@ -405,7 +405,7 @@ struct key *request_key_and_link(struct key_type *type, key_user_put(user); /* link the new key into the appropriate keyring */ - if (!PTR_ERR(key)) + if (!IS_ERR(key)) request_key_link(key, dest_keyring); } -- cgit v1.2.3 From bcf945d36fa0598f41ac4ad46a9dc43135460263 Mon Sep 17 00:00:00 2001 From: David Howells Date: Thu, 4 Aug 2005 13:07:06 -0700 Subject: [PATCH] Error during attempt to join key management session can leave semaphore pinned The attached patch prevents an error during the key session joining operation from hanging future joins in the D state [CAN-2005-2098]. The problem is that the error handling path for the KEYCTL_JOIN_SESSION_KEYRING operation has one error path that doesn't release the session management semaphore. Further attempts to get the semaphore will then sleep for ever in the D state. This can happen in four situations, all involving an attempt to allocate a new session keyring: (1) ENOMEM. (2) The users key quota being reached. (3) A keyring name that is an empty string. (4) A keyring name that is too long. Any user may attempt this operation, and so any user can cause the problem to occur. Signed-Off-By: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/keys/process_keys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'security/keys') diff --git a/security/keys/process_keys.c b/security/keys/process_keys.c index 9b0369c5a22..c089f78fb94 100644 --- a/security/keys/process_keys.c +++ b/security/keys/process_keys.c @@ -678,7 +678,7 @@ long join_session_keyring(const char *name) keyring = keyring_alloc(name, tsk->uid, tsk->gid, 0, NULL); if (IS_ERR(keyring)) { ret = PTR_ERR(keyring); - goto error; + goto error2; } } else if (IS_ERR(keyring)) { -- cgit v1.2.3 From 94efe72f762e2c147d8146d637d5ece5614c8d94 Mon Sep 17 00:00:00 2001 From: David Howells Date: Thu, 4 Aug 2005 13:07:07 -0700 Subject: [PATCH] Destruction of failed keyring oopses The attached patch makes sure that a keyring that failed to instantiate properly is destroyed without oopsing [CAN-2005-2099]. The problem occurs in three stages: (1) The key allocator initialises the type-specific data to all zeroes. In the case of a keyring, this will become a link in the keyring name list when the keyring is instantiated. (2) If a user (any user) attempts to add a keyring with anything other than an empty payload, the keyring instantiation function will fail with an error and won't add the keyring to the name list. (3) The keyring's destructor then sees that the keyring has a description (name) and tries to remove the keyring from the name list, which oopses because the link pointers are both zero. This bug permits any user to take down a box trivially. Signed-Off-By: David Howells Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- security/keys/keyring.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'security/keys') diff --git a/security/keys/keyring.c b/security/keys/keyring.c index a1f6bac647a..9c208c756df 100644 --- a/security/keys/keyring.c +++ b/security/keys/keyring.c @@ -201,7 +201,11 @@ static void keyring_destroy(struct key *keyring) if (keyring->description) { write_lock(&keyring_name_lock); - list_del(&keyring->type_data.link); + + if (keyring->type_data.link.next != NULL && + !list_empty(&keyring->type_data.link)) + list_del(&keyring->type_data.link); + write_unlock(&keyring_name_lock); } -- cgit v1.2.3