Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED] I have also tested with 2.6.22-rc3-git7 and all appears to be working as expected. Acked-by: Joy Latten [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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[EMAIL PROTECTED] NOT NOW
WAS Acked-by: James Morris [EMAIL PROTECTED] NOT NOW
WAS Acked-by: Eric Paris [EMAIL PROTECTED] 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
Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED] -- James Morris [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
From: James Morris [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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[EMAIL PROTECTED]
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.h2007-03-23 11:01:48.0
-0500
+++ linux-2.6.20.patch/include/net/xfrm.h 2007-03-25 21:36:05.0
-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.0 -0500
+++ linux-2.6.20.patch/net/key/af_key.c 2007-03-25 21:34:35.0 -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.c2007-03-23 11:02:46.0
-0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c 2007-03-25 21:32:51.0
-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,
+
Re: [PATCH]: Add security check before flushing SAD/SPD
On Mon, 26 Mar 2007, Joy Latten wrote:
Signed-off-by: Joy Latten[EMAIL PROTECTED]
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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
On Mon, 26 Mar 2007, James Morris wrote:
On Mon, 26 Mar 2007, Joy Latten wrote:
Signed-off-by: Joy Latten[EMAIL PROTECTED]
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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
I have made improvements based on James' and Eric's comments.
Regards,
Joy
Signed-off-by: Joy Latten[EMAIL PROTECTED]
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.h2007-03-23 11:01:48.0
-0500
+++ linux-2.6.20.patch/include/net/xfrm.h 2007-03-25 21:36:05.0
-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.0 -0500
+++ linux-2.6.20.patch/net/key/af_key.c 2007-03-25 21:34:35.0 -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.c2007-03-23 11:02:46.0
-0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c 2007-03-26 17:19:26.0
-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,
Re: [PATCH]: Add security check before flushing SAD/SPD
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[EMAIL PROTECTED]
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.h2007-03-23 11:01:48.0
-0500
+++ linux-2.6.20.patch/include/net/xfrm.h 2007-03-25 21:36:05.0
-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.0 -0500
+++ linux-2.6.20.patch/net/key/af_key.c 2007-03-25 21:34:35.0 -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.c2007-03-23 11:02:46.0
-0500
+++ linux-2.6.20.patch/net/xfrm/xfrm_policy.c 2007-03-26 17:19:26.0
-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 =
Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED]
+ 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
[EMAIL PROTECTED]
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
From: James Morris [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
On Fri, 2007-03-23 at 11:47 -0700, David Miller wrote: From: James Morris [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH]: Add security check before flushing SAD/SPD
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[EMAIL PROTECTED]
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.c2007-03-21 14:25:51.0
-0500
+++ linux-2.6.20/net/xfrm/xfrm_policy.c 2007-03-21 14:30:59.0 -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.0
-0500
+++ linux-2.6.20/net/xfrm/xfrm_state.c 2007-03-21 14:27:48.0 -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);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
From: Joy Latten [EMAIL PROTECTED] 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[EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
On Thu, 2007-03-22 at 12:01 -0700, David Miller wrote: From: Joy Latten [EMAIL PROTECTED] 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[EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH]: Add security check before flushing SAD/SPD
From: Eric Paris [EMAIL PROTECTED] 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. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
