Linux-Security-Module Archive on lore.kernel.org
 help / color / Atom feed
* Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next
@ 2020-04-17 15:48 Paul Moore
  2020-04-17 16:32 ` Richard Haines
                   ` (2 more replies)
  0 siblings, 3 replies; 35+ messages in thread
From: Paul Moore @ 2020-04-17 15:48 UTC (permalink / raw)
  To: David Howells; +Cc: keyrings, selinux, linux-security-module

I just notice that the "selinux: Implement the watch_key security
hook" patch made it's way into linux-next via 9ba09998baa9:

  commit 9ba09998baa995518d94c9a32e6329b28ccb9045
  Author: David Howells <dhowells@redhat.com>
  Date:   Tue Jan 14 17:07:13 2020 +0000

   selinux: Implement the watch_key security hook

   Implement the watch_key security hook to make sure that a key grants the
   caller View permission in order to set a watch on a key.

   For the moment, the watch_devices security hook is left unimplemented as
   it's not obvious what the object should be since the queue is global and
   didn't previously exist.

   Signed-off-by: David Howells <dhowells@redhat.com>
   Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

I'm reasonably confident that this code hasn't been tested as I expect
it would fail, or at the very least behave in unintended ways.  The
problem is the selinux_watch_key(...) function, shown below:

+static int selinux_watch_key(struct key *key)
+{
+       struct key_security_struct *ksec = key->security;
+       u32 sid = current_sid();
+
+       return avc_has_perm(&selinux_state,
+                           sid, ksec->sid, SECCLASS_KEY, KEY_NEED_VIEW, NULL);
+}

... in particular it is the fifth argument to avc_has_perm(),
"KEY_NEED_VIEW" which is a problem.  KEY_NEED_VIEW is not a SELinux
permission and would likely result in odd behavior when passed to
avc_has_perm().  Given that the keyring permission to SELinux object
class permission is variable depending on the key_perms policy
capability, it probably makes the most sense to pull the permission
mapping in selinux_key_permission() out into a separate function, e.g.
key_perm_to_av(...) (see the other XXX_to_av() functions in
security/selinux/hooks.c), and then use this newly created mapping
function in both selinux_key_permission() and selinux_watch_key().  Or
you could just duplicate the KEY_NEED_VIEW mapping code in both
functions, but I would advise against that.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, back to index

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-17 15:48 Problem with 9ba09998baa9 ("selinux: Implement the watch_key security hook") in linux-next Paul Moore
2020-04-17 16:32 ` Richard Haines
2020-04-17 16:59   ` Paul Moore
2020-04-21 12:29 ` David Howells
2020-04-22 19:20   ` Paul Moore
2020-04-22 21:09     ` Paul Moore
2020-04-24 23:43   ` David Howells
2020-04-26 20:53     ` Paul Moore
2020-04-27 14:12     ` [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms David Howells
2020-04-27 14:36       ` Stephen Smalley
2020-04-27 15:24         ` Paul Moore
2020-04-27 17:02       ` Stephen Smalley
2020-04-27 22:17         ` Paul Moore
2020-04-28 12:54 ` [PATCH] selinux: Fix use of KEY_NEED_* instead of KEY__* perms [v2] David Howells
2020-04-28 14:32   ` Stephen Smalley
2020-04-28 15:57   ` David Howells
2020-04-28 16:19     ` Stephen Smalley
2020-05-01 16:37       ` Paul Moore
2020-05-12 22:33       ` [PATCH] keys: Make the KEY_NEED_* perms an enum rather than a mask David Howells
2020-05-13  1:04         ` Paul Moore
2020-05-13 12:58         ` Stephen Smalley
2020-05-13 15:25         ` Casey Schaufler
2020-05-13 23:13         ` David Howells
2020-05-14 12:08           ` Stephen Smalley
2020-05-14 14:45             ` Stephen Smalley
2020-05-13 23:16         ` David Howells
2020-05-13 23:25         ` David Howells
2020-05-14 11:00         ` Jarkko Sakkinen
2020-05-14 16:58         ` [PATCH] keys: Move permissions checking decisions into the checking code David Howells
2020-05-14 17:06           ` Casey Schaufler
2020-05-15 15:06           ` Stephen Smalley
2020-05-15 16:45           ` David Howells
2020-05-15 18:55             ` Stephen Smalley
2020-05-15 19:10               ` Casey Schaufler
2020-05-15 22:27             ` David Howells

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org
	public-inbox-index linux-security-module

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git