Re: [PATCH v2] KEYS: make keyctl_invalidate() also require Setattr permission

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi David,

On Fri, Sep 08, 2017 at 04:41:40PM +0100, David Howells wrote:
> Eric Biggers <ebiggers3@xxxxxxxxx> wrote:
> 
> > True, but Setattr permission has already been overloaded to allow several
> > different types of modifications,
> 
> Perhaps therein lies the problem.  Setattr is too overloaded and really needs
> splitting along the following lines:
> 
>  (1) Security: ownership, access, security label, restrictions.
> 
>  (2) Content: alter/update, timeout.
> 
>  (3) Revocation and invalidation.
> 
> Maybe I should create some sort of ACL for keys and map setperm onto that.
> 
> I wonder if this could also intersect with user namespaces in some way so that
> you can make an ACL that has users from multiple namespaces - might be tricky,
> though.

There would be a *lot* of changes needed to implement ACLs for keys.  Setting
them, getting them, evaluating them, updating all the LSMs to understand any new
permissions, deciding how the existing keyctl operations affect the ACL,
deciding what ACL to use by default, etc...  And it would make the keyrings
permissions system even more complicated, when people already have trouble
understanding it.  Complicated permissions systems are difficult to use
correctly.

Also I suppose that Invalidate would be implied by the old "Search" permission,
so users would need to use a new keyctl to remove Invalidate permission from the
ACL to get sane behavior?  That would be a pain, in comparison to having the bug
fixed by default.

> 
> > and it makes *much* more sense than Search permission which should not allow
> > any modifications.  And in practice I expect people care more about whether
> > modifications are permitted or not, than the details of the finer-grained
> > permissions.
> 
> I disagree still.  To allow users to invalidate a key you would *also* have to
> give them the ability to muck around with the permissions.
> 

True, though actually keyctl_setperm() also requires that the user own the key
or be root.

And while finer-grained permissions are preferable in theory, out of curiosity
do you have an example of an actual user who needs to be able to invalidate, or
perhaps in plainer language, "delete" keys without otherwise being able to
modify the keys or their containing keyring(s)?

Meanwhile, users of filesystem-level encryption need to set up keys that can be
searched by non-root users but not deleted.  This *should* be a normal, very
boring use case, since being able to grant read-only access should be a
fundamental part of any access control system.  But not with the keyrings
permission system, apparently :-)

> > Sort of, but actually keyctl_set_timeout() can be called at any time, and
> > the timeout can be set to as little as 1 second.  So I don't see how
> > keyctl_revoke() is that much different, fundamentally.
> 
> The timeout is a property of the key content and revoked is one of the states
> the key can be in.
> 
> > >  (1) I add a flag to a key to say that it can be invalidated and a keyctl to
> > >      change that flag.
> > 
> > And who would have permission to change that flag?  It seems to be the same
> > problem again.
> 
> No.  Setperm would be required to change the flag, but not to apply the
> invalidate operation if the flag is set.  Think of Invalidate as being a
> mass-unlink operation effected by the garbage collector.
> 
> Kernel-created DNS keys, for example, don't grant Setperm to anyone.
> 

Perhaps keyctl_invalidate() should by default require Setattr permission, or
perhaps owning the key --- but after a call to a new keyctl_set_invalidatable()
which would require Setattr permission, just Search permission would be
required?  It seems that would address your concern about needing to grant
Setattr permission to invalidate keys.

> 
> > What would the behavior be if ->allow_invalidation() was not supplied?
> 
> The obvious would be to check that you're the owner of the key.
> 
> > In other words, would the purpose of this be to lock down invalidation of
> > dns_resolver keys, or to restrict invalidation to *only* dns_resolver keys?
> 
> To restrict who could invalidate keys of a particular type.  Actually, it
> wants to be per-key not per-key-type.  Hmmm...
> 
> > Granted by who, and how?  And do you mean keyctl_clear(), or
> > keyctl_invalidate()?
> 
> keyctl_clear().  Empty the keyring, not render it unusable.
> 
> 
> David

If there is a "keyctl_set_invalidatable()" being added too, I don't really see
the point of adding an ->allow_invalidation() method and mucking around with
keyctl_clear() permissions as well.

Eric



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]