Re: [PATCH 1/1]: ipsec audit

[Steve Grubb <sgrubb@redhat.com>]

On Thursday 26 October 2006 20:23, Joy Latten wrote:
> Please let me know if this patch is acceptable.

Sorry about the delay...but I have some feedback. The patch got wrapped by 
email client, so I can't apply it. But I can see a few issues.


> diff -urpN linux-2.6.18.ppc64/include/linux/audit.h
> linux-2.6.18.ppc64.patch/include/linux/audit.h
> --- linux-2.6.18.ppc64/include/linux/audit.h	2006-10-26
> 03:10:10.000000000 -0500
> +++ linux-2.6.18.ppc64.patch/include/linux/audit.h	2006-10-26
> 07:04:08.000000000 -0500
> @@ -100,6 +100,10 @@
>  #define AUDIT_MAC_CIPSOV4_DEL	1408	/* NetLabel: del CIPSOv4 DOI entry
> */
>  #define AUDIT_MAC_MAP_ADD	1409	/* NetLabel: add LSM domain mapping */
>  #define AUDIT_MAC_MAP_DEL	1410	/* NetLabel: del LSM domain mapping */
> +#define AUDIT_MAC_IPSEC_ADDSA   1411	/* Add a XFRM state */
> +#define AUDIT_MAC_IPSEC_DELSA   1412	/* Delete a XFRM state */
> +#define AUDIT_MAC_IPSEC_ADDSPD  1413	/* Add a XFRM policy */
> +#define AUDIT_MAC_IPSEC_DELSPD  1414	/* Delete a XFRM policy */

Looks good.

> diff -urpN linux-2.6.18.ppc64/net/xfrm/xfrm_policy.c
> linux-2.6.18.ppc64.patch/net/xfrm/xfrm_policy.c
> --- linux-2.6.18.ppc64/net/xfrm/xfrm_policy.c	2006-10-26
> 03:10:11.000000000 -0500
> +++ linux-2.6.18.ppc64.patch/net/xfrm/xfrm_policy.c	2006-10-26
> 07:04:08.000000000 -0500
> @@ -374,13 +374,21 @@ static void xfrm_policy_gc_task(void *da
>   * entry dead. The rule must be unlinked from lists to the moment.
>   */
>
> -static void xfrm_policy_kill(struct xfrm_policy *policy)
> +static void xfrm_policy_kill(struct xfrm_policy *policy, uid_t auid)
>  {
>  	int dead;
>
>  	write_lock_bh(&policy->lock);
>  	dead = policy->dead;
>  	policy->dead = 1;
> +
> +	if (policy->security)

If this is NULL we get no audit message?

> +		audit_log(current->audit_context, GFP_ATOMIC,
> +			  AUDIT_MAC_IPSEC_DELSPD,
> +			  "spd delete: auid=%u 

subj=  should be after auid field. This means that you need to collect the 
secid out of the netlink packets and the audit context and send it as well.

> ctx_alg=%d ctx_doi=%d

I'd drop the ctx in favor of sp.

> ctx=%s",  
> +			  auid, policy->security->ctx_alg,
> +			  policy->security->ctx_doi, policy->security->ctx_str);
> +

Also, the last field should be res=%u. res is the results, 1 meaning success 
and 0 failure. This means we want this function called on failure, too.

>  	write_unlock_bh(&policy->lock);
>
>  	if (unlikely(dead)) {
> @@ -417,7 +425,7 @@ static u32 xfrm_gen_index(int dir)
>  	}
>  }
>
> -int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
> +int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl,
> uid_t auid)
>  {
>  	struct xfrm_policy *pol, **p;
>  	struct xfrm_policy *delpol = NULL;
> @@ -460,10 +468,18 @@ int xfrm_policy_insert(int dir, struct x
>  	write_unlock_bh(&xfrm_policy_lock);
>
>  	if (delpol)
> -		xfrm_policy_kill(delpol);
> +		xfrm_policy_kill(delpol, auid);
>
>  	read_lock_bh(&xfrm_policy_lock);
>  	gc_list = NULL;
> +
> +	if (policy->security)
> +		audit_log(current->audit_context, GFP_ATOMIC,
> +			  AUDIT_MAC_IPSEC_ADDSPD,
> +			  "spd add: auid=%u ctx_alg=%d ctx_doi=%d ctx=%s",
> +			  auid, policy->security->ctx_alg,
> +			  policy->security->ctx_doi, policy->security->ctx_str);
> +

Same comments apply here.

> diff -urpN linux-2.6.18.ppc64/net/xfrm/xfrm_state.c
> linux-2.6.18.ppc64.patch/net/xfrm/xfrm_state.c
> --- linux-2.6.18.ppc64/net/xfrm/xfrm_state.c	2006-10-26
> 03:10:09.000000000 -0500
> +++ linux-2.6.18.ppc64.patch/net/xfrm/xfrm_state.c	2006-10-26
> 07:04:08.000000000 -0500
> @@ -244,6 +245,13 @@ int __xfrm_state_delete(struct xfrm_stat
>  			list_del(&x->byspi);
>  			__xfrm_state_put(x);
>  		}
> +		if (x->security)
> +			audit_log(current->audit_context, GFP_ATOMIC,
> +				  AUDIT_MAC_IPSEC_ADDSA,
> +				  "SAD delete: auid=%u 

need subj=

> ctx_alg=%d ctx_doi=%d ctx=%s", 

I'd drop ctx in favor of sa.

> +				  auid, x->security->ctx_alg,
> +				  x->security->ctx_doi, x->security->ctx_str);

res=%u.

> @@ -441,7 +449,7 @@ out:
>  	return x;
>  }
>
> -static void __xfrm_state_insert(struct xfrm_state *x)
> +static void __xfrm_state_insert(struct xfrm_state *x, uid_t auid)
>  {
>  	unsigned h = xfrm_dst_hash(&x->id.daddr, x->props.family);
>
> @@ -461,12 +469,20 @@ static void __xfrm_state_insert(struct x
>  		xfrm_state_hold(x);
>
>  	wake_up(&km_waitq);
> +
> +	if (x->security)
> +		audit_log(current->audit_context, GFP_ATOMIC,
> +			  AUDIT_MAC_IPSEC_ADDSA,
> +			  "SAD add: auid=%u ctx_alg=%d ctx_doi=%d, ctx=%s",
> +			  auid, x->security->ctx_alg, x->security->ctx_doi,
> +			  x->security->ctx_str);

Same comments as the previous.

> @@ -1125,11 +1141,15 @@ static void xfrm_state_put_afinfo(struct
>  /* Temporarily located here until net/xfrm/xfrm_tunnel.c is created */
>  void xfrm_state_delete_tunnel(struct xfrm_state *x)
>  {
> +	uid_t auid;
> +
> +	auid = audit_get_loginuid(current->audit_context);
> +
>  	if (x->tunnel) {
>  		struct xfrm_state *t = x->tunnel;
>
>  		if (atomic_read(&t->tunnel_users) == 2)
> -			xfrm_state_delete(t);
> +			xfrm_state_delete(t, auid);

auid appears to only get used here, you can probably move get_loginuid inside 
the if statement so the other path is not penalized.

> diff -urpN linux-2.6.18.ppc64/net/xfrm/xfrm_user.c
> linux-2.6.18.ppc64.patch/net/xfrm/xfrm_user.c
> --- linux-2.6.18.ppc64/net/xfrm/xfrm_user.c	2006-10-26
> 03:10:11.000000000 -0500
> +++ linux-2.6.18.ppc64.patch/net/xfrm/xfrm_user.c	2006-10-26
> 07:04:08.000000000 -0500
> @@ -27,6 +27,7 @@
>  #include <net/xfrm.h>
>  #include <net/netlink.h>
>  #include <asm/uaccess.h>
> +#include <linux/audit.h>
>
>  static int verify_one_alg(struct rtattr **xfrma, enum xfrm_attr_type_t
> type)
>  {
> @@ -396,9 +397,9 @@ static int xfrm_add_sa(struct sk_buff *s
>
>  	xfrm_state_hold(x);
>  	if (nlh->nlmsg_type == XFRM_MSG_NEWSA)
> -		err = xfrm_state_add(x);
> +		err = xfrm_state_add(x, NETLINK_CB(skb).loginuid);

Also need to collect NETLINK_CB(skb).sid in this and other places below.

>  	else
> -		err = xfrm_state_update(x);
> +		err = xfrm_state_update(x, NETLINK_CB(skb).loginuid);
>
>  	if (err < 0) {
>  		x->km.state = XFRM_STATE_DEAD;
> @@ -435,7 +436,7 @@ static int xfrm_del_sa(struct sk_buff *s
>  		goto out;
>  	}
>
> -	err = xfrm_state_delete(x);
> +	err = xfrm_state_delete(x, NETLINK_CB(skb).loginuid);
>  	if (err < 0)
>  		goto out;
>
> @@ -859,7 +860,7 @@ static int xfrm_add_policy(struct sk_buf
>  	 * in netlink excl is a flag and you wouldnt need
>  	 * a type XFRM_MSG_UPDPOLICY - JHS */
>  	excl = nlh->nlmsg_type == XFRM_MSG_NEWPOLICY;
> -	err = xfrm_policy_insert(p->dir, xp, excl);
> +	err = xfrm_policy_insert(p->dir, xp, excl, NETLINK_CB(skb).loginuid);
>  	if (err) {
>  		security_xfrm_policy_free(xp);
>  		kfree(xp);
> @@ -1026,6 +1027,7 @@ static int xfrm_get_policy(struct sk_buf
>  	int err;
>  	struct km_event c;
>  	int delete;
> +	uid_t auid;
>
>  	p = NLMSG_DATA(nlh);
>  	delete = nlh->nlmsg_type == XFRM_MSG_DELPOLICY;
> @@ -1034,8 +1036,9 @@ static int xfrm_get_policy(struct sk_buf
>  	if (err)
>  		return err;
>
> +	auid = NETLINK_CB(skb).loginuid;
>  	if (p->index)
> -		xp = xfrm_policy_byid(p->dir, p->index, delete);
> +		xp = xfrm_policy_byid(p->dir, p->index, delete, auid);
>  	else {
>  		struct rtattr **rtattrs = (struct rtattr **)xfrma;
>  		struct rtattr *rt = rtattrs[XFRMA_SEC_CTX-1];
> @@ -1052,7 +1055,7 @@ static int xfrm_get_policy(struct sk_buf
>  			if ((err = security_xfrm_policy_alloc(&tmp, uctx)))
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(p->dir, &p->sel, tmp.security, delete);
> +		xp = xfrm_policy_bysel_ctx(p->dir, &p->sel, tmp.security, delete,
> auid);
>  		security_xfrm_policy_free(&tmp);
>  	}
>  	if (xp == NULL)
> @@ -1090,7 +1093,7 @@ static int xfrm_flush_sa(struct sk_buff
>  	struct km_event c;
>  	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
>
> -	xfrm_state_flush(p->proto);
> +	xfrm_state_flush(p->proto, NETLINK_CB(skb).loginuid);
>  	c.data.proto = p->proto;
>  	c.event = nlh->nlmsg_type;
>  	c.seq = nlh->nlmsg_seq;
> @@ -1237,7 +1240,7 @@ static int xfrm_flush_policy(struct sk_b
>  {
>  struct km_event c;
>
> -	xfrm_policy_flush();
> +	xfrm_policy_flush(NETLINK_CB(skb).loginuid);
>  	c.event = nlh->nlmsg_type;
>  	c.seq = nlh->nlmsg_seq;
>  	c.pid = nlh->nlmsg_pid;
> @@ -1251,9 +1254,12 @@ static int xfrm_add_pol_expire(struct sk
>  	struct xfrm_user_polexpire *up = NLMSG_DATA(nlh);
>  	struct xfrm_userpolicy_info *p = &up->pol;
>  	int err = -ENOENT;
> +	uid_t auid;
> +
> +	auid = NETLINK_CB(skb).loginuid;
>
>  	if (p->index)
> -		xp = xfrm_policy_byid(p->dir, p->index, 0);
> +		xp = xfrm_policy_byid(p->dir, p->index, 0, auid);
>  	else {
>  		struct rtattr **rtattrs = (struct rtattr **)xfrma;
>  		struct rtattr *rt = rtattrs[XFRMA_SEC_CTX-1];
> @@ -1270,7 +1276,7 @@ static int xfrm_add_pol_expire(struct sk
>  			if ((err = security_xfrm_policy_alloc(&tmp, uctx)))
>  				return err;
>  		}
> -		xp = xfrm_policy_bysel_ctx(p->dir, &p->sel, tmp.security, 0);
> +		xp = xfrm_policy_bysel_ctx(p->dir, &p->sel, tmp.security, 0, auid);
>  		security_xfrm_policy_free(&tmp);
>  	}
>
> @@ -1285,7 +1291,7 @@ static int xfrm_add_pol_expire(struct sk
>  	read_unlock(&xp->lock);
>  	err = 0;
>  	if (up->hard) {
> -		xfrm_policy_delete(xp, p->dir);
> +		xfrm_policy_delete(xp, p->dir, auid);
>  	} else {
>  		// reset the timers here?
>  		printk("Dont know what to do with soft policy expire\n");
> @@ -1318,7 +1324,7 @@ static int xfrm_add_sa_expire(struct sk_
>  	km_state_expired(x, ue->hard, current->pid);
>
>  	if (ue->hard)
> -		__xfrm_state_delete(x);
> +		__xfrm_state_delete(x, NETLINK_CB(skb).loginuid);
>  out:
>  	spin_unlock_bh(&x->lock);
>  	xfrm_state_put(x);