Re: [RFC][PATCH] Smack<->Audit integration
On Mon, Mar 10, 2008 at 09:07:08AM -0700, Casey Schaufler wrote:
--- "Ahmed S. Darwish" <darwish.07(a)gmail.com> wrote:
...
>
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index c444f48..2c8bb4c 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -57,6 +57,15 @@ struct inode_smack {
> char *smk_inode; /* label of the fso */
> struct mutex smk_lock; /* initialization lock */
> int smk_flags; /* smack inode flags */
> + int secid; /* security identifier */
No.
Secid's are horrid things and every effort should be made to
expunge them from the known universe. Under no circumstances
should thier use be expanded. The only reason Smack has them
at all is because certain interfaces that in my mind should
have known better use them. If you must deal with secids,
and for this round of audit I think that's a given, use
smack_to_secid(sp->smk_inode) where you need to. If there's a
real performance issue apply intelligence to smack_to_secid
instead of storing the secid. There ought to be a way to
use container_of to do smack_to_secid, but I had trouble with
that and moved along without figuring out what I had done
wrong.
mm .. I should have remembered the un-official Smack motto:
"Everything is a label, and whenever possible, this label is allocated
once through system lifetime"
About performance, yes there'll be issues searching labels espicially
in audit_rule_match() which got called at the end of every system call.
I'll try it using container_of (it should work at the end).
...
> @@ -1696,9 +1738,9 @@ static int smack_msg_queue_alloc_security(struct
> msg_queue *msq)
> */
> static void smack_msg_queue_free_security(struct msg_queue *msq)
> {
> - struct kern_ipc_perm *kisp = &msq->q_perm;
> + struct kern_ipc_perm *ipcp = &msq->q_perm;
>
> - kisp->security = NULL;
> + kfree(ipcp->security);
> }
Don't you just hate repetative reviewers?
Probably hating secids with passion :) ?
Admittedly, after some thinking I felt now that they don't fit with
the Smack model very well.
...
> +
> +/**
> + * smack_audit_rule_match - Audit given secid identified object ?
> + * @secid: Security id to test
> + * @field: Message flags given from user-space
> + * @op: Required operation (only equality is allowed)
> + * @vrule: Smack audit rule that will be checked against the secid object
> + * @actx: audit context associated with the check (used for Audit logging)
> + *
> + * This is the core Audit hook. It's used to identify objects like
> + * syscalls and inodes requested from user-space to be audited from
> + * remaining kernel objects.
> + */
> +static int smack_audit_rule_match(u32 secid, u32 field, u32 op, void *vrule,
> + struct audit_context *actx)
> +{
> + struct smack_known *smk_rule = vrule;
char *smack;
More of "everything is a label".
> +
> + if (!smk_rule) {
> + audit_log(actx, GFP_KERNEL, AUDIT_SELINUX_ERR,
> + "Smack: missing rule\n");
> + return -ENOENT;
> + }
> +
> + if (field != AUDIT_SUBJ_USER && field != AUDIT_OBJ_USER)
> + return 0;
> +
smack = smack_from_secid(secid);
> + if (op == AUDIT_EQUAL)
> + return (smk_rule->smk_secid == secid);
> + if (op == AUDIT_NOT_EQUAL)
> + return (smk_rule->smk_secid != secid);
if (op == AUDIT_EQUAL)
return (smk_rule->smk_smack == smack);
if (op == AUDIT_NOT_EQUAL)
return (smk_rule->smk_smack == smack);
You've meant using the short-circuit:
smk_rule->smk_smack == smack || strnmp(smack, ..., ..)
Right ?
...
> +
> +/**
> + * smack_audit_rule_free - free internal audit rule representation
> + * @vrule: rule to be freed.
> + *
> + * No memory was allocated in audit_rule_init.
> + */
> +static void smack_audit_rule_free(void *vrule)
> +{
> + /* No-op */
> +}
This little no-op was the only thing that was agreed upon ;)
...
Casey Schaufler
casey(a)schaufler-ca.com
Regards,
--
"Better to light a candle, than curse the darkness"
Ahmed S. Darwish
Homepage: http://darwish.07.googlepages.com
Blog: http://darwish-07.blogspot.com