Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers  wrote:

> Therefore, change find_key_to_update() to return NULL if the found key
> is uninstantiated, so that add_key() replaces the key rather than
> instantiating it.  This seems to be better than fixing __key_update() to
> call __key_instantiate_and_link(), since given all the bugs noted above
> as well as that the existing behavior was undocumented and
> keyctl_instantiate() is supposed to be used instead, I doubt anyone was
> relying on the existing behavior.

keyctl_instantiate() can only be called from an upcall.  It can't be called in
the same context as keyctl_update().

I would be okay with making key_update() wait for completion of construction
in this case.

David

Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers  wrote:

> Therefore, change find_key_to_update() to return NULL if the found key
> is uninstantiated, so that add_key() replaces the key rather than
> instantiating it.  This seems to be better than fixing __key_update() to
> call __key_instantiate_and_link(), since given all the bugs noted above
> as well as that the existing behavior was undocumented and
> keyctl_instantiate() is supposed to be used instead, I doubt anyone was
> relying on the existing behavior.

keyctl_instantiate() can only be called from an upcall.  It can't be called in
the same context as keyctl_update().

I would be okay with making key_update() wait for completion of construction
in this case.

David

Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers  wrote:

> + if ((key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> +(1 << KEY_FLAG_REVOKED) |
> +(1 << KEY_FLAG_INSTANTIATED))) !=
> + (1 << KEY_FLAG_INSTANTIATED)) {

Does this need READ_ONCE(), I wonder?

David

Re: [PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[David Howells]

Eric Biggers  wrote:

> + if ((key->flags & ((1 << KEY_FLAG_INVALIDATED) |
> +(1 << KEY_FLAG_REVOKED) |
> +(1 << KEY_FLAG_INSTANTIATED))) !=
> + (1 << KEY_FLAG_INSTANTIATED)) {

Does this need READ_ONCE(), I wonder?

David

[PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[Eric Biggers]

From: Eric Biggers 

Currently, add_key() will, when passed a key that already exists, call
the key's ->update() method.  But this is heavily broken in the case
where the key is uninstantiated because it doesn't call
__key_instantiate_and_link().  Consequently, it doesn't do most of the
things that are supposed to happen when the key is instantiated, such as
setting KEY_FLAG_INSTANTIATED, clearing KEY_FLAG_USER_CONSTRUCT and
awakening tasks waiting on it, and incrementing key->user->nikeys.

It also never takes key_construction_mutex, which means that
->instantiate() can run concurrently with ->update() on the same key.
In the case of the "user" and "logon" key types this causes a memory
leak, at best.  Maybe even worse, the ->update() methods of the
"encrypted" and "trusted" key types actually just dereference a NULL
pointer when passed an uninstantiated key.

Therefore, change find_key_to_update() to return NULL if the found key
is uninstantiated, so that add_key() replaces the key rather than
instantiating it.  This seems to be better than fixing __key_update() to
call __key_instantiate_and_link(), since given all the bugs noted above
as well as that the existing behavior was undocumented and
keyctl_instantiate() is supposed to be used instead, I doubt anyone was
relying on the existing behavior.

This patch only affects *uninstantiated* keys.  For now we still allow a
negatively instantiated key to be updated (thereby positively
instantiating it), although that's broken too (the next patch fixes it)
and I'm not sure that anyone actually uses that functionality either.

Here is a simple reproducer for the bug using the "encrypted" key type
(requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug
pertained to more than just the "encrypted" key type:

#include 
#include 
#include 

int main(void)
{
int ringid = keyctl_join_session_keyring(NULL);

if (fork()) {
for (;;) {
const char payload[] = "update user:foo 32";

usleep(rand() % 1);
add_key("encrypted", "desc", payload, sizeof(payload), ringid);
keyctl_clear(ringid);
}
} else {
for (;;)
request_key("encrypted", "desc", "callout_info", ringid);
}
}

It causes:

BUG: unable to handle kernel NULL pointer dereference at 0018
IP: encrypted_update+0xb0/0x170
PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0
PREEMPT SMP
CPU: 0 PID: 340 Comm: reproduce Tainted: G  D 
4.14.0-rc1-00025-g428490e38b2e #796
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: 8a467a39a340 task.stack: b15c4077
RIP: 0010:encrypted_update+0xb0/0x170
RSP: 0018:b15c40773de8 EFLAGS: 00010246
RAX:  RBX: 8a467a275b00 RCX: 
RDX: 0005 RSI: 8a467a275b14 RDI: b742f303
RBP: b15c40773e20 R08:  R09: 8a467a275b17
R10: 0020 R11:  R12: 
R13:  R14: 8a4677057180 R15: 8a467a275b0f
FS:  7f5d7fb08700() GS:8a467f20() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0018 CR3: 77262005 CR4: 001606f0
Call Trace:
 key_create_or_update+0x2bc/0x460
 SyS_add_key+0x10c/0x1d0
 entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x7f5d7f211259
RSP: 002b:7ffed03904c8 EFLAGS: 0246 ORIG_RAX: 00f8
RAX: ffda RBX: 3b2a7955 RCX: 7f5d7f211259
RDX: 004009e4 RSI: 004009ff RDI: 00400a04
RBP: 68db8bad R08: 3b2a7955 R09: 0004
R10: 001a R11: 0246 R12: 00400868
R13: 7ffed03905d0 R14:  R15: 
Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 
e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 
ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b
RIP: encrypted_update+0xb0/0x170 RSP: b15c40773de8
CR2: 0018

Cc: [v2.6.12+]
Signed-off-by: Eric Biggers 
---
 security/keys/keyring.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..129a4175760b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1056,8 +1056,8 @@ EXPORT_SYMBOL(keyring_restrict);
  * caller must also hold a lock on the keyring semaphore.
  *
  * Returns a pointer to the found key with usage count incremented if
- * successful and returns NULL if not found.  Revoked and invalidated keys are
- * skipped over.
+ * successful and returns NULL if not found.  Revoked, invalidated, and

[PATCH v3 1/7] KEYS: don't let add_key() update an uninstantiated key

[Eric Biggers]

From: Eric Biggers 

Currently, add_key() will, when passed a key that already exists, call
the key's ->update() method.  But this is heavily broken in the case
where the key is uninstantiated because it doesn't call
__key_instantiate_and_link().  Consequently, it doesn't do most of the
things that are supposed to happen when the key is instantiated, such as
setting KEY_FLAG_INSTANTIATED, clearing KEY_FLAG_USER_CONSTRUCT and
awakening tasks waiting on it, and incrementing key->user->nikeys.

It also never takes key_construction_mutex, which means that
->instantiate() can run concurrently with ->update() on the same key.
In the case of the "user" and "logon" key types this causes a memory
leak, at best.  Maybe even worse, the ->update() methods of the
"encrypted" and "trusted" key types actually just dereference a NULL
pointer when passed an uninstantiated key.

Therefore, change find_key_to_update() to return NULL if the found key
is uninstantiated, so that add_key() replaces the key rather than
instantiating it.  This seems to be better than fixing __key_update() to
call __key_instantiate_and_link(), since given all the bugs noted above
as well as that the existing behavior was undocumented and
keyctl_instantiate() is supposed to be used instead, I doubt anyone was
relying on the existing behavior.

This patch only affects *uninstantiated* keys.  For now we still allow a
negatively instantiated key to be updated (thereby positively
instantiating it), although that's broken too (the next patch fixes it)
and I'm not sure that anyone actually uses that functionality either.

Here is a simple reproducer for the bug using the "encrypted" key type
(requires CONFIG_ENCRYPTED_KEYS=y), though as noted above the bug
pertained to more than just the "encrypted" key type:

#include 
#include 
#include 

int main(void)
{
int ringid = keyctl_join_session_keyring(NULL);

if (fork()) {
for (;;) {
const char payload[] = "update user:foo 32";

usleep(rand() % 1);
add_key("encrypted", "desc", payload, sizeof(payload), ringid);
keyctl_clear(ringid);
}
} else {
for (;;)
request_key("encrypted", "desc", "callout_info", ringid);
}
}

It causes:

BUG: unable to handle kernel NULL pointer dereference at 0018
IP: encrypted_update+0xb0/0x170
PGD 7a178067 P4D 7a178067 PUD 77269067 PMD 0
PREEMPT SMP
CPU: 0 PID: 340 Comm: reproduce Tainted: G  D 
4.14.0-rc1-00025-g428490e38b2e #796
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
task: 8a467a39a340 task.stack: b15c4077
RIP: 0010:encrypted_update+0xb0/0x170
RSP: 0018:b15c40773de8 EFLAGS: 00010246
RAX:  RBX: 8a467a275b00 RCX: 
RDX: 0005 RSI: 8a467a275b14 RDI: b742f303
RBP: b15c40773e20 R08:  R09: 8a467a275b17
R10: 0020 R11:  R12: 
R13:  R14: 8a4677057180 R15: 8a467a275b0f
FS:  7f5d7fb08700() GS:8a467f20() knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 0018 CR3: 77262005 CR4: 001606f0
Call Trace:
 key_create_or_update+0x2bc/0x460
 SyS_add_key+0x10c/0x1d0
 entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x7f5d7f211259
RSP: 002b:7ffed03904c8 EFLAGS: 0246 ORIG_RAX: 00f8
RAX: ffda RBX: 3b2a7955 RCX: 7f5d7f211259
RDX: 004009e4 RSI: 004009ff RDI: 00400a04
RBP: 68db8bad R08: 3b2a7955 R09: 0004
R10: 001a R11: 0246 R12: 00400868
R13: 7ffed03905d0 R14:  R15: 
Code: 77 28 e8 64 34 1f 00 45 31 c0 31 c9 48 8d 55 c8 48 89 df 48 8d 75 d0 
e8 ff f9 ff ff 85 c0 41 89 c4 0f 88 84 00 00 00 4c 8b 7d c8 <49> 8b 75 18 4c 89 
ff e8 24 f8 ff ff 85 c0 41 89 c4 78 6d 49 8b
RIP: encrypted_update+0xb0/0x170 RSP: b15c40773de8
CR2: 0018

Cc: [v2.6.12+]
Signed-off-by: Eric Biggers 
---
 security/keys/keyring.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/security/keys/keyring.c b/security/keys/keyring.c
index 4fa82a8a9c0e..129a4175760b 100644
--- a/security/keys/keyring.c
+++ b/security/keys/keyring.c
@@ -1056,8 +1056,8 @@ EXPORT_SYMBOL(keyring_restrict);
  * caller must also hold a lock on the keyring semaphore.
  *
  * Returns a pointer to the found key with usage count incremented if
- * successful and returns NULL if not found.  Revoked and invalidated keys are
- * skipped over.
+ * successful and returns NULL if not found.  Revoked, invalidated, and
+ * uninstantiated keys are skipped over.  (But negative keys are