KEYS: prevent KEYCTL_READ on negative key

From: Eric Biggers <[email protected]>

Because keyctl_read_key() looks up the key with no permissions requested, it may find a negatively instantiated key. If the key is also possessed, we went ahead and called ->read() on the key. But the key payload will actually contain the ->reject_error rather than the normal payload. Thus, the kernel oopses trying to read the user_key_payload from memory address (int)-ENOKEY = 0x00000000ffffff82.

Fortunately the payload data is stored inline, so it shouldn't be possible to abuse this as an arbitrary memory read primitive...

Reproducer:
keyctl new_session
keyctl request2 user desc '' @s
keyctl read $(keyctl show | awk '/user: desc/ {print $1}')

It causes a crash like the following:

     BUG: unable to handle kernel paging request at 00000000ffffff92
     IP: user_read+0x33/0xa0
     PGD 36a54067 P4D 36a54067 PUD 0
     Oops: 0000 [#1] SMP
     CPU: 0 PID: 211 Comm: keyctl Not tainted 4.14.0-rc1 #337
     Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-20170228_101828-anatol 04/01/2014
     task: ffff90aa3b74c3c0 task.stack: ffff9878c0478000
     RIP: 0010:user_read+0x33/0xa0
     RSP: 0018:ffff9878c047bee8 EFLAGS: 00010246
     RAX: 0000000000000001 RBX: ffff90aa3d7da340 RCX: 0000000000000017
     RDX: 0000000000000000 RSI: 00000000ffffff82 RDI: ffff90aa3d7da340
     RBP: ffff9878c047bf00 R08: 00000024f95da94f R09: 0000000000000000
     R10: 0000000000000001 R11: 0000000000000000 R12: 0000000000000000
     R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
     FS:  00007f58ece69740(0000) GS:ffff90aa3e200000(0000) knlGS:0000000000000000
     CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
     CR2: 00000000ffffff92 CR3: 0000000036adc001 CR4: 00000000003606f0
     Call Trace:
      keyctl_read_key+0xac/0xe0
      SyS_keyctl+0x99/0x120
      entry_SYSCALL_64_fastpath+0x1f/0xbe
     RIP: 0033:0x7f58ec787bb9
     RSP: 002b:00007ffc8d401678 EFLAGS: 00000206 ORIG_RAX: 00000000000000fa
     RAX: ffffffffffffffda RBX: 00007ffc8d402800 RCX: 00007f58ec787bb9
     RDX: 0000000000000000 RSI: 00000000174a63ac RDI: 000000000000000b
     RBP: 0000000000000004 R08: 00007ffc8d402809 R09: 0000000000000020
     R10: 0000000000000000 R11: 0000000000000206 R12: 00007ffc8d402800
     R13: 00007ffc8d4016e0 R14: 0000000000000000 R15: 0000000000000000
     Code: e5 41 55 49 89 f5 41 54 49 89 d4 53 48 89 fb e8 a4 b4 ad ff 85 c0 74 09 80 3d b9 4c 96 00 00 74 43 48 8b b3 20 01 00 00 4d 85 ed <0f> b7 5e 10 74 29 4d 85 e4 74 24 4c 39 e3 4c 89 e2 4c 89 ef 48
     RIP: user_read+0x33/0xa0 RSP: ffff9878c047bee8
     CR2: 00000000ffffff92

---
 security/keys/keyctl.c | 5 +++++
 1 file changed, 5 insertions(+)

index ab0b337c84b4..6a82090c7fc1 100644
@@ -766,6 +766,11 @@ long keyctl_read_key(key_serial_t keyid, char __user *buffer, size_t buflen)
 
 	key = key_ref_to_ptr(key_ref);
 
+	if (test_bit(KEY_FLAG_NEGATIVE, &key->flags)) {
+		ret = -ENOKEY;
+		goto error2;
+	}
+
 	/* see if we can read it directly */
 	ret = key_permission(key_ref, KEY_NEED_READ);
 	if (ret = 0)

2.14.1.690.gbb1197296e-goog

Putting the check in key_validate() would make lookups with KEY_LOOKUP_PARTIAL stop returning negative keys, which would break keyctl_describe(), keyctl_chown(), keyctl_setperm(), keyctl_set_timeout(), keyctl_get_security() on negative keys. I presume those are supposed to work?

Another solution would be to remove the special case from lookup_user_key() where it can return a negative/revoked/invalidated/expired key if KEY_LOOKUP_PARTIAL is not specified and the 'perm' mask is 0. The only callers it would affect are the case in question here which is clearly a bug, and the root-only exceptions for keyctl_invalidate() and keyctl_clear(). And I suspect the latter two are unintentional as well. (Is root *supposed* to be able to invalidate a negative/revoked/invalidated/expired key, or clear a revoked/invalidated/expired keyring?)

Eric

Lookups with KEY_LOOKUP_PARTIAL should never return a negative key by definition. A negative key is instantiated with an error code, so is no longer under construction.

key_get_instantiation_authkey() must fail if the key has been constructed - but I guess there's a potential race in keyctl_describe_key(), keyctl_set_timeout() and keyctl_get_security() between getting the auth token and calling lookup_user_key() with perm of 0 in which the key could be instantiated, revoked, or instantiated elsewhere, or simply expire. This would allow the instantiating process a longer access window - but they do/did have a valid token...

It should still be possible to describe, chown, setperm and get the security on negative keys by the normal access mechanism. Changing the timeout should probably be denied.

There are a number of circumstances in which it lookup_user_key() is called with perm=0, and in each case, the caller is responsible for handling the security:

(1) keyctl_invalidate_key() will do so if the caller doesn't have permission, but CAP_SYS_ADMIN is set and the key is marked KEY_FLAG_ROOT_CAN_INVAL.

(2) keyctl_keyring_clear() will do so if the caller doesn't have permission, but CAP_SYS_ADMIN is set and the key is marked KEY_FLAG_ROOT_CAN_CLEAR.

(3) keyctl_keyring_unlink() will do so on the key-to-be-removed since only the keyring needs a perm check.

(4) keyctl_read_key() always does so and then does the READ perm check and the possessor-can-SEARCH can search check itself.

(5) keyctl_describe_key(), keyctl_set_timeout() and keyctl_get_security() will do so if the caller doesn't have permission, but does have a valid authorisation token. The latter requires that the key be under construction.

Functions that use KEY_LOOKUP_PARTIAL include:

keyctl_describe_key()
keyctl_chown_key()
keyctl_setperm_key()
keyctl_set_timeout()
keyctl_get_security()

all of which might need to be called from the upcall program. None of these should look at the payload.

keyctl_read_key() is definitely buggy. Actually, rather than manually testing KEY_FLAG_NEGATIVE there, it should probably use key_validate().

I'm not sure what you think is unintentional.

You should be able to invalidate or unlink negative, revoked or expired keys if you have permission to do so. If you're using keyrings to cache stuff, you need to be able to invalidate negative results as well as positive ones.

Invalidation of an invalidated key doesn't really make sense, but it shouldn't hurt. I can't immediately automatically remove all links to the invalidated key, but have to leave it to the garbage collector to effect.

As for clearing of revoked/invalidated/expired keyrings, I'm not sure whether it makes sense to allow it - however, whilst keyrings are cleared upon revocation (since we have a definite point to do that with the key sem writelocked), they aren't automatically cleared upon expiry or invalidation, so it might make sense to permit it still.

David

Well, that's not what KEY_LOOKUP_PARTIAL actually does. KEY_LOOKUP_PARTIAL allows the returned key to be uninstantiated, negatively instantiated, *or* positively instantiated; and the callers seem to rely on that. Perhaps a better name might have been KEY_LOOKUP_ALLOW_PARTIAL or KEY_LOOKUP_ALLOW_NONPOSITIVE.

On the other hand, without KEY_LOOKUP_PARTIAL, the returned key is required to be positively instantiated. However, there is a special case. Namely, if no permissions are requested, the returned key is allowed to be negative (as well as revoked, invalidated, or expired --- though those can happen at any time anyway until you do down_read(&key->sem)). I'm questioning whether we need that special case.

Yes, by the time key_get_instantiation_authkey() returns, the key may have already been instantiated. But I'm not too concerned about that, since the caller must still have had a non-revoked authorization key shortly before.

It's not about the permission checks. It's about whether a negative key is allowed to be returned or not. And I think overloading 'perm' for that is not really appropriate, and the cause of the bug in keyctl_read_key(). See the code, it ignores the return value of wait_for_key_construction() if 'perm' is 0:

        if (!(lflags & KEY_LOOKUP_PARTIAL)) {
                ret = wait_for_key_construction(key, true);
                switch (ret) {
                case -ERESTARTSYS:
                        goto invalid_key;
                default:
                        if (perm)
                                goto invalid_key;
                case 0:
                        break;
                }
	}

It already does use key_validate(). But key_validate() is also used in lookup_user_key(), where it is expected to accept a negative key.

The real problem seems to be that the permissions mask rather than the flags argument is used to tell lookup_user_key() whether it can return a negative key.

That root is allowed to invalidate or clear a negative/invalidated/revoked/expired key or keyring but regular users cannot.

Again, the problem seems to be that the 'perm' argument is used for more than just the permission check. I think the 'lflags' argument should indicate what state the key is allowed to be in, not 'perm'.

Eric