From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Grubb Subject: Re: [PATCH 1/1]: ipsec audit Date: Mon, 30 Oct 2006 12:01:29 -0500 Message-ID: <200610301201.30040.sgrubb@redhat.com> References: <1161908598.17737.280.camel@faith.austin.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1161908598.17737.280.camel@faith.austin.ibm.com> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Joy Latten Cc: linux-audit@redhat.com List-Id: linux-audit@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 > #include > #include > +#include > > 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);