Re: [PATCH]: Add security check before flushing SAD/SPD

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

Sending again since one of the email addresses was incorrect.

----------------

Ok, I have made improvements based on James' and Eric's comments.

Regards,
Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>

diff -urpN linux-2.6.20.orig/include/net/xfrm.h linux-2.6.20.patch/include/net/xfrm.h
--- linux-2.6.20.orig/include/net/xfrm.h	2007-03-23 11:01:48.000000000 -0500
+++ linux-2.6.20.patch/include/net/xfrm.h	2007-03-25 21:36:05.000000000 -0500
@@ -937,7 +937,7 @@ static inline int xfrm_state_sort(struct
 #endif
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_notify(struct xfrm_state *x, int event);
@@ -991,13 +991,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.20.orig/net/key/af_key.c linux-2.6.20.patch/net/key/af_key.c
--- linux-2.6.20.orig/net/key/af_key.c	2007-03-23 11:02:49.000000000 -0500
+++ linux-2.6.20.patch/net/key/af_key.c	2007-03-25 21:34:35.000000000 -0500
@@ -1645,6 +1645,7 @@ static int pfkey_flush(struct sock *sk, 
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1652,7 +1653,9 @@ static int pfkey_flush(struct sock *sk, 
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2628,10 +2631,13 @@ static int pfkey_spdflush(struct sock *s
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20.patch/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c	2007-03-26 17:19:26.000000000 -0500
@@ -813,11 +813,67 @@ struct xfrm_policy *xfrm_policy_byid(u8 
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			err = security_xfrm_policy_delete(pol);
+			if (err) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD, 0,
+					       pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				err = security_xfrm_policy_delete(pol);
+				if (err) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       0, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -870,7 +926,9 @@ void xfrm_policy_flush(u8 type, struct x
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2485,4 +2543,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c	2007-03-26 17:19:56.000000000 -0500
@@ -388,12 +388,48 @@ int xfrm_state_delete(struct xfrm_state 
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               0, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -416,8 +452,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_user.c linux-2.6.20.patch/net/xfrm/xfrm_user.c
--- linux-2.6.20.orig/net/xfrm/xfrm_user.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_user.c	2007-03-25 21:33:52.000000000 -0500
@@ -1312,10 +1312,13 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1476,7 +1479,9 @@ static int xfrm_flush_policy(struct sk_b
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 26 Mar 2007, Joy Latten wrote:

> 
> Sending again since one of the email addresses was incorrect.
> 
> ----------------
> 
> Ok, I have made improvements based on James' and Eric's comments.
> 

Acked-by: James Morris <jmorris@namei.org>


> +			if (xfrm_id_proto_match(x->id.proto, proto) &&
> +			   (err = security_xfrm_state_delete(x)) != 0) {


Still not a showstopper, but standard idiom would be nice at some point.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

Some time ago this thread bounced back and forth and seemed to come to
rest with the patch below, I cleaned up the comments and put all the
ACKs it received in one place below, but so much time has passed I doubt
if they should still count for free.  I also rediffed the patch against
the latest miller tree.  Is the idea or patch in any way flawed or
unacceptable to people at the moment?

Anyone willing to step up an re-ack the patch to get it moving into the
tree?

-Eric

Currently we check for permission before deleting entries from SAD and
SPD, (see security_xfrm_policy_delete() security_xfrm_state_delete())
However we are not checking for authorization when flushing the SPD and
the SAD completely. It was perhaps missed in the original security hooks
patch.

This patch adds a security check when flushing entries from the SAD and
SPD.  It runs the entire database and checks each entry for a denial.
If the process attempting the flush is unable to remove all of the
entries a denial is logged the the flush function returns an error
without removing anything.

This is particularly useful when a process may need to create or delete
its own xfrm entries used for things like labeled networking but that
same process should not be able to delete other entries or flush the
entire database.

WAS Signed-off-by: Signed-off-by: Joy Latten<latten@austin.ibm.com> NOT NOW
WAS Acked-by: James Morris <jmorris@namei.org> NOT NOW
WAS Acked-by: Eric Paris <eparis@parisplace.org> NOT NOW

---

 include/net/xfrm.h     |    6 ++--
 net/key/af_key.c       |   10 ++++++-
 net/xfrm/xfrm_policy.c |   63 +++++++++++++++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_state.c  |   46 ++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_user.c   |    9 +++++-
 5 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 90185e8..311f25a 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -964,7 +964,7 @@ struct xfrmk_spdinfo {
 
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern void xfrm_sad_getinfo(struct xfrmk_sadinfo *si);
 extern void xfrm_spd_getinfo(struct xfrmk_spdinfo *si);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
@@ -1020,13 +1020,13 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(u8 type, int dir,
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index d302dda..0f8304b 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1682,6 +1682,7 @@ static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hd
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1689,7 +1690,9 @@ static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hd
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2683,10 +2686,13 @@ static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 64a3751..157bfbd 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -834,11 +834,67 @@ struct xfrm_policy *xfrm_policy_byid(u8 type, int dir, u32 id, int delete,
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			err = security_xfrm_policy_delete(pol);
+			if (err) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD, 0,
+					       pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				err = security_xfrm_policy_delete(pol);
+				if (err) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       0, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -891,7 +947,9 @@ void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2583,4 +2641,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 372f06e..2cc9d87 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -391,12 +391,48 @@ int xfrm_state_delete(struct xfrm_state *x)
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               0, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -419,8 +455,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b14c7e5..c06883b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1418,10 +1418,13 @@ static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1582,7 +1585,9 @@ static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

Some time ago this thread bounced back and forth and seemed to come to
rest with the patch below, I cleaned up the comments and put all the
ACKs it received in one place below, but so much time has passed I doubt
if they should still count for free.  I also rediffed the patch against
the latest miller tree.  Is the idea or patch in any way flawed or
unacceptable to people at the moment?

Anyone willing to step up an re-ack the patch to get it moving into the
tree?

-Eric

Currently we check for permission before deleting entries from SAD and
SPD, (see security_xfrm_policy_delete() security_xfrm_state_delete())
However we are not checking for authorization when flushing the SPD and
the SAD completely. It was perhaps missed in the original security hooks
patch.

This patch adds a security check when flushing entries from the SAD and
SPD.  It runs the entire database and checks each entry for a denial.
If the process attempting the flush is unable to remove all of the
entries a denial is logged the the flush function returns an error
without removing anything.

This is particularly useful when a process may need to create or delete
its own xfrm entries used for things like labeled networking but that
same process should not be able to delete other entries or flush the
entire database.

WAS Signed-off-by: Signed-off-by: Joy Latten<latten@austin.ibm.com> NOT NOW
WAS Acked-by: James Morris <jmorris@namei.org> NOT NOW
WAS Acked-by: Eric Paris <eparis@parisplace.org> NOT NOW

---

 include/net/xfrm.h     |    6 ++--
 net/key/af_key.c       |   10 ++++++-
 net/xfrm/xfrm_policy.c |   63 +++++++++++++++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_state.c  |   46 ++++++++++++++++++++++++++++++++--
 net/xfrm/xfrm_user.c   |    9 +++++-
 5 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 90185e8..311f25a 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -964,7 +964,7 @@ struct xfrmk_spdinfo {
 
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern void xfrm_sad_getinfo(struct xfrmk_sadinfo *si);
 extern void xfrm_spd_getinfo(struct xfrmk_spdinfo *si);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
@@ -1020,13 +1020,13 @@ struct xfrm_policy *xfrm_policy_bysel_ctx(u8 type, int dir,
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff --git a/net/key/af_key.c b/net/key/af_key.c
index d302dda..0f8304b 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1682,6 +1682,7 @@ static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hd
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1689,7 +1690,9 @@ static int pfkey_flush(struct sock *sk, struct sk_buff *skb, struct sadb_msg *hd
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2683,10 +2686,13 @@ static int pfkey_spdflush(struct sock *sk, struct sk_buff *skb, struct sadb_msg
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 64a3751..157bfbd 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -834,11 +834,67 @@ struct xfrm_policy *xfrm_policy_byid(u8 type, int dir, u32 id, int delete,
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			err = security_xfrm_policy_delete(pol);
+			if (err) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD, 0,
+					       pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				err = security_xfrm_policy_delete(pol);
+				if (err) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       0, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -891,7 +947,9 @@ void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2583,4 +2641,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 372f06e..2cc9d87 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -391,12 +391,48 @@ int xfrm_state_delete(struct xfrm_state *x)
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               0, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -419,8 +455,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index b14c7e5..c06883b 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1418,10 +1418,13 @@ static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1582,7 +1585,9 @@ static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 4 Jun 2007, Eric Paris wrote:

> Some time ago this thread bounced back and forth and seemed to come to
> rest with the patch below, I cleaned up the comments and put all the
> ACKs it received in one place below, but so much time has passed I doubt
> if they should still count for free.  I also rediffed the patch against
> the latest miller tree.  Is the idea or patch in any way flawed or
> unacceptable to people at the moment?
> 
> Anyone willing to step up an re-ack the patch to get it moving into the
> tree?

Looks good to me.

Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 4 Jun 2007, Eric Paris wrote:

> Some time ago this thread bounced back and forth and seemed to come to
> rest with the patch below, I cleaned up the comments and put all the
> ACKs it received in one place below, but so much time has passed I doubt
> if they should still count for free.  I also rediffed the patch against
> the latest miller tree.  Is the idea or patch in any way flawed or
> unacceptable to people at the moment?
> 
> Anyone willing to step up an re-ack the patch to get it moving into the
> tree?

Looks good to me.

Acked-by: James Morris <jmorris@namei.org>


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

I've applied this patch to 

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-davem


Dave, feel free to pull from that branch.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

I've applied this patch to 

git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-davem


Dave, feel free to pull from that branch.



- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[David Miller]

From: James Morris <jmorris@namei.org>
Date: Mon, 4 Jun 2007 19:13:10 -0400 (EDT)

> I've applied this patch to 
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/selinux-2.6.git#for-davem
> 
> 
> Dave, feel free to pull from that branch.

Thanks James, I'll pull that in for my next round of
networking fixes.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Mon, 2007-06-04 at 15:44 -0400, James Morris wrote:
> On Mon, 4 Jun 2007, Eric Paris wrote:
> 
> > Some time ago this thread bounced back and forth and seemed to come to
> > rest with the patch below, I cleaned up the comments and put all the
> > ACKs it received in one place below, but so much time has passed I doubt
> > if they should still count for free.  I also rediffed the patch against
> > the latest miller tree.  Is the idea or patch in any way flawed or
> > unacceptable to people at the moment?
> > 
> > Anyone willing to step up an re-ack the patch to get it moving into the
> > tree?
> 
> Looks good to me.
> 
> Acked-by: James Morris <jmorris@namei.org>
> 
> 

I have also tested with 2.6.22-rc3-git7 and all appears to be working as
expected. 

Acked-by: Joy Latten <latten@austin.ibm.com>

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Mon, 2007-06-04 at 15:44 -0400, James Morris wrote:
> On Mon, 4 Jun 2007, Eric Paris wrote:
> 
> > Some time ago this thread bounced back and forth and seemed to come to
> > rest with the patch below, I cleaned up the comments and put all the
> > ACKs it received in one place below, but so much time has passed I doubt
> > if they should still count for free.  I also rediffed the patch against
> > the latest miller tree.  Is the idea or patch in any way flawed or
> > unacceptable to people at the moment?
> > 
> > Anyone willing to step up an re-ack the patch to get it moving into the
> > tree?
> 
> Looks good to me.
> 
> Acked-by: James Morris <jmorris@namei.org>
> 
> 

I have also tested with 2.6.22-rc3-git7 and all appears to be working as
expected. 

Acked-by: Joy Latten <latten@austin.ibm.com>



--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

I have made improvements based on James' and Eric's comments.

Regards,
Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>

diff -urpN linux-2.6.20.orig/include/net/xfrm.h linux-2.6.20.patch/include/net/xfrm.h
--- linux-2.6.20.orig/include/net/xfrm.h	2007-03-23 11:01:48.000000000 -0500
+++ linux-2.6.20.patch/include/net/xfrm.h	2007-03-25 21:36:05.000000000 -0500
@@ -937,7 +937,7 @@ static inline int xfrm_state_sort(struct
 #endif
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_notify(struct xfrm_state *x, int event);
@@ -991,13 +991,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.20.orig/net/key/af_key.c linux-2.6.20.patch/net/key/af_key.c
--- linux-2.6.20.orig/net/key/af_key.c	2007-03-23 11:02:49.000000000 -0500
+++ linux-2.6.20.patch/net/key/af_key.c	2007-03-25 21:34:35.000000000 -0500
@@ -1645,6 +1645,7 @@ static int pfkey_flush(struct sock *sk, 
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1652,7 +1653,9 @@ static int pfkey_flush(struct sock *sk, 
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2628,10 +2631,13 @@ static int pfkey_spdflush(struct sock *s
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20.patch/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c	2007-03-26 17:19:26.000000000 -0500
@@ -813,11 +813,67 @@ struct xfrm_policy *xfrm_policy_byid(u8 
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			err = security_xfrm_policy_delete(pol);
+			if (err) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD, 0,
+					       pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				err = security_xfrm_policy_delete(pol);
+				if (err) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       0, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -870,7 +926,9 @@ void xfrm_policy_flush(u8 type, struct x
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2485,4 +2543,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c	2007-03-26 17:19:56.000000000 -0500
@@ -388,12 +388,48 @@ int xfrm_state_delete(struct xfrm_state 
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               0, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -416,8 +452,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_user.c linux-2.6.20.patch/net/xfrm/xfrm_user.c
--- linux-2.6.20.orig/net/xfrm/xfrm_user.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_user.c	2007-03-25 21:33:52.000000000 -0500
@@ -1312,10 +1312,13 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1476,7 +1479,9 @@ static int xfrm_flush_policy(struct sk_b
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

I have made improvements based on James' and Eric's comments.

Regards,
Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>

diff -urpN linux-2.6.20.orig/include/net/xfrm.h linux-2.6.20.patch/include/net/xfrm.h
--- linux-2.6.20.orig/include/net/xfrm.h	2007-03-23 11:01:48.000000000 -0500
+++ linux-2.6.20.patch/include/net/xfrm.h	2007-03-25 21:36:05.000000000 -0500
@@ -937,7 +937,7 @@ static inline int xfrm_state_sort(struct
 #endif
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_notify(struct xfrm_state *x, int event);
@@ -991,13 +991,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.20.orig/net/key/af_key.c linux-2.6.20.patch/net/key/af_key.c
--- linux-2.6.20.orig/net/key/af_key.c	2007-03-23 11:02:49.000000000 -0500
+++ linux-2.6.20.patch/net/key/af_key.c	2007-03-25 21:34:35.000000000 -0500
@@ -1645,6 +1645,7 @@ static int pfkey_flush(struct sock *sk, 
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1652,7 +1653,9 @@ static int pfkey_flush(struct sock *sk, 
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2628,10 +2631,13 @@ static int pfkey_spdflush(struct sock *s
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20.patch/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c	2007-03-26 17:19:26.000000000 -0500
@@ -813,11 +813,67 @@ struct xfrm_policy *xfrm_policy_byid(u8 
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			err = security_xfrm_policy_delete(pol);
+			if (err) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD, 0,
+					       pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				err = security_xfrm_policy_delete(pol);
+				if (err) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       0, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -870,7 +926,9 @@ void xfrm_policy_flush(u8 type, struct x
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2485,4 +2543,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c	2007-03-26 17:19:56.000000000 -0500
@@ -388,12 +388,48 @@ int xfrm_state_delete(struct xfrm_state 
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               0, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -416,8 +452,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_user.c linux-2.6.20.patch/net/xfrm/xfrm_user.c
--- linux-2.6.20.orig/net/xfrm/xfrm_user.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_user.c	2007-03-25 21:33:52.000000000 -0500
@@ -1312,10 +1312,13 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1476,7 +1479,9 @@ static int xfrm_flush_policy(struct sk_b
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Mon, 2007-03-26 at 13:39 -0600, Joy Latten wrote:
> +			if ((err = security_xfrm_policy_delete(pol)) != 0) {
> +				xfrm_audit_log(audit_info->loginuid,
> +					       audit_info->secid,
> +					       AUDIT_MAC_IPSEC_DELSPD,
> +					       err ? 0 : 1, pol, NULL);
> +				return err;

In all of the denial log statements you keep the "err ? 0 : 1" which are
common among audit, but in this patch we always know that err is 1. Is
it worth simplifying this down to just a 0 in the all of the
xfrm_audit_log calls?

-Eric

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Mon, 2007-03-26 at 13:39 -0600, Joy Latten wrote:
> +			if ((err = security_xfrm_policy_delete(pol)) != 0) {
> +				xfrm_audit_log(audit_info->loginuid,
> +					       audit_info->secid,
> +					       AUDIT_MAC_IPSEC_DELSPD,
> +					       err ? 0 : 1, pol, NULL);
> +				return err;

In all of the denial log statements you keep the "err ? 0 : 1" which are
common among audit, but in this patch we always know that err is 1. Is
it worth simplifying this down to just a 0 in the all of the
xfrm_audit_log calls?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 26 Mar 2007, James Morris wrote:

> On Mon, 26 Mar 2007, Joy Latten wrote:
> 
> > Signed-off-by: Joy Latten<latten@austin.ibm.com>
> 
> This looks ok to me, although I have a couple of minor issues (which 
> should probably not stop it being merged):
> 
> > +			if ((err = security_xfrm_policy_delete(pol)) != 0) {
> 
> The value of 'err' is implicitly inverted several times in this function 
> (and similarly in the state flush one).  Something like
> 
> 	ret = (fn() != 0);

Oops, ignore the above.

The correct idiom is:

	err = fn();
	if (err) {
		/* handle error */
	}


Please use that, to reduce confusion :-)



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 26 Mar 2007, James Morris wrote:

> On Mon, 26 Mar 2007, Joy Latten wrote:
> 
> > Signed-off-by: Joy Latten<latten@austin.ibm.com>
> 
> This looks ok to me, although I have a couple of minor issues (which 
> should probably not stop it being merged):
> 
> > +			if ((err = security_xfrm_policy_delete(pol)) != 0) {
> 
> The value of 'err' is implicitly inverted several times in this function 
> (and similarly in the state flush one).  Something like
> 
> 	ret = (fn() != 0);

Oops, ignore the above.

The correct idiom is:

	err = fn();
	if (err) {
		/* handle error */
	}


Please use that, to reduce confusion :-)



- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 26 Mar 2007, Joy Latten wrote:

> Signed-off-by: Joy Latten<latten@austin.ibm.com>

This looks ok to me, although I have a couple of minor issues (which 
should probably not stop it being merged):

> +			if ((err = security_xfrm_policy_delete(pol)) != 0) {

The value of 'err' is implicitly inverted several times in this function 
(and similarly in the state flush one).  Something like

	ret = (fn() != 0);

might be better.


> +                }
> +		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {

Tab damage?


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Mon, 26 Mar 2007, Joy Latten wrote:

> Signed-off-by: Joy Latten<latten@austin.ibm.com>

This looks ok to me, although I have a couple of minor issues (which 
should probably not stop it being merged):

> +			if ((err = security_xfrm_policy_delete(pol)) != 0) {

The value of 'err' is implicitly inverted several times in this function 
(and similarly in the state flush one).  Something like

	ret = (fn() != 0);

might be better.


> +                }
> +		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {

Tab damage?


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 20:56 -0400, James Morris wrote:
>On Thu, 22 Mar 2007, Joy Latten wrote:
>> > Perhaps a better semantic would be to fail the entire flush operation if 
>> > one of the security checks failed.  e.g. loop through for permissions 
>> > first, then if all ok, loop through for deletion.
>> > 
>> Ok, will code this up and test it if there are no objections.
>
>I'd suggest making the permission loop a noop if CONFIG_SECURITY=n, via a 
>static inline function.

Reworked patch with improved semantics as suggested.
I used CONFIG_SECURITY_NETWORK_XFRM instead
of CONFIG_SECURITY since this can be considered part of the
labeled networking semantics. Let me know if my assumption 
sounds incorrect.

Also, I have built and tested with and without CONFIG_SECURITY_NETWORK_XFRM.

Regards,
Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>


diff -urpN linux-2.6.20.orig/include/net/xfrm.h linux-2.6.20.patch/include/net/xfrm.h
--- linux-2.6.20.orig/include/net/xfrm.h	2007-03-23 11:01:48.000000000 -0500
+++ linux-2.6.20.patch/include/net/xfrm.h	2007-03-25 21:36:05.000000000 -0500
@@ -937,7 +937,7 @@ static inline int xfrm_state_sort(struct
 #endif
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_notify(struct xfrm_state *x, int event);
@@ -991,13 +991,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.20.orig/net/key/af_key.c linux-2.6.20.patch/net/key/af_key.c
--- linux-2.6.20.orig/net/key/af_key.c	2007-03-23 11:02:49.000000000 -0500
+++ linux-2.6.20.patch/net/key/af_key.c	2007-03-25 21:34:35.000000000 -0500
@@ -1645,6 +1645,7 @@ static int pfkey_flush(struct sock *sk, 
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1652,7 +1653,9 @@ static int pfkey_flush(struct sock *sk, 
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2628,10 +2631,13 @@ static int pfkey_spdflush(struct sock *s
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20.patch/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c	2007-03-25 21:32:51.000000000 -0500
@@ -813,11 +813,65 @@ struct xfrm_policy *xfrm_policy_byid(u8 
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			if ((err = security_xfrm_policy_delete(pol)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD,
+					       err ? 0 : 1, pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				if ((err = security_xfrm_policy_delete(pol)) != 0) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       err ? 0 : 1, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -870,7 +924,9 @@ void xfrm_policy_flush(u8 type, struct x
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2485,4 +2541,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c	2007-03-25 21:33:02.000000000 -0500
@@ -388,12 +388,48 @@ int xfrm_state_delete(struct xfrm_state 
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               err ? 0 : 1, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -416,8 +452,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_user.c linux-2.6.20.patch/net/xfrm/xfrm_user.c
--- linux-2.6.20.orig/net/xfrm/xfrm_user.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_user.c	2007-03-25 21:33:52.000000000 -0500
@@ -1312,10 +1312,13 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1476,7 +1479,9 @@ static int xfrm_flush_policy(struct sk_b
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 20:56 -0400, James Morris wrote:
>On Thu, 22 Mar 2007, Joy Latten wrote:
>> > Perhaps a better semantic would be to fail the entire flush operation if 
>> > one of the security checks failed.  e.g. loop through for permissions 
>> > first, then if all ok, loop through for deletion.
>> > 
>> Ok, will code this up and test it if there are no objections.
>
>I'd suggest making the permission loop a noop if CONFIG_SECURITY=n, via a 
>static inline function.

Reworked patch with improved semantics as suggested.
I used CONFIG_SECURITY_NETWORK_XFRM instead
of CONFIG_SECURITY since this can be considered part of the
labeled networking semantics. Let me know if my assumption 
sounds incorrect.

Also, I have built and tested with and without CONFIG_SECURITY_NETWORK_XFRM.

Regards,
Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>


diff -urpN linux-2.6.20.orig/include/net/xfrm.h linux-2.6.20.patch/include/net/xfrm.h
--- linux-2.6.20.orig/include/net/xfrm.h	2007-03-23 11:01:48.000000000 -0500
+++ linux-2.6.20.patch/include/net/xfrm.h	2007-03-25 21:36:05.000000000 -0500
@@ -937,7 +937,7 @@ static inline int xfrm_state_sort(struct
 #endif
 extern struct xfrm_state *xfrm_find_acq_byseq(u32 seq);
 extern int xfrm_state_delete(struct xfrm_state *x);
-extern void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
+extern int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info);
 extern int xfrm_replay_check(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_advance(struct xfrm_state *x, __be32 seq);
 extern void xfrm_replay_notify(struct xfrm_state *x, int event);
@@ -991,13 +991,13 @@ struct xfrm_policy *xfrm_policy_bysel_ct
 					  struct xfrm_sec_ctx *ctx, int delete,
 					  int *err);
 struct xfrm_policy *xfrm_policy_byid(u8, int dir, u32 id, int delete, int *err);
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 u32 xfrm_get_acqseq(void);
 void xfrm_alloc_spi(struct xfrm_state *x, __be32 minspi, __be32 maxspi);
 struct xfrm_state * xfrm_find_acq(u8 mode, u32 reqid, u8 proto,
 				  xfrm_address_t *daddr, xfrm_address_t *saddr,
 				  int create, unsigned short family);
-extern void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
+extern int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info);
 extern int xfrm_sk_policy_insert(struct sock *sk, int dir, struct xfrm_policy *pol);
 extern int xfrm_bundle_ok(struct xfrm_policy *pol, struct xfrm_dst *xdst,
 			  struct flowi *fl, int family, int strict);
diff -urpN linux-2.6.20.orig/net/key/af_key.c linux-2.6.20.patch/net/key/af_key.c
--- linux-2.6.20.orig/net/key/af_key.c	2007-03-23 11:02:49.000000000 -0500
+++ linux-2.6.20.patch/net/key/af_key.c	2007-03-25 21:34:35.000000000 -0500
@@ -1645,6 +1645,7 @@ static int pfkey_flush(struct sock *sk, 
 	unsigned proto;
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	proto = pfkey_satype2proto(hdr->sadb_msg_satype);
 	if (proto == 0)
@@ -1652,7 +1653,9 @@ static int pfkey_flush(struct sock *sk, 
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_state_flush(proto, &audit_info);
+	err = xfrm_state_flush(proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = proto;
 	c.seq = hdr->sadb_msg_seq;
 	c.pid = hdr->sadb_msg_pid;
@@ -2628,10 +2631,13 @@ static int pfkey_spdflush(struct sock *s
 {
 	struct km_event c;
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = audit_get_loginuid(current->audit_context);
 	audit_info.secid = 0;
-	xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	err = xfrm_policy_flush(XFRM_POLICY_TYPE_MAIN, &audit_info);
+	if (err)
+		return err;
 	c.data.type = XFRM_POLICY_TYPE_MAIN;
 	c.event = XFRM_MSG_FLUSHPOLICY;
 	c.pid = hdr->sadb_msg_pid;
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20.patch/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c	2007-03-25 21:32:51.000000000 -0500
@@ -813,11 +813,65 @@ struct xfrm_policy *xfrm_policy_byid(u8 
 }
 EXPORT_SYMBOL(xfrm_policy_byid);
 
-void xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
 {
-	int dir;
+	int dir, err = 0;
+
+	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
+		struct xfrm_policy *pol;
+		struct hlist_node *entry;
+		int i;
+
+		hlist_for_each_entry(pol, entry,
+				     &xfrm_policy_inexact[dir], bydst) {
+			if (pol->type != type)
+				continue;
+			if ((err = security_xfrm_policy_delete(pol)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSPD,
+					       err ? 0 : 1, pol, NULL);
+				return err;
+			}
+                }
+		for (i = xfrm_policy_bydst[dir].hmask; i >= 0; i--) {
+			hlist_for_each_entry(pol, entry,
+					     xfrm_policy_bydst[dir].table + i,
+					     bydst) {
+				if (pol->type != type)
+					continue;
+				if ((err = security_xfrm_policy_delete(pol)) != 0) {
+					xfrm_audit_log(audit_info->loginuid,
+						       audit_info->secid,
+						       AUDIT_MAC_IPSEC_DELSPD,
+						       err ? 0 : 1, pol, NULL);
+					return err;
+				}
+			}
+		}
+	}
+	return err;
+}
+#else
+static inline int
+xfrm_policy_flush_secctx_check(u8 type, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_policy_flush(u8 type, struct xfrm_audit *audit_info)
+{
+	int dir, err = 0;
 
 	write_lock_bh(&xfrm_policy_lock);
+
+	err = xfrm_policy_flush_secctx_check(type, audit_info);
+	if (err)
+		goto out;
+
 	for (dir = 0; dir < XFRM_POLICY_MAX; dir++) {
 		struct xfrm_policy *pol;
 		struct hlist_node *entry;
@@ -870,7 +924,9 @@ void xfrm_policy_flush(u8 type, struct x
 		xfrm_policy_count[dir] -= killed;
 	}
 	atomic_inc(&flow_cache_genid);
+out:
 	write_unlock_bh(&xfrm_policy_lock);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_policy_flush);
 
@@ -2485,4 +2541,3 @@ restore_state:
 }
 EXPORT_SYMBOL(xfrm_migrate);
 #endif
-
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20.patch/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_state.c	2007-03-25 21:33:02.000000000 -0500
@@ -388,12 +388,48 @@ int xfrm_state_delete(struct xfrm_state 
 }
 EXPORT_SYMBOL(xfrm_state_delete);
 
-void xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+#ifdef CONFIG_SECURITY_NETWORK_XFRM
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info) 
 {
-	int i;
-	int err = 0;
+	int i, err = 0;
+
+	for (i = 0; i <= xfrm_state_hmask; i++) {
+		struct hlist_node *entry;
+		struct xfrm_state *x;
+
+		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
+			if (xfrm_id_proto_match(x->id.proto, proto) &&
+			   (err = security_xfrm_state_delete(x)) != 0) {
+				xfrm_audit_log(audit_info->loginuid,
+					       audit_info->secid,
+					       AUDIT_MAC_IPSEC_DELSA, 
+                                               err ? 0 : 1, NULL, x);
+
+				return err;
+			}
+		}
+	}
+
+	return err;
+}
+#else
+static inline int
+xfrm_state_flush_secctx_check(u8 proto, struct xfrm_audit *audit_info)
+{
+	return 0;
+}
+#endif
+
+int xfrm_state_flush(u8 proto, struct xfrm_audit *audit_info)
+{
+	int i, err = 0;
 
 	spin_lock_bh(&xfrm_state_lock);
+	err = xfrm_state_flush_secctx_check(proto, audit_info);
+	if (err)
+		goto out;
+		
 	for (i = 0; i <= xfrm_state_hmask; i++) {
 		struct hlist_node *entry;
 		struct xfrm_state *x;
@@ -416,8 +452,12 @@ restart:
 			}
 		}
 	}
+	err = 0;
+
+out:
 	spin_unlock_bh(&xfrm_state_lock);
 	wake_up(&km_waitq);
+	return err;
 }
 EXPORT_SYMBOL(xfrm_state_flush);
 
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_user.c linux-2.6.20.patch/net/xfrm/xfrm_user.c
--- linux-2.6.20.orig/net/xfrm/xfrm_user.c	2007-03-23 11:02:46.000000000 -0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_user.c	2007-03-25 21:33:52.000000000 -0500
@@ -1312,10 +1312,13 @@ static int xfrm_flush_sa(struct sk_buff 
 	struct km_event c;
 	struct xfrm_usersa_flush *p = NLMSG_DATA(nlh);
 	struct xfrm_audit audit_info;
+	int err;
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_state_flush(p->proto, &audit_info);
+	err = xfrm_state_flush(p->proto, &audit_info);
+	if (err)
+		return err;
 	c.data.proto = p->proto;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;
@@ -1476,7 +1479,9 @@ static int xfrm_flush_policy(struct sk_b
 
 	audit_info.loginuid = NETLINK_CB(skb).loginuid;
 	audit_info.secid = NETLINK_CB(skb).sid;
-	xfrm_policy_flush(type, &audit_info);
+	err = xfrm_policy_flush(type, &audit_info);
+	if (err)
+		return err;
 	c.data.type = type;
 	c.event = nlh->nlmsg_type;
 	c.seq = nlh->nlmsg_seq;

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Fri, 2007-03-23 at 11:47 -0700, David Miller wrote:
> From: James Morris <jmorris@namei.org>
> Date: Fri, 23 Mar 2007 14:46:48 -0400 (EDT)
> 
> > A 'flush' has a semantic implication that all entries will be removed, and 
> > it should be atomic and either succeed or fail at that granularity.
> 
> Correct.

Fair enough, does it matter that we have no way to report failure back
to users who can no longer assume success?

-Eric

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Fri, 2007-03-23 at 11:47 -0700, David Miller wrote:
> From: James Morris <jmorris@namei.org>
> Date: Fri, 23 Mar 2007 14:46:48 -0400 (EDT)
> 
> > A 'flush' has a semantic implication that all entries will be removed, and 
> > it should be atomic and either succeed or fail at that granularity.
> 
> Correct.

Fair enough, does it matter that we have no way to report failure back
to users who can no longer assume success?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Fri, 2007-03-23 at 12:59 -0400, Eric Paris wrote:
> On Fri, 2007-03-23 at 10:33 -0600, Joy Latten wrote:
> > On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:
> > 
> > > 
> > > In either case though proper auditing needs to be addressed.  I see that
> > > the first patch from Joy wouldn't audit deletion failures.  It appears
> > > to me if the check is done per policy then the security hook return code
> > > needs to be recorded and passed to xfrm_audit_log instead of the hard
> > > coded 1 result used now.
> > > 
> > > Assuming we go with James's double loop what should we be auditing for a
> > > security hook denial?  Just audit the first policy entry which we tried
> > > to remove but couldn't and then leave the rest of the auditing in those
> > > functions the way it is now in case there was no denial, calling
> > > xfrm_audit_log with a hard coded 1 for the result?
> > > 
> > Actually, I thought the original intent of the ipsec auditing was to
> > just audit changes made to the SAD/SPD databases, not securiy hook
> > denials, right? 
> 
> Then what is the point of the 'result' field that we capture and log in
> xfrm_audit_log if the only things you care to audit are successful
> changes to the databases?
> 
Yes, I think we do want to audit the security denial since it is the
reason we could not change the policy. In the flush case it seem it will
be the only reason. As you suggested, I will audit the first denial
since this is the reason the flush will fail.

But sometimes, in other cases, the delete or add could fail for other
reasons too such as not being able to allocate memory, not finding the
entry, etc... which is passed in the result field. 


Regards,
Joy 

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Fri, 2007-03-23 at 12:59 -0400, Eric Paris wrote:
> On Fri, 2007-03-23 at 10:33 -0600, Joy Latten wrote:
> > On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:
> > 
> > > 
> > > In either case though proper auditing needs to be addressed.  I see that
> > > the first patch from Joy wouldn't audit deletion failures.  It appears
> > > to me if the check is done per policy then the security hook return code
> > > needs to be recorded and passed to xfrm_audit_log instead of the hard
> > > coded 1 result used now.
> > > 
> > > Assuming we go with James's double loop what should we be auditing for a
> > > security hook denial?  Just audit the first policy entry which we tried
> > > to remove but couldn't and then leave the rest of the auditing in those
> > > functions the way it is now in case there was no denial, calling
> > > xfrm_audit_log with a hard coded 1 for the result?
> > > 
> > Actually, I thought the original intent of the ipsec auditing was to
> > just audit changes made to the SAD/SPD databases, not securiy hook
> > denials, right? 
> 
> Then what is the point of the 'result' field that we capture and log in
> xfrm_audit_log if the only things you care to audit are successful
> changes to the databases?
> 
Yes, I think we do want to audit the security denial since it is the
reason we could not change the policy. In the flush case it seem it will
be the only reason. As you suggested, I will audit the first denial
since this is the reason the flush will fail.

But sometimes, in other cases, the delete or add could fail for other
reasons too such as not being able to allocate memory, not finding the
entry, etc... which is passed in the result field. 


Regards,
Joy 

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[David Miller]

From: James Morris <jmorris@namei.org>
Date: Fri, 23 Mar 2007 14:46:48 -0400 (EDT)

> A 'flush' has a semantic implication that all entries will be removed, and 
> it should be atomic and either succeed or fail at that granularity.

Correct.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Fri, 23 Mar 2007, Eric Paris wrote:

> Maybe I'm way out on a limb here but if I am a regular user and I say
> rm /tmp/* and I only have permissions to delete some of the files I
> expect just those couple to be delete, not the whole operation denied.

I don't think this analogy holds up, as rm is a per-file deletion 
operation, and it is the shell which expands the wildcard for you.

A 'flush' has a semantic implication that all entries will be removed, and 
it should be atomic and either succeed or fail at that granularity.



- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Fri, 23 Mar 2007, Eric Paris wrote:

> Maybe I'm way out on a limb here but if I am a regular user and I say
> rm /tmp/* and I only have permissions to delete some of the files I
> expect just those couple to be delete, not the whole operation denied.

I don't think this analogy holds up, as rm is a per-file deletion 
operation, and it is the shell which expands the wildcard for you.

A 'flush' has a semantic implication that all entries will be removed, and 
it should be atomic and either succeed or fail at that granularity.



- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Fri, 2007-03-23 at 10:33 -0600, Joy Latten wrote:
> On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:
> 
> > 
> > In either case though proper auditing needs to be addressed.  I see that
> > the first patch from Joy wouldn't audit deletion failures.  It appears
> > to me if the check is done per policy then the security hook return code
> > needs to be recorded and passed to xfrm_audit_log instead of the hard
> > coded 1 result used now.
> > 
> > Assuming we go with James's double loop what should we be auditing for a
> > security hook denial?  Just audit the first policy entry which we tried
> > to remove but couldn't and then leave the rest of the auditing in those
> > functions the way it is now in case there was no denial, calling
> > xfrm_audit_log with a hard coded 1 for the result?
> > 
> Actually, I thought the original intent of the ipsec auditing was to
> just audit changes made to the SAD/SPD databases, not securiy hook
> denials, right? 

Then what is the point of the 'result' field that we capture and log in
xfrm_audit_log if the only things you care to audit are successful
changes to the databases?

-Eric

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Fri, 2007-03-23 at 10:33 -0600, Joy Latten wrote:
> On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:
> 
> > 
> > In either case though proper auditing needs to be addressed.  I see that
> > the first patch from Joy wouldn't audit deletion failures.  It appears
> > to me if the check is done per policy then the security hook return code
> > needs to be recorded and passed to xfrm_audit_log instead of the hard
> > coded 1 result used now.
> > 
> > Assuming we go with James's double loop what should we be auditing for a
> > security hook denial?  Just audit the first policy entry which we tried
> > to remove but couldn't and then leave the rest of the auditing in those
> > functions the way it is now in case there was no denial, calling
> > xfrm_audit_log with a hard coded 1 for the result?
> > 
> Actually, I thought the original intent of the ipsec auditing was to
> just audit changes made to the SAD/SPD databases, not securiy hook
> denials, right? 

Then what is the point of the 'result' field that we capture and log in
xfrm_audit_log if the only things you care to audit are successful
changes to the databases?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:

> 
> In either case though proper auditing needs to be addressed.  I see that
> the first patch from Joy wouldn't audit deletion failures.  It appears
> to me if the check is done per policy then the security hook return code
> needs to be recorded and passed to xfrm_audit_log instead of the hard
> coded 1 result used now.
> 
> Assuming we go with James's double loop what should we be auditing for a
> security hook denial?  Just audit the first policy entry which we tried
> to remove but couldn't and then leave the rest of the auditing in those
> functions the way it is now in case there was no denial, calling
> xfrm_audit_log with a hard coded 1 for the result?
> 
Actually, I thought the original intent of the ipsec auditing was to
just audit changes made to the SAD/SPD databases, not securiy hook
denials, right? 

Joy

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Fri, 2007-03-23 at 01:39 -0400, Eric Paris wrote:

> 
> In either case though proper auditing needs to be addressed.  I see that
> the first patch from Joy wouldn't audit deletion failures.  It appears
> to me if the check is done per policy then the security hook return code
> needs to be recorded and passed to xfrm_audit_log instead of the hard
> coded 1 result used now.
> 
> Assuming we go with James's double loop what should we be auditing for a
> security hook denial?  Just audit the first policy entry which we tried
> to remove but couldn't and then leave the rest of the auditing in those
> functions the way it is now in case there was no denial, calling
> xfrm_audit_log with a hard coded 1 for the result?
> 
Actually, I thought the original intent of the ipsec auditing was to
just audit changes made to the SAD/SPD databases, not securiy hook
denials, right? 

Joy

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[David Miller]

From: Eric Paris <eparis@parisplace.org>
Date: Fri, 23 Mar 2007 01:39:47 -0400

> It seems reasonable to me that the check for every policy (which is
> between current->security->sid and xp->security->ctx_sid) makes sense.
> There doesn't appear to me right offhand to be anything intrinsic in the
> code which says that a flush request must flush everything or nothing.

The "intrinsic" part comes from the heritage of routing daemons and
what they expect from flush operations, these assumption live in the
ipsec daemons as well.

A daemon, routing or ipsec, is flushing the tables in order to create
an entirely "clean slate", and they very much expect the policy and
state tables to be %100 empty after the operation.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Thu, 2007-03-22 at 19:49 -0400, James Morris wrote:
> On Thu, 22 Mar 2007, Joy Latten wrote:
> 
> > > I would look at this patch differently if there were some
> > > security level key being checked for a match here, which is
> > > an input key to the flush, but that is not what is happening
> > > here as the object is being looked at by itself.
> > 
> > Yes, I understand what you are saying.
> > I was concerned about having to check each entry
> > to flush database.
> > 
> > I did this patch because we check for authorization
> > when deleting single specified entries from the SAD/SPD. It
> > seem like a hole to me that we check for this, but that same
> > user/process can delete the entire database with no checks.
> 
> Indeed.  Removing an entry is modifying MAC policy, which requires 
> appropriate authorization.
> 
> The security label is encapsulated with the object, which is why it's 
> passed to the security layer.
> 
> Perhaps a better semantic would be to fail the entire flush operation if 
> one of the security checks failed.  e.g. loop through for permissions 
> first, then if all ok, loop through for deletion.

Maybe I'm way out on a limb here but if I am a regular user and I say
rm /tmp/* and I only have permissions to delete some of the files I
expect just those couple to be delete, not the whole operation denied.

It seems reasonable to me that the check for every policy (which is
between current->security->sid and xp->security->ctx_sid) makes sense.
There doesn't appear to me right offhand to be anything intrinsic in the
code which says that a flush request must flush everything or nothing.

In either case though proper auditing needs to be addressed.  I see that
the first patch from Joy wouldn't audit deletion failures.  It appears
to me if the check is done per policy then the security hook return code
needs to be recorded and passed to xfrm_audit_log instead of the hard
coded 1 result used now.

Assuming we go with James's double loop what should we be auditing for a
security hook denial?  Just audit the first policy entry which we tried
to remove but couldn't and then leave the rest of the auditing in those
functions the way it is now in case there was no denial, calling
xfrm_audit_log with a hard coded 1 for the result?

-Eric

Re: [PATCH]: Add security check before flushing SAD/SPD

[Eric Paris]

On Thu, 2007-03-22 at 19:49 -0400, James Morris wrote:
> On Thu, 22 Mar 2007, Joy Latten wrote:
> 
> > > I would look at this patch differently if there were some
> > > security level key being checked for a match here, which is
> > > an input key to the flush, but that is not what is happening
> > > here as the object is being looked at by itself.
> > 
> > Yes, I understand what you are saying.
> > I was concerned about having to check each entry
> > to flush database.
> > 
> > I did this patch because we check for authorization
> > when deleting single specified entries from the SAD/SPD. It
> > seem like a hole to me that we check for this, but that same
> > user/process can delete the entire database with no checks.
> 
> Indeed.  Removing an entry is modifying MAC policy, which requires 
> appropriate authorization.
> 
> The security label is encapsulated with the object, which is why it's 
> passed to the security layer.
> 
> Perhaps a better semantic would be to fail the entire flush operation if 
> one of the security checks failed.  e.g. loop through for permissions 
> first, then if all ok, loop through for deletion.

Maybe I'm way out on a limb here but if I am a regular user and I say
rm /tmp/* and I only have permissions to delete some of the files I
expect just those couple to be delete, not the whole operation denied.

It seems reasonable to me that the check for every policy (which is
between current->security->sid and xp->security->ctx_sid) makes sense.
There doesn't appear to me right offhand to be anything intrinsic in the
code which says that a flush request must flush everything or nothing.

In either case though proper auditing needs to be addressed.  I see that
the first patch from Joy wouldn't audit deletion failures.  It appears
to me if the check is done per policy then the security hook return code
needs to be recorded and passed to xfrm_audit_log instead of the hard
coded 1 result used now.

Assuming we go with James's double loop what should we be auditing for a
security hook denial?  Just audit the first policy entry which we tried
to remove but couldn't and then leave the rest of the auditing in those
functions the way it is now in case there was no denial, calling
xfrm_audit_log with a hard coded 1 for the result?

-Eric


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Thu, 22 Mar 2007, Joy Latten wrote:

> > Perhaps a better semantic would be to fail the entire flush operation if 
> > one of the security checks failed.  e.g. loop through for permissions 
> > first, then if all ok, loop through for deletion.
> > 
> Ok, will code this up and test it if there are no objections.

I'd suggest making the permission loop a noop if CONFIG_SECURITY=n, via a 
static inline function.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Thu, 22 Mar 2007, Joy Latten wrote:

> > Perhaps a better semantic would be to fail the entire flush operation if 
> > one of the security checks failed.  e.g. loop through for permissions 
> > first, then if all ok, loop through for deletion.
> > 
> Ok, will code this up and test it if there are no objections.

I'd suggest making the permission loop a noop if CONFIG_SECURITY=n, via a 
static inline function.


-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 19:49 -0400, James Morris wrote:
> On Thu, 22 Mar 2007, Joy Latten wrote:
> 
> > > I would look at this patch differently if there were some
> > > security level key being checked for a match here, which is
> > > an input key to the flush, but that is not what is happening
> > > here as the object is being looked at by itself.
> > 
> > Yes, I understand what you are saying.
> > I was concerned about having to check each entry
> > to flush database.
> > 
> > I did this patch because we check for authorization
> > when deleting single specified entries from the SAD/SPD. It
> > seem like a hole to me that we check for this, but that same
> > user/process can delete the entire database with no checks.
> 
> Indeed.  Removing an entry is modifying MAC policy, which requires 
> appropriate authorization.
> 
> The security label is encapsulated with the object, which is why it's 
> passed to the security layer.
> 
> Perhaps a better semantic would be to fail the entire flush operation if 
> one of the security checks failed.  e.g. loop through for permissions 
> first, then if all ok, loop through for deletion.
> 
Ok, will code this up and test it if there are no objections.

Joy

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 19:49 -0400, James Morris wrote:
> On Thu, 22 Mar 2007, Joy Latten wrote:
> 
> > > I would look at this patch differently if there were some
> > > security level key being checked for a match here, which is
> > > an input key to the flush, but that is not what is happening
> > > here as the object is being looked at by itself.
> > 
> > Yes, I understand what you are saying.
> > I was concerned about having to check each entry
> > to flush database.
> > 
> > I did this patch because we check for authorization
> > when deleting single specified entries from the SAD/SPD. It
> > seem like a hole to me that we check for this, but that same
> > user/process can delete the entire database with no checks.
> 
> Indeed.  Removing an entry is modifying MAC policy, which requires 
> appropriate authorization.
> 
> The security label is encapsulated with the object, which is why it's 
> passed to the security layer.
> 
> Perhaps a better semantic would be to fail the entire flush operation if 
> one of the security checks failed.  e.g. loop through for permissions 
> first, then if all ok, loop through for deletion.
> 
Ok, will code this up and test it if there are no objections.

Joy

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Thu, 22 Mar 2007, Joy Latten wrote:

> > I would look at this patch differently if there were some
> > security level key being checked for a match here, which is
> > an input key to the flush, but that is not what is happening
> > here as the object is being looked at by itself.
> 
> Yes, I understand what you are saying.
> I was concerned about having to check each entry
> to flush database.
> 
> I did this patch because we check for authorization
> when deleting single specified entries from the SAD/SPD. It
> seem like a hole to me that we check for this, but that same
> user/process can delete the entire database with no checks.

Indeed.  Removing an entry is modifying MAC policy, which requires 
appropriate authorization.

The security label is encapsulated with the object, which is why it's 
passed to the security layer.

Perhaps a better semantic would be to fail the entire flush operation if 
one of the security checks failed.  e.g. loop through for permissions 
first, then if all ok, loop through for deletion.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH]: Add security check before flushing SAD/SPD

[James Morris]

On Thu, 22 Mar 2007, Joy Latten wrote:

> > I would look at this patch differently if there were some
> > security level key being checked for a match here, which is
> > an input key to the flush, but that is not what is happening
> > here as the object is being looked at by itself.
> 
> Yes, I understand what you are saying.
> I was concerned about having to check each entry
> to flush database.
> 
> I did this patch because we check for authorization
> when deleting single specified entries from the SAD/SPD. It
> seem like a hole to me that we check for this, but that same
> user/process can delete the entire database with no checks.

Indeed.  Removing an entry is modifying MAC policy, which requires 
appropriate authorization.

The security label is encapsulated with the object, which is why it's 
passed to the security layer.

Perhaps a better semantic would be to fail the entire flush operation if 
one of the security checks failed.  e.g. loop through for permissions 
first, then if all ok, loop through for deletion.


- James
-- 
James Morris
<jmorris@namei.org>

--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 12:01 -0700, David Miller wrote:
> From: Joy Latten <latten@austin.ibm.com>
> Date: Thu, 22 Mar 2007 12:35:39 -0600
> 
> > Within selinux we check for authorization before deleting entries from
> > SAD and SPD. 
> > 
> > We are not checking for authorization when flushing the SPD and
> > the SAD. It was perhaps missed in original patch.
> > 
> > This patch adds security check when flushing entries from SAD and SPD.
> > 
> > Please let me know if this patch is ok.
> > It was built against linux-2.6.21-rc4-git5. I have also tested it.
> > 
> > Signed-off-by: Joy Latten<latten@austin.ibm.com>
> 
> I don't understand this and it does not sit well with me.
> 
> If we are flushing the policy database, we are flushing it
> regardless of what the security layer might or might not say.
> 
> I would look at this patch differently if there were some
> security level key being checked for a match here, which is
> an input key to the flush, but that is not what is happening
> here as the object is being looked at by itself.

Yes, I understand what you are saying.
I was concerned about having to check each entry
to flush database.

I did this patch because we check for authorization
when deleting single specified entries from the SAD/SPD. It
seem like a hole to me that we check for this, but that same
user/process can delete the entire database with no checks.

Unfortunately, each policy entry or SA can have a different security
label. And that is why I would have to check each entry's
security label before deleting. To see if the user/process has
authorization to delete an entry with that security label.

Including selinux list for suggestions.

Joy

Re: [PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

On Thu, 2007-03-22 at 12:01 -0700, David Miller wrote:
> From: Joy Latten <latten@austin.ibm.com>
> Date: Thu, 22 Mar 2007 12:35:39 -0600
> 
> > Within selinux we check for authorization before deleting entries from
> > SAD and SPD. 
> > 
> > We are not checking for authorization when flushing the SPD and
> > the SAD. It was perhaps missed in original patch.
> > 
> > This patch adds security check when flushing entries from SAD and SPD.
> > 
> > Please let me know if this patch is ok.
> > It was built against linux-2.6.21-rc4-git5. I have also tested it.
> > 
> > Signed-off-by: Joy Latten<latten@austin.ibm.com>
> 
> I don't understand this and it does not sit well with me.
> 
> If we are flushing the policy database, we are flushing it
> regardless of what the security layer might or might not say.
> 
> I would look at this patch differently if there were some
> security level key being checked for a match here, which is
> an input key to the flush, but that is not what is happening
> here as the object is being looked at by itself.

Yes, I understand what you are saying.
I was concerned about having to check each entry
to flush database.

I did this patch because we check for authorization
when deleting single specified entries from the SAD/SPD. It
seem like a hole to me that we check for this, but that same
user/process can delete the entire database with no checks.

Unfortunately, each policy entry or SA can have a different security
label. And that is why I would have to check each entry's
security label before deleting. To see if the user/process has
authorization to delete an entry with that security label.

Including selinux list for suggestions.

Joy


--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@tycho.nsa.gov with
the words "unsubscribe selinux" without quotes as the message.

Re: [PATCH]: Add security check before flushing SAD/SPD

[David Miller]

From: Joy Latten <latten@austin.ibm.com>
Date: Thu, 22 Mar 2007 12:35:39 -0600

> Within selinux we check for authorization before deleting entries from
> SAD and SPD. 
> 
> We are not checking for authorization when flushing the SPD and
> the SAD. It was perhaps missed in original patch.
> 
> This patch adds security check when flushing entries from SAD and SPD.
> 
> Please let me know if this patch is ok.
> It was built against linux-2.6.21-rc4-git5. I have also tested it.
> 
> Signed-off-by: Joy Latten<latten@austin.ibm.com>

I don't understand this and it does not sit well with me.

If we are flushing the policy database, we are flushing it
regardless of what the security layer might or might not say.

I would look at this patch differently if there were some
security level key being checked for a match here, which is
an input key to the flush, but that is not what is happening
here as the object is being looked at by itself.

[PATCH]: Add security check before flushing SAD/SPD

[Joy Latten]

Within selinux we check for authorization before deleting entries from
SAD and SPD. 

We are not checking for authorization when flushing the SPD and
the SAD. It was perhaps missed in original patch.

This patch adds security check when flushing entries from SAD and SPD.

Please let me know if this patch is ok.
It was built against linux-2.6.21-rc4-git5. I have also tested it.

Joy

Signed-off-by: Joy Latten<latten@austin.ibm.com>


diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_policy.c linux-2.6.20/net/xfrm/xfrm_policy.c
--- linux-2.6.20.orig/net/xfrm/xfrm_policy.c	2007-03-21 14:25:51.000000000 -0500
+++ linux-2.6.20/net/xfrm/xfrm_policy.c	2007-03-21 14:30:59.000000000 -0500
@@ -829,6 +829,8 @@ void xfrm_policy_flush(u8 type, struct x
 				     &xfrm_policy_inexact[dir], bydst) {
 			if (pol->type != type)
 				continue;
+			if (security_xfrm_policy_delete(pol))
+				continue;
 			hlist_del(&pol->bydst);
 			hlist_del(&pol->byidx);
 			write_unlock_bh(&xfrm_policy_lock);
@@ -850,6 +852,8 @@ void xfrm_policy_flush(u8 type, struct x
 					     bydst) {
 				if (pol->type != type)
 					continue;
+				if (security_xfrm_policy_delete(pol))
+					continue;
 				hlist_del(&pol->bydst);
 				hlist_del(&pol->byidx);
 				write_unlock_bh(&xfrm_policy_lock);
diff -urpN linux-2.6.20.orig/net/xfrm/xfrm_state.c linux-2.6.20/net/xfrm/xfrm_state.c
--- linux-2.6.20.orig/net/xfrm/xfrm_state.c	2007-03-21 14:25:51.000000000 -0500
+++ linux-2.6.20/net/xfrm/xfrm_state.c	2007-03-21 14:27:48.000000000 -0500
@@ -400,7 +400,8 @@ void xfrm_state_flush(u8 proto, struct x
 restart:
 		hlist_for_each_entry(x, entry, xfrm_state_bydst+i, bydst) {
 			if (!xfrm_state_kern(x) &&
-			    xfrm_id_proto_match(x->id.proto, proto)) {
+			    xfrm_id_proto_match(x->id.proto, proto) &&
+			    !security_xfrm_state_delete(x)) {
 				xfrm_state_hold(x);
 				spin_unlock_bh(&xfrm_state_lock);