* Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key
@ 2017-09-25 13:29 ` David Howells
0 siblings, 0 replies; 15+ messages in thread
From: David Howells @ 2017-09-25 13:29 UTC (permalink / raw)
To: Eric Biggers
Cc: dhowells, keyrings, Michael Halcrow, linux-security-module,
linux-kernel, Eric Biggers, stable
Eric Biggers <ebiggers3@gmail.com> wrote:
> 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?
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.
> 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.
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.
> The only callers it would affect are the case in question here which is
> clearly a bug,
keyctl_read_key() is definitely buggy. Actually, rather than manually testing
KEY_FLAG_NEGATIVE there, it should probably use key_validate().
> and the root-only exceptions for keyctl_invalidate() and
> keyctl_clear(). And I suspect the latter two are unintentional as well.
I'm not sure what you think is unintentional.
> (Is root *supposed* to be able to invalidate a
> negative/revoked/invalidated/expired key, or clear a
> revoked/invalidated/expired keyring?)
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] KEYS: prevent KEYCTL_READ on negative key
@ 2017-09-25 13:29 ` David Howells
0 siblings, 0 replies; 15+ messages in thread
From: David Howells @ 2017-09-25 13:29 UTC (permalink / raw)
To: linux-security-module
Eric Biggers <ebiggers3@gmail.com> wrote:
> 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?
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.
> 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.
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.
> The only callers it would affect are the case in question here which is
> clearly a bug,
keyctl_read_key() is definitely buggy. Actually, rather than manually testing
KEY_FLAG_NEGATIVE there, it should probably use key_validate().
> and the root-only exceptions for keyctl_invalidate() and
> keyctl_clear(). And I suspect the latter two are unintentional as well.
I'm not sure what you think is unintentional.
> (Is root *supposed* to be able to invalidate a
> negative/revoked/invalidated/expired key, or clear a
> revoked/invalidated/expired keyring?)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key
2017-09-25 13:29 ` David Howells
(?)
@ 2017-09-25 18:35 ` Eric Biggers
-1 siblings, 0 replies; 15+ messages in thread
From: Eric Biggers @ 2017-09-25 18:35 UTC (permalink / raw)
To: linux-security-module
On Mon, Sep 25, 2017 at 02:29:56PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
>
> > 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?
>
> 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.
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.
>
> 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...
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 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.
>
> > 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.
>
> 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.
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;
}
}
>
> 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.
>
> > The only callers it would affect are the case in question here which is
> > clearly a bug,
>
> keyctl_read_key() is definitely buggy. Actually, rather than manually testing
> KEY_FLAG_NEGATIVE there, it should probably use key_validate().
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.
>
> > and the root-only exceptions for keyctl_invalidate() and
> > keyctl_clear(). And I suspect the latter two are unintentional as well.
>
> I'm not sure what you think is unintentional.
>
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'.
> > (Is root *supposed* to be able to invalidate a
> > negative/revoked/invalidated/expired key, or clear a
> > revoked/invalidated/expired keyring?)
>
> 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.
>
Eric
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] KEYS: prevent KEYCTL_READ on negative key
@ 2017-09-25 18:35 ` Eric Biggers
0 siblings, 0 replies; 15+ messages in thread
From: Eric Biggers @ 2017-09-25 18:35 UTC (permalink / raw)
To: David Howells
Cc: keyrings, Michael Halcrow, linux-security-module, linux-kernel,
Eric Biggers, stable
On Mon, Sep 25, 2017 at 02:29:56PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
>
> > 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?
>
> 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.
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.
>
> 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...
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 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.
>
> > 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.
>
> 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.
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;
}
}
>
> 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.
>
> > The only callers it would affect are the case in question here which is
> > clearly a bug,
>
> keyctl_read_key() is definitely buggy. Actually, rather than manually testing
> KEY_FLAG_NEGATIVE there, it should probably use key_validate().
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.
>
> > and the root-only exceptions for keyctl_invalidate() and
> > keyctl_clear(). And I suspect the latter two are unintentional as well.
>
> I'm not sure what you think is unintentional.
>
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'.
> > (Is root *supposed* to be able to invalidate a
> > negative/revoked/invalidated/expired key, or clear a
> > revoked/invalidated/expired keyring?)
>
> 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.
>
Eric
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH] KEYS: prevent KEYCTL_READ on negative key
@ 2017-09-25 18:35 ` Eric Biggers
0 siblings, 0 replies; 15+ messages in thread
From: Eric Biggers @ 2017-09-25 18:35 UTC (permalink / raw)
To: linux-security-module
On Mon, Sep 25, 2017 at 02:29:56PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
>
> > 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?
>
> 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.
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.
>
> 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...
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 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.
>
> > 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.
>
> 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.
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;
}
}
>
> 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.
>
> > The only callers it would affect are the case in question here which is
> > clearly a bug,
>
> keyctl_read_key() is definitely buggy. Actually, rather than manually testing
> KEY_FLAG_NEGATIVE there, it should probably use key_validate().
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.
>
> > and the root-only exceptions for keyctl_invalidate() and
> > keyctl_clear(). And I suspect the latter two are unintentional as well.
>
> I'm not sure what you think is unintentional.
>
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'.
> > (Is root *supposed* to be able to invalidate a
> > negative/revoked/invalidated/expired key, or clear a
> > revoked/invalidated/expired keyring?)
>
> 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.
>
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 15+ messages in thread