| Message ID | 156717352917.2204.17206219813087348132.stgit@warthog.procyon.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Keyrings, Block and USB notifications [ver #7] | expand |
On 8/30/2019 6:58 AM, David Howells wrote: > Implement the watch_key security hook in Smack to make sure that a key > grants the caller Read permission in order to set a watch on a key. > > Also implement the post_notification security hook to make sure that the > notification source is granted Write permission by the watch queue. > > 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> I tried running your key tests and they fail in "keyctl/move/valid", with 11 FAILED messages, finally hanging after "UNLINK KEY FROM SESSION". It's possible that my Fedora26 system is somehow incompatible with the tests. I don't see anything in your code that would cause this, as the Smack policy on the system shouldn't restrict any access. > --- > > include/linux/lsm_audit.h | 1 + > security/smack/smack_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 82 insertions(+), 1 deletion(-) > > diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h > index 915330abf6e5..734d67889826 100644 > --- a/include/linux/lsm_audit.h > +++ b/include/linux/lsm_audit.h > @@ -74,6 +74,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_FILE 12 > #define LSM_AUDIT_DATA_IBPKEY 13 > #define LSM_AUDIT_DATA_IBENDPORT 14 > +#define LSM_AUDIT_DATA_NOTIFICATION 15 > union { > struct path path; > struct dentry *dentry; > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index 4c5e5a438f8b..1c2a908c6446 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -4274,7 +4274,7 @@ static int smack_key_permission(key_ref_t key_ref, > if (tkp == NULL) > return -EACCES; > > - if (smack_privileged_cred(CAP_MAC_OVERRIDE, cred)) > + if (smack_privileged(CAP_MAC_OVERRIDE)) > return 0; > > #ifdef CONFIG_AUDIT > @@ -4320,8 +4320,81 @@ static int smack_key_getsecurity(struct key *key, char **_buffer) > return length; > } > > + > +#ifdef CONFIG_KEY_NOTIFICATIONS > +/** > + * smack_watch_key - Smack access to watch a key for notifications. > + * @key: The key to be watched > + * > + * Return 0 if the @watch->cred has permission to read from the key object and > + * an error otherwise. > + */ > +static int smack_watch_key(struct key *key) > +{ > + struct smk_audit_info ad; > + struct smack_known *tkp = smk_of_current(); > + int rc; > + > + if (key == NULL) > + return -EINVAL; > + /* > + * If the key hasn't been initialized give it access so that > + * it may do so. > + */ > + if (key->security == NULL) > + return 0; > + /* > + * This should not occur > + */ > + if (tkp == NULL) > + return -EACCES; > + > + if (smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred())) > + return 0; > + > +#ifdef CONFIG_AUDIT > + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_KEY); > + ad.a.u.key_struct.key = key->serial; > + ad.a.u.key_struct.key_desc = key->description; > +#endif > + rc = smk_access(tkp, key->security, MAY_READ, &ad); > + rc = smk_bu_note("key watch", tkp, key->security, MAY_READ, rc); > + return rc; > +} > +#endif /* CONFIG_KEY_NOTIFICATIONS */ > #endif /* CONFIG_KEYS */ > > +#ifdef CONFIG_WATCH_QUEUE > +/** > + * smack_post_notification - Smack access to post a notification to a queue > + * @w_cred: The credentials of the watcher. > + * @cred: The credentials of the event source (may be NULL). > + * @n: The notification message to be posted. > + */ > +static int smack_post_notification(const struct cred *w_cred, > + const struct cred *cred, > + struct watch_notification *n) > +{ > + struct smk_audit_info ad; > + struct smack_known *subj, *obj; > + int rc; > + > + /* Always let maintenance notifications through. */ > + if (n->type == WATCH_TYPE_META) > + return 0; > + > + if (!cred) > + return 0; > + subj = smk_of_task(smack_cred(cred)); > + obj = smk_of_task(smack_cred(w_cred)); > + > + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_NOTIFICATION); > + rc = smk_access(subj, obj, MAY_WRITE, &ad); > + rc = smk_bu_note("notification", subj, obj, MAY_WRITE, rc); > + return rc; > +} > +#endif /* CONFIG_WATCH_QUEUE */ > + > /* > * Smack Audit hooks > * > @@ -4710,8 +4783,15 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(key_free, smack_key_free), > LSM_HOOK_INIT(key_permission, smack_key_permission), > LSM_HOOK_INIT(key_getsecurity, smack_key_getsecurity), > +#ifdef CONFIG_KEY_NOTIFICATIONS > + LSM_HOOK_INIT(watch_key, smack_watch_key), > +#endif > #endif /* CONFIG_KEYS */ > > +#ifdef CONFIG_WATCH_QUEUE > + LSM_HOOK_INIT(post_notification, smack_post_notification), > +#endif > + > /* Audit hooks */ > #ifdef CONFIG_AUDIT > LSM_HOOK_INIT(audit_rule_init, smack_audit_rule_init), >
Casey Schaufler <casey@schaufler-ca.com> wrote: > I tried running your key tests and they fail in "keyctl/move/valid", > with 11 FAILED messages, finally hanging after "UNLINK KEY FROM SESSION". > It's possible that my Fedora26 system is somehow incompatible with the > tests. I don't see anything in your code that would cause this, as the > Smack policy on the system shouldn't restrict any access. Can you go into keyutils/tests/keyctl/move/valid/ and grab the test.out file? I presume you're running with an upstream-ish kernel and a cutting edge keyutils installed? David
On 9/3/2019 8:41 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >> I tried running your key tests and they fail in "keyctl/move/valid", >> with 11 FAILED messages, finally hanging after "UNLINK KEY FROM SESSION". >> It's possible that my Fedora26 system is somehow incompatible with the >> tests. I don't see anything in your code that would cause this, as the >> Smack policy on the system shouldn't restrict any access. > Can you go into keyutils/tests/keyctl/move/valid/ and grab the test.out file? Inline below > I presume you're running with an upstream-ish kernel Built from your tree. It's possible I've missed an important CONFIG or two. > and a cutting edge > keyutils installed? Also built from your tree. > > David $ cat test.out ++++ BEGINNING TEST +++ ADD KEYRING keyctl newring wibble @s 1065401533 +++ ADD KEY keyctl add user lizard gizzard 1065401533 483362336 +++ LIST KEYRING WITH ONE keyctl rlist 1065401533 483362336 +++ MOVE KEY 1 keyctl move 483362336 1065401533 @s keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ CHECK KEY LINKAGE keyctl rlist @s 1065401533 === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ CHECK KEY REMOVED keyctl rlist 1065401533 483362336 === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ MOVE KEY 2 keyctl move 483362336 1065401533 @s keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ FORCE MOVE KEY 2 keyctl move -f 483362336 1065401533 @s keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ MOVE KEY 3 keyctl move 483362336 @s 1065401533 keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ MOVE KEY 4 keyctl move -f 483362336 @s 1065401533 keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 \_ user: lizard ============== +++ ADD KEY 2 keyctl add user lizard gizzard @s 898499184 +++ MOVE KEY 5 keyctl move 483362336 1065401533 @s keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 | \_ user: lizard 898499184 --alswrv 0 0 \_ user: lizard ============== +++ CHECK KEY UNMOVED keyctl rlist 1065401533 483362336 +++ CHECK KEY UNDISPLACED keyctl rlist @s 1065401533 898499184 +++ FORCE MOVE KEY 6 keyctl move -f 483362336 1065401533 @s keyctl_move: Operation not supported === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 | \_ user: lizard 898499184 --alswrv 0 0 \_ user: lizard ============== +++ CHECK KEY REMOVED keyctl rlist 1065401533 483362336 === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 | \_ user: lizard 898499184 --alswrv 0 0 \_ user: lizard ============== +++ CHECK KEY DISPLACED keyctl rlist @s 1065401533 898499184 === FAILED === Session Keyring 680859405 --alswrv 0 0 keyring: RHTS/keyctl/32472 1065401533 --alswrv 0 0 \_ keyring: wibble 483362336 --alswrv 0 0 | \_ user: lizard 898499184 --alswrv 0 0 \_ user: lizard ============== +++ UNLINK KEY FROM SESSION keyctl unlink 483362336 @s +++ WAITING FOR KEY TO BE UNLINKED keyctl unlink 483362336 @s keyctl_unlink: No such file or directory keyctl unlink 483362336 @s keyctl_unlink: No such file or directory ...
Casey Schaufler <casey@schaufler-ca.com> wrote: > Built from your tree. What branch? keys-next? > keyctl move 483362336 1065401533 @s > keyctl_move: Operation not supported Odd. That should be unconditional if you have CONFIG_KEYS and v5.3-rc1. Can you try: keyctl supports or just: keyctl add user a a @s which will give you an id, say 1234, then: keyctl move 1234 @s @u see if that works. David
On 9/3/2019 11:06 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >> Built from your tree. > What branch? keys-next? I rebuilt with keys-next, updated the tests again, and now the suite looks to be running trouble free. I do see a message SKIP DUE TO DISABLED SELINUX which I take to mean that there is an SELinux specific test. > >> keyctl move 483362336 1065401533 @s >> keyctl_move: Operation not supported > Odd. That should be unconditional if you have CONFIG_KEYS and v5.3-rc1. Can > you try: > > keyctl supports > > or just: > > keyctl add user a a @s > > which will give you an id, say 1234, then: > > keyctl move 1234 @s @u > > see if that works. > > David
Casey Schaufler <casey@schaufler-ca.com> wrote: > I rebuilt with keys-next, updated the tests again, and now > the suite looks to be running trouble free. Glad to hear that, thanks. > I do see a message SKIP DUE TO DISABLED SELINUX which I take to mean that > there is an SELinux specific test. tests/bugzillas/bz1031154/runtest.sh David
Casey Schaufler <casey@schaufler-ca.com> wrote: > I rebuilt with keys-next, updated the tests again, and now > the suite looks to be running trouble free. Can I put you down as an Acked-by or something on this patch? Thanks, David
On 9/4/2019 5:08 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >> I rebuilt with keys-next, updated the tests again, and now >> the suite looks to be running trouble free. > Can I put you down as an Acked-by or something on this patch? I haven't done anything to see if the patch is actually useful. I don't have much (read: anything) in the way of key tests for Smack, so I can't say if this is what I want long term. But as it does appear harmless, yes, you can add my Acked-by on this. > > Thanks, > David
diff --git a/include/linux/lsm_audit.h b/include/linux/lsm_audit.h index 915330abf6e5..734d67889826 100644 --- a/include/linux/lsm_audit.h +++ b/include/linux/lsm_audit.h @@ -74,6 +74,7 @@ struct common_audit_data { #define LSM_AUDIT_DATA_FILE 12 #define LSM_AUDIT_DATA_IBPKEY 13 #define LSM_AUDIT_DATA_IBENDPORT 14 +#define LSM_AUDIT_DATA_NOTIFICATION 15 union { struct path path; struct dentry *dentry; diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 4c5e5a438f8b..1c2a908c6446 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -4274,7 +4274,7 @@ static int smack_key_permission(key_ref_t key_ref, if (tkp == NULL) return -EACCES; - if (smack_privileged_cred(CAP_MAC_OVERRIDE, cred)) + if (smack_privileged(CAP_MAC_OVERRIDE)) return 0; #ifdef CONFIG_AUDIT @@ -4320,8 +4320,81 @@ static int smack_key_getsecurity(struct key *key, char **_buffer) return length; } + +#ifdef CONFIG_KEY_NOTIFICATIONS +/** + * smack_watch_key - Smack access to watch a key for notifications. + * @key: The key to be watched + * + * Return 0 if the @watch->cred has permission to read from the key object and + * an error otherwise. + */ +static int smack_watch_key(struct key *key) +{ + struct smk_audit_info ad; + struct smack_known *tkp = smk_of_current(); + int rc; + + if (key == NULL) + return -EINVAL; + /* + * If the key hasn't been initialized give it access so that + * it may do so. + */ + if (key->security == NULL) + return 0; + /* + * This should not occur + */ + if (tkp == NULL) + return -EACCES; + + if (smack_privileged_cred(CAP_MAC_OVERRIDE, current_cred())) + return 0; + +#ifdef CONFIG_AUDIT + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_KEY); + ad.a.u.key_struct.key = key->serial; + ad.a.u.key_struct.key_desc = key->description; +#endif + rc = smk_access(tkp, key->security, MAY_READ, &ad); + rc = smk_bu_note("key watch", tkp, key->security, MAY_READ, rc); + return rc; +} +#endif /* CONFIG_KEY_NOTIFICATIONS */ #endif /* CONFIG_KEYS */ +#ifdef CONFIG_WATCH_QUEUE +/** + * smack_post_notification - Smack access to post a notification to a queue + * @w_cred: The credentials of the watcher. + * @cred: The credentials of the event source (may be NULL). + * @n: The notification message to be posted. + */ +static int smack_post_notification(const struct cred *w_cred, + const struct cred *cred, + struct watch_notification *n) +{ + struct smk_audit_info ad; + struct smack_known *subj, *obj; + int rc; + + /* Always let maintenance notifications through. */ + if (n->type == WATCH_TYPE_META) + return 0; + + if (!cred) + return 0; + subj = smk_of_task(smack_cred(cred)); + obj = smk_of_task(smack_cred(w_cred)); + + smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_NOTIFICATION); + rc = smk_access(subj, obj, MAY_WRITE, &ad); + rc = smk_bu_note("notification", subj, obj, MAY_WRITE, rc); + return rc; +} +#endif /* CONFIG_WATCH_QUEUE */ + /* * Smack Audit hooks * @@ -4710,8 +4783,15 @@ static struct security_hook_list smack_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(key_free, smack_key_free), LSM_HOOK_INIT(key_permission, smack_key_permission), LSM_HOOK_INIT(key_getsecurity, smack_key_getsecurity), +#ifdef CONFIG_KEY_NOTIFICATIONS + LSM_HOOK_INIT(watch_key, smack_watch_key), +#endif #endif /* CONFIG_KEYS */ +#ifdef CONFIG_WATCH_QUEUE + LSM_HOOK_INIT(post_notification, smack_post_notification), +#endif + /* Audit hooks */ #ifdef CONFIG_AUDIT LSM_HOOK_INIT(audit_rule_init, smack_audit_rule_init),
Implement the watch_key security hook in Smack to make sure that a key grants the caller Read permission in order to set a watch on a key. Also implement the post_notification security hook to make sure that the notification source is granted Write permission by the watch queue. 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> --- include/linux/lsm_audit.h | 1 + security/smack/smack_lsm.c | 82 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 1 deletion(-)