* [PATCH 2/6] secmark: make secmark object handling generic
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
@ 2010-09-24 20:45 ` Eric Paris
2010-09-25 8:39 ` Pablo Neira Ayuso
2010-09-24 20:45 ` [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid Eric Paris
` (4 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Eric Paris @ 2010-09-24 20:45 UTC (permalink / raw)
To: linux-kernel, selinux, netfilter-devel
Cc: jmorris, sds, jengelh, paul.moore, casey, linux-security-module,
netfilter, mr.dash.four
Right now secmark has lots of direct selinux calls. Use all LSM calls and
remove all SELinux specific knowledge. The only SELinux specific knowledge
we leave is the mode. The only point is to make sure that other LSMs at
least test this generic code before they assume it works. (They may also
have to make changes if they do not represent labels as strings)
Signed-off-by: Eric Paris <eparis@redhat.com>
---
include/linux/netfilter/xt_SECMARK.h | 14 ++------
include/linux/security.h | 25 +++++++++++++
include/linux/selinux.h | 63 ----------------------------------
net/netfilter/xt_CT.c | 1 -
net/netfilter/xt_SECMARK.c | 57 +++++++++++++++----------------
security/capability.c | 17 +++++++++
security/security.c | 18 ++++++++++
security/selinux/exports.c | 49 --------------------------
security/selinux/hooks.c | 24 +++++++++++++
security/selinux/include/security.h | 1 +
10 files changed, 116 insertions(+), 153 deletions(-)
diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
index 6fcd344..b8d55c4 100644
--- a/include/linux/netfilter/xt_SECMARK.h
+++ b/include/linux/netfilter/xt_SECMARK.h
@@ -10,19 +10,13 @@
* 'mode' refers to the specific security subsystem which the
* packets are being marked for.
*/
-#define SECMARK_MODE_SEL 0x01 /* SELinux */
-#define SECMARK_SELCTX_MAX 256
-
-struct xt_secmark_target_selinux_info {
- __u32 selsid;
- char selctx[SECMARK_SELCTX_MAX];
-};
+#define SECMARK_MODE_SELINUX 0x01 /* SELinux */
+#define SECMARK_SECCTX_MAX 256
struct xt_secmark_target_info {
__u8 mode;
- union {
- struct xt_secmark_target_selinux_info sel;
- } u;
+ __u32 secid;
+ char secctx[SECMARK_SECCTX_MAX];
};
#endif /*_XT_SECMARK_H_target */
diff --git a/include/linux/security.h b/include/linux/security.h
index c2f0cdb..2f2e181 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -960,6 +960,12 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
* Sets the new child socket's sid to the openreq sid.
* @inet_conn_established:
* Sets the connection's peersid to the secmark on skb.
+ * @secmark_relabel_packet:
+ * check if the process should be allowed to relabel packets to the given secid
+ * @security_secmark_refcount_inc
+ * tells the LSM to increment the number of secmark labeling rules loaded
+ * @security_secmark_refcount_dec
+ * tells the LSM to decrement the number of secmark labeling rules loaded
* @req_classify_flow:
* Sets the flow's sid to the openreq sid.
* @tun_dev_create:
@@ -1597,6 +1603,9 @@ struct security_operations {
struct request_sock *req);
void (*inet_csk_clone) (struct sock *newsk, const struct request_sock *req);
void (*inet_conn_established) (struct sock *sk, struct sk_buff *skb);
+ int (*secmark_relabel_packet) (u32 secid);
+ void (*secmark_refcount_inc) (void);
+ void (*secmark_refcount_dec) (void);
void (*req_classify_flow) (const struct request_sock *req, struct flowi *fl);
int (*tun_dev_create)(void);
void (*tun_dev_post_create)(struct sock *sk);
@@ -2560,6 +2569,9 @@ void security_inet_csk_clone(struct sock *newsk,
const struct request_sock *req);
void security_inet_conn_established(struct sock *sk,
struct sk_buff *skb);
+int security_secmark_relabel_packet(u32 secid);
+void security_secmark_refcount_inc(void);
+void security_secmark_refcount_dec(void);
int security_tun_dev_create(void);
void security_tun_dev_post_create(struct sock *sk);
int security_tun_dev_attach(struct sock *sk);
@@ -2714,6 +2726,19 @@ static inline void security_inet_conn_established(struct sock *sk,
{
}
+static inline int security_secmark_relabel_packet(u32 secid)
+{
+ return 0;
+}
+
+static inline void security_secmark_refcount_inc(void)
+{
+}
+
+static inline void security_secmark_refcount_dec(void)
+{
+}
+
static inline int security_tun_dev_create(void)
{
return 0;
diff --git a/include/linux/selinux.h b/include/linux/selinux.h
index 82e0f26..44f4596 100644
--- a/include/linux/selinux.h
+++ b/include/linux/selinux.h
@@ -21,74 +21,11 @@ struct kern_ipc_perm;
#ifdef CONFIG_SECURITY_SELINUX
/**
- * selinux_string_to_sid - map a security context string to a security ID
- * @str: the security context string to be mapped
- * @sid: ID value returned via this.
- *
- * Returns 0 if successful, with the SID stored in sid. A value
- * of zero for sid indicates no SID could be determined (but no error
- * occurred).
- */
-int selinux_string_to_sid(char *str, u32 *sid);
-
-/**
- * selinux_secmark_relabel_packet_permission - secmark permission check
- * @sid: SECMARK ID value to be applied to network packet
- *
- * Returns 0 if the current task is allowed to set the SECMARK label of
- * packets with the supplied security ID. Note that it is implicit that
- * the packet is always being relabeled from the default unlabeled value,
- * and that the access control decision is made in the AVC.
- */
-int selinux_secmark_relabel_packet_permission(u32 sid);
-
-/**
- * selinux_secmark_refcount_inc - increments the secmark use counter
- *
- * SELinux keeps track of the current SECMARK targets in use so it knows
- * when to apply SECMARK label access checks to network packets. This
- * function incements this reference count to indicate that a new SECMARK
- * target has been configured.
- */
-void selinux_secmark_refcount_inc(void);
-
-/**
- * selinux_secmark_refcount_dec - decrements the secmark use counter
- *
- * SELinux keeps track of the current SECMARK targets in use so it knows
- * when to apply SECMARK label access checks to network packets. This
- * function decements this reference count to indicate that one of the
- * existing SECMARK targets has been removed/flushed.
- */
-void selinux_secmark_refcount_dec(void);
-
-/**
* selinux_is_enabled - is SELinux enabled?
*/
bool selinux_is_enabled(void);
#else
-static inline int selinux_string_to_sid(const char *str, u32 *sid)
-{
- *sid = 0;
- return 0;
-}
-
-static inline int selinux_secmark_relabel_packet_permission(u32 sid)
-{
- return 0;
-}
-
-static inline void selinux_secmark_refcount_inc(void)
-{
- return;
-}
-
-static inline void selinux_secmark_refcount_dec(void)
-{
- return;
-}
-
static inline bool selinux_is_enabled(void)
{
return false;
diff --git a/net/netfilter/xt_CT.c b/net/netfilter/xt_CT.c
index 0cb6053..782e519 100644
--- a/net/netfilter/xt_CT.c
+++ b/net/netfilter/xt_CT.c
@@ -9,7 +9,6 @@
#include <linux/module.h>
#include <linux/gfp.h>
#include <linux/skbuff.h>
-#include <linux/selinux.h>
#include <linux/netfilter_ipv4/ip_tables.h>
#include <linux/netfilter_ipv6/ip6_tables.h>
#include <linux/netfilter/x_tables.h>
diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
index 364ad16..fa775b7 100644
--- a/net/netfilter/xt_SECMARK.c
+++ b/net/netfilter/xt_SECMARK.c
@@ -14,8 +14,8 @@
*/
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <linux/module.h>
+#include <linux/security.h>
#include <linux/skbuff.h>
-#include <linux/selinux.h>
#include <linux/netfilter/x_tables.h>
#include <linux/netfilter/xt_SECMARK.h>
@@ -38,10 +38,9 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
BUG_ON(info->mode != mode);
switch (mode) {
- case SECMARK_MODE_SEL:
- secmark = info->u.sel.selsid;
+ case SECMARK_MODE_SELINUX:
+ secmark = info->secid;
break;
-
default:
BUG();
}
@@ -50,33 +49,33 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
return XT_CONTINUE;
}
-static int checkentry_selinux(struct xt_secmark_target_info *info)
+static int checkentry_lsm(struct xt_secmark_target_info *info)
{
int err;
- struct xt_secmark_target_selinux_info *sel = &info->u.sel;
- sel->selctx[SECMARK_SELCTX_MAX - 1] = '\0';
+ info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
+ info->secid = 0;
- err = selinux_string_to_sid(sel->selctx, &sel->selsid);
+ err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
+ &info->secid);
if (err) {
if (err == -EINVAL)
- pr_info("invalid SELinux context \'%s\'\n",
- sel->selctx);
+ pr_info("invalid security context \'%s\'\n", info->secctx);
return err;
}
- if (!sel->selsid) {
- pr_info("unable to map SELinux context \'%s\'\n", sel->selctx);
+ if (!info->secid) {
+ pr_info("unable to map security context \'%s\'\n", info->secctx);
return -ENOENT;
}
- err = selinux_secmark_relabel_packet_permission(sel->selsid);
+ err = security_secmark_relabel_packet(info->secid);
if (err) {
pr_info("unable to obtain relabeling permission\n");
return err;
}
- selinux_secmark_refcount_inc();
+ security_secmark_refcount_inc();
return 0;
}
@@ -99,17 +98,17 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
}
switch (info->mode) {
- case SECMARK_MODE_SEL:
- err = checkentry_selinux(info);
- if (err)
- return err;
+ case SECMARK_MODE_SELINUX:
break;
-
default:
pr_info("invalid mode: %hu\n", info->mode);
return -EINVAL;
}
+ err = checkentry_lsm(info);
+ if (err)
+ return err;
+
if (!mode)
mode = info->mode;
return 0;
@@ -118,20 +117,20 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
{
switch (mode) {
- case SECMARK_MODE_SEL:
- selinux_secmark_refcount_dec();
+ case SECMARK_MODE_SELINUX:
+ security_secmark_refcount_dec();
}
}
static struct xt_target secmark_tg_reg __read_mostly = {
- .name = "SECMARK",
- .revision = 0,
- .family = NFPROTO_UNSPEC,
- .checkentry = secmark_tg_check,
- .destroy = secmark_tg_destroy,
- .target = secmark_tg,
- .targetsize = sizeof(struct xt_secmark_target_info),
- .me = THIS_MODULE,
+ .name = "SECMARK",
+ .revision = 0,
+ .family = NFPROTO_UNSPEC,
+ .checkentry = secmark_tg_check,
+ .destroy = secmark_tg_destroy,
+ .target = secmark_tg,
+ .targetsize = sizeof(struct xt_secmark_target_info),
+ .me = THIS_MODULE,
};
static int __init secmark_tg_init(void)
diff --git a/security/capability.c b/security/capability.c
index 1e26299..806061b 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -681,7 +681,18 @@ static void cap_inet_conn_established(struct sock *sk, struct sk_buff *skb)
{
}
+static int cap_secmark_relabel_packet(u32 secid)
+{
+ return 0;
+}
+static void cap_secmark_refcount_inc(void)
+{
+}
+
+static void cap_secmark_refcount_dec(void)
+{
+}
static void cap_req_classify_flow(const struct request_sock *req,
struct flowi *fl)
@@ -781,7 +792,8 @@ static int cap_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
static int cap_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
{
- return -EOPNOTSUPP;
+ *secid = 0;
+ return 0;
}
static void cap_release_secctx(char *secdata, u32 seclen)
@@ -1023,6 +1035,9 @@ void __init security_fixup_ops(struct security_operations *ops)
set_to_cap_if_null(ops, inet_conn_request);
set_to_cap_if_null(ops, inet_csk_clone);
set_to_cap_if_null(ops, inet_conn_established);
+ set_to_cap_if_null(ops, secmark_relabel_packet);
+ set_to_cap_if_null(ops, secmark_refcount_inc);
+ set_to_cap_if_null(ops, secmark_refcount_dec);
set_to_cap_if_null(ops, req_classify_flow);
set_to_cap_if_null(ops, tun_dev_create);
set_to_cap_if_null(ops, tun_dev_post_create);
diff --git a/security/security.c b/security/security.c
index 1d77d8b..1975b2e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1144,6 +1144,24 @@ void security_inet_conn_established(struct sock *sk,
security_ops->inet_conn_established(sk, skb);
}
+int security_secmark_relabel_packet(u32 secid)
+{
+ return security_ops->secmark_relabel_packet(secid);
+}
+EXPORT_SYMBOL(security_secmark_relabel_packet);
+
+void security_secmark_refcount_inc(void)
+{
+ security_ops->secmark_refcount_inc();
+}
+EXPORT_SYMBOL(security_secmark_refcount_inc);
+
+void security_secmark_refcount_dec(void)
+{
+ security_ops->secmark_refcount_dec();
+}
+EXPORT_SYMBOL(security_secmark_refcount_dec);
+
int security_tun_dev_create(void)
{
return security_ops->tun_dev_create();
diff --git a/security/selinux/exports.c b/security/selinux/exports.c
index c0a454a..9066438 100644
--- a/security/selinux/exports.c
+++ b/security/selinux/exports.c
@@ -11,58 +11,9 @@
* it under the terms of the GNU General Public License version 2,
* as published by the Free Software Foundation.
*/
-#include <linux/types.h>
-#include <linux/kernel.h>
#include <linux/module.h>
-#include <linux/selinux.h>
-#include <linux/fs.h>
-#include <linux/ipc.h>
-#include <asm/atomic.h>
#include "security.h"
-#include "objsec.h"
-
-/* SECMARK reference count */
-extern atomic_t selinux_secmark_refcount;
-
-int selinux_string_to_sid(char *str, u32 *sid)
-{
- if (selinux_enabled)
- return security_context_to_sid(str, strlen(str), sid);
- else {
- *sid = 0;
- return 0;
- }
-}
-EXPORT_SYMBOL_GPL(selinux_string_to_sid);
-
-int selinux_secmark_relabel_packet_permission(u32 sid)
-{
- if (selinux_enabled) {
- const struct task_security_struct *__tsec;
- u32 tsid;
-
- __tsec = current_security();
- tsid = __tsec->sid;
-
- return avc_has_perm(tsid, sid, SECCLASS_PACKET,
- PACKET__RELABELTO, NULL);
- }
- return 0;
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_relabel_packet_permission);
-
-void selinux_secmark_refcount_inc(void)
-{
- atomic_inc(&selinux_secmark_refcount);
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_refcount_inc);
-
-void selinux_secmark_refcount_dec(void)
-{
- atomic_dec(&selinux_secmark_refcount);
-}
-EXPORT_SYMBOL_GPL(selinux_secmark_refcount_dec);
bool selinux_is_enabled(void)
{
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index aaa2dd8..7cfcd19 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4290,6 +4290,27 @@ static void selinux_inet_conn_established(struct sock *sk, struct sk_buff *skb)
selinux_skb_peerlbl_sid(skb, family, &sksec->peer_sid);
}
+static int selinux_secmark_relabel_packet(u32 sid)
+{
+ const struct task_security_struct *__tsec;
+ u32 tsid;
+
+ __tsec = current_security();
+ tsid = __tsec->sid;
+
+ return avc_has_perm(tsid, sid, SECCLASS_PACKET, PACKET__RELABELTO, NULL);
+}
+
+static void selinux_secmark_refcount_inc(void)
+{
+ atomic_inc(&selinux_secmark_refcount);
+}
+
+static void selinux_secmark_refcount_dec(void)
+{
+ atomic_dec(&selinux_secmark_refcount);
+}
+
static void selinux_req_classify_flow(const struct request_sock *req,
struct flowi *fl)
{
@@ -5545,6 +5566,9 @@ static struct security_operations selinux_ops = {
.inet_conn_request = selinux_inet_conn_request,
.inet_csk_clone = selinux_inet_csk_clone,
.inet_conn_established = selinux_inet_conn_established,
+ .secmark_relabel_packet = selinux_secmark_relabel_packet,
+ .secmark_refcount_inc = selinux_secmark_refcount_inc,
+ .secmark_refcount_dec = selinux_secmark_refcount_dec,
.req_classify_flow = selinux_req_classify_flow,
.tun_dev_create = selinux_tun_dev_create,
.tun_dev_post_create = selinux_tun_dev_post_create,
diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index 9df4477..a71e224 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -9,6 +9,7 @@
#define _SELINUX_SECURITY_H_
#include <linux/magic.h>
+#include <linux/types.h>
#include "flask.h"
#define SECSID_NULL 0x00000000 /* unspecified SID */
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] secmark: make secmark object handling generic
2010-09-24 20:45 ` [PATCH 2/6] secmark: make secmark object handling generic Eric Paris
@ 2010-09-25 8:39 ` Pablo Neira Ayuso
2010-09-27 16:47 ` Eric Paris
0 siblings, 1 reply; 25+ messages in thread
From: Pablo Neira Ayuso @ 2010-09-25 8:39 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On 24/09/10 22:45, Eric Paris wrote:
> Right now secmark has lots of direct selinux calls. Use all LSM calls and
> remove all SELinux specific knowledge. The only SELinux specific knowledge
> we leave is the mode. The only point is to make sure that other LSMs at
> least test this generic code before they assume it works. (They may also
> have to make changes if they do not represent labels as strings)
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>
> include/linux/netfilter/xt_SECMARK.h | 14 ++------
> include/linux/security.h | 25 +++++++++++++
> include/linux/selinux.h | 63 ----------------------------------
> net/netfilter/xt_CT.c | 1 -
> net/netfilter/xt_SECMARK.c | 57 +++++++++++++++----------------
> security/capability.c | 17 +++++++++
> security/security.c | 18 ++++++++++
> security/selinux/exports.c | 49 --------------------------
> security/selinux/hooks.c | 24 +++++++++++++
> security/selinux/include/security.h | 1 +
> 10 files changed, 116 insertions(+), 153 deletions(-)
>
> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> index 6fcd344..b8d55c4 100644
> --- a/include/linux/netfilter/xt_SECMARK.h
> +++ b/include/linux/netfilter/xt_SECMARK.h
> @@ -10,19 +10,13 @@
> * 'mode' refers to the specific security subsystem which the
> * packets are being marked for.
> */
> -#define SECMARK_MODE_SEL 0x01 /* SELinux */
> -#define SECMARK_SELCTX_MAX 256
> -
> -struct xt_secmark_target_selinux_info {
> - __u32 selsid;
> - char selctx[SECMARK_SELCTX_MAX];
> -};
> +#define SECMARK_MODE_SELINUX 0x01 /* SELinux */
> +#define SECMARK_SECCTX_MAX 256
The SECMARK_MODE_SEL is exposed to user-space, even if we have a copy of
it in the internal iptables tree, I'm not sure if it's a good policy to
change it.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] secmark: make secmark object handling generic
2010-09-25 8:39 ` Pablo Neira Ayuso
@ 2010-09-27 16:47 ` Eric Paris
0 siblings, 0 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-27 16:47 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On Sat, 2010-09-25 at 10:39 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > Right now secmark has lots of direct selinux calls. Use all LSM calls and
> > remove all SELinux specific knowledge. The only SELinux specific knowledge
> > we leave is the mode. The only point is to make sure that other LSMs at
> > least test this generic code before they assume it works. (They may also
> > have to make changes if they do not represent labels as strings)
> >
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > ---
> >
> > include/linux/netfilter/xt_SECMARK.h | 14 ++------
> > include/linux/security.h | 25 +++++++++++++
> > include/linux/selinux.h | 63 ----------------------------------
> > net/netfilter/xt_CT.c | 1 -
> > net/netfilter/xt_SECMARK.c | 57 +++++++++++++++----------------
> > security/capability.c | 17 +++++++++
> > security/security.c | 18 ++++++++++
> > security/selinux/exports.c | 49 --------------------------
> > security/selinux/hooks.c | 24 +++++++++++++
> > security/selinux/include/security.h | 1 +
> > 10 files changed, 116 insertions(+), 153 deletions(-)
> >
> > diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> > index 6fcd344..b8d55c4 100644
> > --- a/include/linux/netfilter/xt_SECMARK.h
> > +++ b/include/linux/netfilter/xt_SECMARK.h
> > @@ -10,19 +10,13 @@
> > * 'mode' refers to the specific security subsystem which the
> > * packets are being marked for.
> > */
> > -#define SECMARK_MODE_SEL 0x01 /* SELinux */
> > -#define SECMARK_SELCTX_MAX 256
> > -
> > -struct xt_secmark_target_selinux_info {
> > - __u32 selsid;
> > - char selctx[SECMARK_SELCTX_MAX];
> > -};
> > +#define SECMARK_MODE_SELINUX 0x01 /* SELinux */
> > +#define SECMARK_SECCTX_MAX 256
>
> The SECMARK_MODE_SEL is exposed to user-space, even if we have a copy of
> it in the internal iptables tree, I'm not sure if it's a good policy to
> change it.
Ok. I just hate 'SEL' as an abbreviation of SELinux. I always think
it's select or something like that. I could do something hideous like
#define SECMARK_MODE_SEL 0x01
#define SECMARK_MODE_SELINUX SECMARK_MODE_SEL
But it seems like too much work and doesn't add anything really. I'll
revert that part of this change.
-Eric
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
2010-09-24 20:45 ` [PATCH 2/6] secmark: make secmark object handling generic Eric Paris
@ 2010-09-24 20:45 ` Eric Paris
2010-09-25 8:41 ` Pablo Neira Ayuso
2010-09-27 0:50 ` James Morris
2010-09-24 20:45 ` [PATCH 4/6] security: secid_to_secctx returns len when data is NULL Eric Paris
` (3 subsequent siblings)
5 siblings, 2 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-24 20:45 UTC (permalink / raw)
To: linux-kernel, selinux, netfilter-devel
Cc: jmorris, sds, jengelh, paul.moore, casey, linux-security-module,
netfilter, mr.dash.four
Currently the nfconntrack export code sends the kernel internal secid to
userspace in a couple of proc files and over netlink as an integer. This
is wrong. This number is a kernel internal. This patch changes the export
code to output either 0 or 1 for this value. A future patch will implement
sending the name rather than the number in a new field.
Signed-off-by: Eric Paris <eparis@redhat.com>
---
.../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 2 +-
net/netfilter/nf_conntrack_netlink.c | 2 +-
net/netfilter/nf_conntrack_standalone.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 244f7cb..053d7d3 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -149,7 +149,7 @@ static int ct_seq_show(struct seq_file *s, void *v)
#endif
#ifdef CONFIG_NF_CONNTRACK_SECMARK
- if (seq_printf(s, "secmark=%u ", ct->secmark))
+ if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
goto release;
#endif
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 5bae1cd..3a50699 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -247,7 +247,7 @@ nla_put_failure:
static inline int
ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
{
- NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark));
+ NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark ? 1 : 0));
return 0;
nla_put_failure:
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index eb973fc..a5761d3 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -169,7 +169,7 @@ static int ct_seq_show(struct seq_file *s, void *v)
#endif
#ifdef CONFIG_NF_CONNTRACK_SECMARK
- if (seq_printf(s, "secmark=%u ", ct->secmark))
+ if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
goto release;
#endif
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-24 20:45 ` [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid Eric Paris
@ 2010-09-25 8:41 ` Pablo Neira Ayuso
2010-09-27 16:44 ` Eric Paris
2010-09-27 0:50 ` James Morris
1 sibling, 1 reply; 25+ messages in thread
From: Pablo Neira Ayuso @ 2010-09-25 8:41 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On 24/09/10 22:45, Eric Paris wrote:
> Currently the nfconntrack export code sends the kernel internal secid to
> userspace in a couple of proc files and over netlink as an integer. This
> is wrong. This number is a kernel internal. This patch changes the export
> code to output either 0 or 1 for this value. A future patch will implement
> sending the name rather than the number in a new field.
I'm not sure why you need this transitional patch if you later on
replace it.
Better to change the /proc output to make it consistent with patch 5/6?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-25 8:41 ` Pablo Neira Ayuso
@ 2010-09-27 16:44 ` Eric Paris
0 siblings, 0 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-27 16:44 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On Sat, 2010-09-25 at 10:41 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > Currently the nfconntrack export code sends the kernel internal secid to
> > userspace in a couple of proc files and over netlink as an integer. This
> > is wrong. This number is a kernel internal. This patch changes the export
> > code to output either 0 or 1 for this value. A future patch will implement
> > sending the name rather than the number in a new field.
>
> I'm not sure why you need this transitional patch if you later on
> replace it.
>
> Better to change the /proc output to make it consistent with patch 5/6?
Jan has stated his opinion I should not export the new secctx field via
the procfs interface. This patch was included in the series in case
that was the consensus of the lists and thus we just wouldn't apply
patch 6/6 (which drops secmark and adds secctx to procfs)
-Eric
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-24 20:45 ` [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid Eric Paris
2010-09-25 8:41 ` Pablo Neira Ayuso
@ 2010-09-27 0:50 ` James Morris
2010-09-27 17:01 ` Eric Paris
1 sibling, 1 reply; 25+ messages in thread
From: James Morris @ 2010-09-27 0:50 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, sds, jengelh, paul.moore,
casey, linux-security-module, netfilter, mr.dash.four
On Fri, 24 Sep 2010, Eric Paris wrote:
> Currently the nfconntrack export code sends the kernel internal secid to
> userspace in a couple of proc files and over netlink as an integer. This
> is wrong. This number is a kernel internal.
I wouldn't say wrong, exactly.
The secid is a 32-bit integer which each LSM maps to its own domain.
It's unlikely to change from this format (we were v. lucky to get this
field, btw).
There's really no difference between this and the various kernel addresses
exported via procfs by the networking code -- it's an opaque token which
you can use to identify the security marking of a packet without
necessarily having any knowledge of the security model itself.
It's potentially useful for debugging and writing simple userland tools,
and may be in use already by apps.
> This patch changes the export
> code to output either 0 or 1 for this value.
This potentially breaks existing apps and does not seem to add any value
itself. If you want to determine if a packet is marked, then simply check
for zero or not.
> A future patch will implement sending the name rather than the number
> in a new field.
For the reasons above, I think the secctx string needs to be exported in
addition to this rather than instead of.
- James
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 0:50 ` James Morris
@ 2010-09-27 17:01 ` Eric Paris
2010-09-27 18:29 ` Paul Moore
0 siblings, 1 reply; 25+ messages in thread
From: Eric Paris @ 2010-09-27 17:01 UTC (permalink / raw)
To: James Morris
Cc: linux-kernel, selinux, netfilter-devel, sds, jengelh, paul.moore,
casey, linux-security-module, netfilter, mr.dash.four
On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> On Fri, 24 Sep 2010, Eric Paris wrote:
> For the reasons above, I think the secctx string needs to be exported in
> addition to this rather than instead of.
I won't argue, I don't agree with your reasoning, but I'm not opposed to
this result. We have 3 competing suggestions:
Jan suggested we:
completely eliminate secmark from procfs+netlink and only export secctx
in netlink.
Eric suggested we:
completely eliminate secmark from procfs+netlink and then export secctx
in procfs+netlink
sounds like James suggested we:
continue to export meaningless and confusing secmark from procfs+netlink
and then export secctx in procfs+netlink as well.
I'm going to implement James' idea and resend the patch series. Any
strong objections?
-Eric
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 17:01 ` Eric Paris
@ 2010-09-27 18:29 ` Paul Moore
2010-09-27 19:25 ` Eric Paris
0 siblings, 1 reply; 25+ messages in thread
From: Paul Moore @ 2010-09-27 18:29 UTC (permalink / raw)
To: Eric Paris
Cc: James Morris, linux-kernel, selinux, netfilter-devel, sds,
jengelh, casey, linux-security-module, netfilter, mr.dash.four
On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > On Fri, 24 Sep 2010, Eric Paris wrote:
>
> > For the reasons above, I think the secctx string needs to be exported in
> > addition to this rather than instead of.
>
> I won't argue, I don't agree with your reasoning, but I'm not opposed to
> this result. We have 3 competing suggestions:
>
> Jan suggested we:
> completely eliminate secmark from procfs+netlink and only export secctx
> in netlink.
>
> Eric suggested we:
> completely eliminate secmark from procfs+netlink and then export secctx
> in procfs+netlink
>
> sounds like James suggested we:
> continue to export meaningless and confusing secmark from procfs+netlink
> and then export secctx in procfs+netlink as well.
>
> I'm going to implement James' idea and resend the patch series. Any
> strong objections?
I apologize for not getting a chance to look at these patches sooner.
In general they look fine to me and my only real concern was addressed
by Pablo already (breaking userspace due to #define changes).
As far as exporting the 32bit secid/secmark to userspace, I think that
is a mistake. James correctly points out that it does map to a LSM
specific value, e.g. SELinux and Smack security labels, but I don't
think he makes it clear that in the two LSMs that currently use secids
the mapping between the secid and the secctx is not constant; the secids
are transient values that will change with each boot in a manner that
userspace can not predict. For this reason, I think exporting the
secids is only going to cause users/admins grief, whereas exporting the
associated secctx should be a much more stable value and is likely what
the user/admin is expecting anyway.
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 18:29 ` Paul Moore
@ 2010-09-27 19:25 ` Eric Paris
2010-09-27 19:45 ` Paul Moore
` (3 more replies)
0 siblings, 4 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-27 19:25 UTC (permalink / raw)
To: Paul Moore
Cc: James Morris, linux-kernel, selinux, netfilter-devel, sds,
jengelh, casey, linux-security-module, netfilter, mr.dash.four
On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
> On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> > On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > > On Fri, 24 Sep 2010, Eric Paris wrote:
> >
> > > For the reasons above, I think the secctx string needs to be exported in
> > > addition to this rather than instead of.
> >
> > I won't argue, I don't agree with your reasoning, but I'm not opposed to
> > this result. We have 3 competing suggestions:
> >
> > Jan suggested we:
> > completely eliminate secmark from procfs+netlink and only export secctx
> > in netlink.
> >
> > Eric suggested we:
> > completely eliminate secmark from procfs+netlink and then export secctx
> > in procfs+netlink
> >
> > sounds like James suggested we:
> > continue to export meaningless and confusing secmark from procfs+netlink
> > and then export secctx in procfs+netlink as well.
> >
> > I'm going to implement James' idea and resend the patch series. Any
> > strong objections?
>
> I apologize for not getting a chance to look at these patches sooner.
> In general they look fine to me and my only real concern was addressed
> by Pablo already (breaking userspace due to #define changes).
>
> As far as exporting the 32bit secid/secmark to userspace, I think that
> is a mistake. James correctly points out that it does map to a LSM
> specific value, e.g. SELinux and Smack security labels, but I don't
> think he makes it clear that in the two LSMs that currently use secids
> the mapping between the secid and the secctx is not constant; the secids
> are transient values that will change with each boot in a manner that
> userspace can not predict. For this reason, I think exporting the
> secids is only going to cause users/admins grief, whereas exporting the
> associated secctx should be a much more stable value and is likely what
> the user/admin is expecting anyway.
So it sounds to me like Paul agrees with me that exporting the SELinux
sid as 'secmark=' was a bad idea. It's the whole reason this thread
started, someone wanted to be able to translate and use that field (and
instantly realized it was useless.)
I see it as having 3 options. lets assume was have a packet with
selinux sid=121 and selinux context=packet_t. We can
1) secmark=121 secctx=packet_t
This continues to send secmark like we do and people might continue to
be baffled by the 121.
2) secmark=1 secctx=packet_t
This sends a secmark field to userspace so if an application which
reads this exists (I doubt such an application actually exists in in the
real world) it will still get all of the information it got before but
noone will be baffled by what the number means. 1/0 is pretty obvious.
3) secctx=packet_t
Smallest easiest, what my patches actually do. Could theoretically
break some script that expected the field to be there, but any such
script is already broken since that field can be easily compiled
out......
James, if you are adamant about #1 I'll resend, otherwise I'm sticking
with #3.....
-Eric
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 19:25 ` Eric Paris
@ 2010-09-27 19:45 ` Paul Moore
2010-09-27 22:48 ` Pablo Neira Ayuso
` (2 subsequent siblings)
3 siblings, 0 replies; 25+ messages in thread
From: Paul Moore @ 2010-09-27 19:45 UTC (permalink / raw)
To: Eric Paris
Cc: James Morris, linux-kernel, selinux, netfilter-devel, sds,
jengelh, casey, linux-security-module, netfilter, mr.dash.four
On Mon, 2010-09-27 at 15:25 -0400, Eric Paris wrote:
> On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
> > On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
> > > On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
> > > > On Fri, 24 Sep 2010, Eric Paris wrote:
> > >
> > > > For the reasons above, I think the secctx string needs to be exported in
> > > > addition to this rather than instead of.
> > >
> > > I won't argue, I don't agree with your reasoning, but I'm not opposed to
> > > this result. We have 3 competing suggestions:
> > >
> > > Jan suggested we:
> > > completely eliminate secmark from procfs+netlink and only export secctx
> > > in netlink.
> > >
> > > Eric suggested we:
> > > completely eliminate secmark from procfs+netlink and then export secctx
> > > in procfs+netlink
> > >
> > > sounds like James suggested we:
> > > continue to export meaningless and confusing secmark from procfs+netlink
> > > and then export secctx in procfs+netlink as well.
> > >
> > > I'm going to implement James' idea and resend the patch series. Any
> > > strong objections?
> >
> > I apologize for not getting a chance to look at these patches sooner.
> > In general they look fine to me and my only real concern was addressed
> > by Pablo already (breaking userspace due to #define changes).
> >
> > As far as exporting the 32bit secid/secmark to userspace, I think that
> > is a mistake. James correctly points out that it does map to a LSM
> > specific value, e.g. SELinux and Smack security labels, but I don't
> > think he makes it clear that in the two LSMs that currently use secids
> > the mapping between the secid and the secctx is not constant; the secids
> > are transient values that will change with each boot in a manner that
> > userspace can not predict. For this reason, I think exporting the
> > secids is only going to cause users/admins grief, whereas exporting the
> > associated secctx should be a much more stable value and is likely what
> > the user/admin is expecting anyway.
>
> So it sounds to me like Paul agrees with me that exporting the SELinux
> sid as 'secmark=' was a bad idea. It's the whole reason this thread
> started, someone wanted to be able to translate and use that field (and
> instantly realized it was useless.)
>
> I see it as having 3 options. lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t. We can
>
> 1) secmark=121 secctx=packet_t
> This continues to send secmark like we do and people might continue to
> be baffled by the 121.
>
> 2) secmark=1 secctx=packet_t
> This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means. 1/0 is pretty obvious.
>
> 3) secctx=packet_t
> Smallest easiest, what my patches actually do. Could theoretically
> break some script that expected the field to be there, but any such
> script is already broken since that field can be easily compiled
> out......
>
> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....
James, if you do feel strongly about door #1, can you provide a good
example of how someone might use the secid to do something useful in
userspace? I ask because I can't think of anything and it would be nice
to have the example on record for future work.
--
paul moore
linux @ hp
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 19:25 ` Eric Paris
2010-09-27 19:45 ` Paul Moore
@ 2010-09-27 22:48 ` Pablo Neira Ayuso
2010-09-28 0:00 ` Jan Engelhardt
2010-09-27 23:45 ` James Morris
2010-09-28 12:32 ` Casey Schaufler
3 siblings, 1 reply; 25+ messages in thread
From: Pablo Neira Ayuso @ 2010-09-27 22:48 UTC (permalink / raw)
To: Eric Paris
Cc: Paul Moore, James Morris, linux-kernel, selinux, netfilter-devel,
sds, jengelh, casey, linux-security-module, netfilter,
mr.dash.four
On 27/09/10 21:25, Eric Paris wrote:
> I see it as having 3 options. lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t. We can
>
> 1) secmark=121 secctx=packet_t
> This continues to send secmark like we do and people might continue to
> be baffled by the 121.
>
> 2) secmark=1 secctx=packet_t
> This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means. 1/0 is pretty obvious.
In netlink, we can obsolete fields without breaking backward
compatibility. Applications parsing the /proc entry may break, but they
should use stable interfaces (like netlink) instead.
BTW, if we finally stop including CTA_SECMARK in netlink messages,
please add a small comment on the right of the definition in
nfnetlink_conntrack.h (something like /* obsolete */ or /* unused */).
Thanks!
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 22:48 ` Pablo Neira Ayuso
@ 2010-09-28 0:00 ` Jan Engelhardt
2010-09-28 8:45 ` Mr Dash Four
0 siblings, 1 reply; 25+ messages in thread
From: Jan Engelhardt @ 2010-09-28 0:00 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Eric Paris, Paul Moore, James Morris, linux-kernel, selinux,
netfilter-devel, sds, casey, linux-security-module, netfilter,
mr.dash.four
On Tuesday 2010-09-28 00:48, Pablo Neira Ayuso wrote:
>On 27/09/10 21:25, Eric Paris wrote:
>> I see it as having 3 options. lets assume was have a packet with
>> selinux sid=121 and selinux context=packet_t. We can
>>
>> 1) secmark=121 secctx=packet_t
>> This continues to send secmark like we do and people might continue to
>> be baffled by the 121.
>>
>> 2) secmark=1 secctx=packet_t
>> This sends a secmark field to userspace so if an application which
>> reads this exists (I doubt such an application actually exists in in the
>> real world) it will still get all of the information it got before but
>> noone will be baffled by what the number means. 1/0 is pretty obvious.
>
>In netlink, we can obsolete fields without breaking backward
>compatibility. Applications parsing the /proc entry may break, but they
>should use stable interfaces (like netlink) instead.
Which I take as a pro stance on not adding any more procfs fields.
>BTW, if we finally stop including CTA_SECMARK in netlink messages,
>please add a small comment on the right of the definition in
>nfnetlink_conntrack.h (something like /* obsolete */ or /* unused */).
>Thanks!
Mh, I prefer "obsolete". A lot of times in the kernel there is "unused"
and it reads like, "if it's unused, why is is there?" (it /is/ used,
though as a filler).
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-28 0:00 ` Jan Engelhardt
@ 2010-09-28 8:45 ` Mr Dash Four
0 siblings, 0 replies; 25+ messages in thread
From: Mr Dash Four @ 2010-09-28 8:45 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Pablo Neira Ayuso, Eric Paris, Paul Moore, James Morris,
linux-kernel, selinux, netfilter-devel, sds, casey,
linux-security-module, netfilter
>> In netlink, we can obsolete fields without breaking backward
>> compatibility. Applications parsing the /proc entry may break, but they
>> should use stable interfaces (like netlink) instead.
>>
>
> Which I take as a pro stance on not adding any more procfs fields.
>
How did you figure that one out then?
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 19:25 ` Eric Paris
2010-09-27 19:45 ` Paul Moore
2010-09-27 22:48 ` Pablo Neira Ayuso
@ 2010-09-27 23:45 ` James Morris
2010-09-28 12:32 ` Casey Schaufler
3 siblings, 0 replies; 25+ messages in thread
From: James Morris @ 2010-09-27 23:45 UTC (permalink / raw)
To: Eric Paris
Cc: Paul Moore, linux-kernel, selinux, netfilter-devel, sds, jengelh,
casey, linux-security-module, netfilter, mr.dash.four
On Mon, 27 Sep 2010, Eric Paris wrote:
> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....
Looks like #3 it is.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid
2010-09-27 19:25 ` Eric Paris
` (2 preceding siblings ...)
2010-09-27 23:45 ` James Morris
@ 2010-09-28 12:32 ` Casey Schaufler
3 siblings, 0 replies; 25+ messages in thread
From: Casey Schaufler @ 2010-09-28 12:32 UTC (permalink / raw)
To: Eric Paris
Cc: Paul Moore, James Morris, linux-kernel, selinux, netfilter-devel,
sds, jengelh, linux-security-module, netfilter, mr.dash.four
On 9/27/2010 12:25 PM, Eric Paris wrote:
> On Mon, 2010-09-27 at 14:29 -0400, Paul Moore wrote:
>> On Mon, 2010-09-27 at 13:01 -0400, Eric Paris wrote:
>>> On Mon, 2010-09-27 at 10:50 +1000, James Morris wrote:
>>>> On Fri, 24 Sep 2010, Eric Paris wrote:
>>>> For the reasons above, I think the secctx string needs to be exported in
>>>> addition to this rather than instead of.
>>> I won't argue, I don't agree with your reasoning, but I'm not opposed to
>>> this result. We have 3 competing suggestions:
>>>
>>> Jan suggested we:
>>> completely eliminate secmark from procfs+netlink and only export secctx
>>> in netlink.
>>>
>>> Eric suggested we:
>>> completely eliminate secmark from procfs+netlink and then export secctx
>>> in procfs+netlink
>>>
>>> sounds like James suggested we:
>>> continue to export meaningless and confusing secmark from procfs+netlink
>>> and then export secctx in procfs+netlink as well.
>>>
>>> I'm going to implement James' idea and resend the patch series. Any
>>> strong objections?
>> I apologize for not getting a chance to look at these patches sooner.
>> In general they look fine to me and my only real concern was addressed
>> by Pablo already (breaking userspace due to #define changes).
>>
>> As far as exporting the 32bit secid/secmark to userspace, I think that
>> is a mistake. James correctly points out that it does map to a LSM
>> specific value, e.g. SELinux and Smack security labels, but I don't
>> think he makes it clear that in the two LSMs that currently use secids
>> the mapping between the secid and the secctx is not constant; the secids
>> are transient values that will change with each boot in a manner that
>> userspace can not predict. For this reason, I think exporting the
>> secids is only going to cause users/admins grief, whereas exporting the
>> associated secctx should be a much more stable value and is likely what
>> the user/admin is expecting anyway.
> So it sounds to me like Paul agrees with me that exporting the SELinux
> sid as 'secmark=' was a bad idea. It's the whole reason this thread
> started, someone wanted to be able to translate and use that field (and
> instantly realized it was useless.)
>
> I see it as having 3 options. lets assume was have a packet with
> selinux sid=121 and selinux context=packet_t. We can
>
> 1) secmark=121 secctx=packet_t
> This continues to send secmark like we do and people might continue to
> be baffled by the 121.
>
> 2) secmark=1 secctx=packet_t
> This sends a secmark field to userspace so if an application which
> reads this exists (I doubt such an application actually exists in in the
> real world) it will still get all of the information it got before but
> noone will be baffled by what the number means. 1/0 is pretty obvious.
>
> 3) secctx=packet_t
> Smallest easiest, what my patches actually do. Could theoretically
> break some script that expected the field to be there, but any such
> script is already broken since that field can be easily compiled
> out......
>
> James, if you are adamant about #1 I'll resend, otherwise I'm sticking
> with #3.....
The Smack Project leadership recommends #3.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/6] security: secid_to_secctx returns len when data is NULL
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
2010-09-24 20:45 ` [PATCH 2/6] secmark: make secmark object handling generic Eric Paris
2010-09-24 20:45 ` [PATCH 3/6] secmark: export binary yes/no rather than kernel internal secid Eric Paris
@ 2010-09-24 20:45 ` Eric Paris
2010-09-27 13:49 ` Casey Schaufler
2010-09-24 20:45 ` [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink Eric Paris
` (2 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Eric Paris @ 2010-09-24 20:45 UTC (permalink / raw)
To: linux-kernel, selinux, netfilter-devel
Cc: jmorris, sds, jengelh, paul.moore, casey, linux-security-module,
netfilter, mr.dash.four
With the (long ago) interface change to have the secid_to_secctx functions
do the string allocation instead of having the caller do the allocation we
lost the ability to query the security server for the length of the
upcoming string. The SECMARK code would like to allocate a netlink skb
with enough length to hold the string but it is just too unclean to do the
string allocation twice or to do the allocation the first time and hold
onto the string and slen. This patch adds the ability to call
security_secid_to_secctx() with a NULL data pointer and it will just set
the slen pointer.
Signed-off-by: Eric Paris <eparis@redhat.com>
---
security/selinux/ss/services.c | 11 +++++++++--
security/smack/smack_lsm.c | 3 ++-
2 files changed, 11 insertions(+), 3 deletions(-)
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 73508af..1d4955a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -998,7 +998,8 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
{
char *scontextp;
- *scontext = NULL;
+ if (scontext)
+ *scontext = NULL;
*scontext_len = 0;
if (context->len) {
@@ -1015,6 +1016,9 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
*scontext_len += strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)) + 1;
*scontext_len += mls_compute_context_len(context);
+ if (!scontext)
+ return 0;
+
/* Allocate space for the context; caller must free this space. */
scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
if (!scontextp)
@@ -1054,7 +1058,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
struct context *context;
int rc = 0;
- *scontext = NULL;
+ if (scontext)
+ *scontext = NULL;
*scontext_len = 0;
if (!ss_initialized) {
@@ -1062,6 +1067,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
char *scontextp;
*scontext_len = strlen(initial_sid_to_string[sid]) + 1;
+ if (!scontext)
+ goto out;
scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
if (!scontextp) {
rc = -ENOMEM;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index c448d57..b95d7b1 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3005,7 +3005,8 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
{
char *sp = smack_from_secid(secid);
- *secdata = sp;
+ if (secdata)
+ *secdata = sp;
*seclen = strlen(sp);
return 0;
}
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 4/6] security: secid_to_secctx returns len when data is NULL
2010-09-24 20:45 ` [PATCH 4/6] security: secid_to_secctx returns len when data is NULL Eric Paris
@ 2010-09-27 13:49 ` Casey Schaufler
0 siblings, 0 replies; 25+ messages in thread
From: Casey Schaufler @ 2010-09-27 13:49 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, linux-security-module, netfilter, mr.dash.four
On 9/24/2010 1:45 PM, Eric Paris wrote:
> With the (long ago) interface change to have the secid_to_secctx functions
> do the string allocation instead of having the caller do the allocation we
> lost the ability to query the security server for the length of the
> upcoming string. The SECMARK code would like to allocate a netlink skb
> with enough length to hold the string but it is just too unclean to do the
> string allocation twice or to do the allocation the first time and hold
> onto the string and slen. This patch adds the ability to call
> security_secid_to_secctx() with a NULL data pointer and it will just set
> the slen pointer.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: Casey Schaufler <casey@schaufler-ca.com>
For the Smack bit at least.
> ---
>
> security/selinux/ss/services.c | 11 +++++++++--
> security/smack/smack_lsm.c | 3 ++-
> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 73508af..1d4955a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -998,7 +998,8 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
> {
> char *scontextp;
>
> - *scontext = NULL;
> + if (scontext)
> + *scontext = NULL;
> *scontext_len = 0;
>
> if (context->len) {
> @@ -1015,6 +1016,9 @@ static int context_struct_to_string(struct context *context, char **scontext, u3
> *scontext_len += strlen(sym_name(&policydb, SYM_TYPES, context->type - 1)) + 1;
> *scontext_len += mls_compute_context_len(context);
>
> + if (!scontext)
> + return 0;
> +
> /* Allocate space for the context; caller must free this space. */
> scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
> if (!scontextp)
> @@ -1054,7 +1058,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
> struct context *context;
> int rc = 0;
>
> - *scontext = NULL;
> + if (scontext)
> + *scontext = NULL;
> *scontext_len = 0;
>
> if (!ss_initialized) {
> @@ -1062,6 +1067,8 @@ static int security_sid_to_context_core(u32 sid, char **scontext,
> char *scontextp;
>
> *scontext_len = strlen(initial_sid_to_string[sid]) + 1;
> + if (!scontext)
> + goto out;
> scontextp = kmalloc(*scontext_len, GFP_ATOMIC);
> if (!scontextp) {
> rc = -ENOMEM;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index c448d57..b95d7b1 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3005,7 +3005,8 @@ static int smack_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> {
> char *sp = smack_from_secid(secid);
>
> - *secdata = sp;
> + if (secdata)
> + *secdata = sp;
> *seclen = strlen(sp);
> return 0;
> }
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
` (2 preceding siblings ...)
2010-09-24 20:45 ` [PATCH 4/6] security: secid_to_secctx returns len when data is NULL Eric Paris
@ 2010-09-24 20:45 ` Eric Paris
2010-09-24 21:08 ` Jan Engelhardt
2010-09-27 11:01 ` Pablo Neira Ayuso
2010-09-24 20:45 ` [PATCH 6/6] secmark: export secctx, drop secmark in procfs Eric Paris
2010-09-24 21:01 ` [PATCH 1/6] secmark: do not return early if there was no error Jan Engelhardt
5 siblings, 2 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-24 20:45 UTC (permalink / raw)
To: linux-kernel, selinux, netfilter-devel
Cc: jmorris, sds, jengelh, paul.moore, casey, linux-security-module,
netfilter, mr.dash.four
The conntrack code can export the internal secid to userspace. These are
dynamic, can change on lsm changes, and have no meaning in userspace. We
should instead be sending lsm contexts to userspace instead. This patch sends
the secctx (rather than secid) to userspace over the netlink socket. We use a
new field CTA_SECCTX and stop using the the old CTA_SECMARK field since it did
not send particularly useful information.
Signed-off-by: Eric Paris <eparis@redhat.com>
---
include/linux/netfilter/nfnetlink_conntrack.h | 8 ++++
net/netfilter/nf_conntrack_netlink.c | 46 ++++++++++++++++++++-----
2 files changed, 44 insertions(+), 10 deletions(-)
diff --git a/include/linux/netfilter/nfnetlink_conntrack.h b/include/linux/netfilter/nfnetlink_conntrack.h
index 9ed534c..ba03544 100644
--- a/include/linux/netfilter/nfnetlink_conntrack.h
+++ b/include/linux/netfilter/nfnetlink_conntrack.h
@@ -41,6 +41,7 @@ enum ctattr_type {
CTA_NAT_SEQ_ADJ_REPLY,
CTA_SECMARK,
CTA_ZONE,
+ CTA_SECCTX,
__CTA_MAX
};
#define CTA_MAX (__CTA_MAX - 1)
@@ -172,4 +173,11 @@ enum ctattr_help {
};
#define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
+enum ctattr_secctx {
+ CTA_SECCTX_UNSPEC,
+ CTA_SECCTX_NAME,
+ __CTA_SECCTX_MAX
+};
+#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)
+
#endif /* _IPCONNTRACK_NETLINK_H */
diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 3a50699..b3c6285 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -22,6 +22,7 @@
#include <linux/rculist_nulls.h>
#include <linux/types.h>
#include <linux/timer.h>
+#include <linux/security.h>
#include <linux/skbuff.h>
#include <linux/errno.h>
#include <linux/netlink.h>
@@ -245,16 +246,31 @@ nla_put_failure:
#ifdef CONFIG_NF_CONNTRACK_SECMARK
static inline int
-ctnetlink_dump_secmark(struct sk_buff *skb, const struct nf_conn *ct)
+ctnetlink_dump_secctx(struct sk_buff *skb, const struct nf_conn *ct)
{
- NLA_PUT_BE32(skb, CTA_SECMARK, htonl(ct->secmark ? 1 : 0));
- return 0;
+ struct nlattr *nest_secctx;
+ int len, ret;
+ char *secctx;
+
+ ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+ if (ret)
+ return ret;
+
+ ret = -1;
+ nest_secctx = nla_nest_start(skb, CTA_SECCTX | NLA_F_NESTED);
+ if (!nest_secctx)
+ goto nla_put_failure;
+ NLA_PUT_STRING(skb, CTA_SECCTX_NAME, secctx);
+ nla_nest_end(skb, nest_secctx);
+
+ ret = 0;
nla_put_failure:
- return -1;
+ security_release_secctx(secctx, len);
+ return ret;
}
#else
-#define ctnetlink_dump_secmark(a, b) (0)
+#define ctnetlink_dump_secctx(a, b) (0)
#endif
#define master_tuple(ct) &(ct->master->tuplehash[IP_CT_DIR_ORIGINAL].tuple)
@@ -391,7 +407,7 @@ ctnetlink_fill_info(struct sk_buff *skb, u32 pid, u32 seq,
ctnetlink_dump_protoinfo(skb, ct) < 0 ||
ctnetlink_dump_helpinfo(skb, ct) < 0 ||
ctnetlink_dump_mark(skb, ct) < 0 ||
- ctnetlink_dump_secmark(skb, ct) < 0 ||
+ ctnetlink_dump_secctx(skb, ct) < 0 ||
ctnetlink_dump_id(skb, ct) < 0 ||
ctnetlink_dump_use(skb, ct) < 0 ||
ctnetlink_dump_master(skb, ct) < 0 ||
@@ -437,6 +453,17 @@ ctnetlink_counters_size(const struct nf_conn *ct)
;
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ctnetlink_nlmsg_secctx_size(const struct nf_conn *ct)
+{
+ int len;
+
+ security_secid_to_secctx(ct->secmark, NULL, &len);
+
+ return sizeof(char) * len;
+}
+#endif
+
static inline size_t
ctnetlink_nlmsg_size(const struct nf_conn *ct)
{
@@ -453,7 +480,8 @@ ctnetlink_nlmsg_size(const struct nf_conn *ct)
+ nla_total_size(0) /* CTA_HELP */
+ nla_total_size(NF_CT_HELPER_NAME_LEN) /* CTA_HELP_NAME */
#ifdef CONFIG_NF_CONNTRACK_SECMARK
- + nla_total_size(sizeof(u_int32_t)) /* CTA_SECMARK */
+ + nla_total_size(0) /* CTA_SECCTX */
+ + nla_total_size(ctnetlink_nlmsg_secctx_size(ct)) /* CTA_SECCTX_NAME */
#endif
#ifdef CONFIG_NF_NAT_NEEDED
+ 2 * nla_total_size(0) /* CTA_NAT_SEQ_ADJ_ORIG|REPL */
@@ -554,11 +582,9 @@ ctnetlink_conntrack_event(unsigned int events, struct nf_ct_event *item)
&& ctnetlink_dump_helpinfo(skb, ct) < 0)
goto nla_put_failure;
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
if ((events & (1 << IPCT_SECMARK) || ct->secmark)
- && ctnetlink_dump_secmark(skb, ct) < 0)
+ && ctnetlink_dump_secctx(skb, ct) < 0)
goto nla_put_failure;
-#endif
if (events & (1 << IPCT_RELATED) &&
ctnetlink_dump_master(skb, ct) < 0)
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink
2010-09-24 20:45 ` [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink Eric Paris
@ 2010-09-24 21:08 ` Jan Engelhardt
2010-09-27 11:01 ` Pablo Neira Ayuso
1 sibling, 0 replies; 25+ messages in thread
From: Jan Engelhardt @ 2010-09-24 21:08 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, paul.moore,
casey, linux-security-module, netfilter, mr.dash.four
On Friday 2010-09-24 22:45, Eric Paris wrote:
>+#ifdef CONFIG_NF_CONNTRACK_SECMARK
>+static int ctnetlink_nlmsg_secctx_size(const struct nf_conn *ct)
>+{
>+ int len;
>+
>+ security_secid_to_secctx(ct->secmark, NULL, &len);
>+
>+ return sizeof(char) * len;
>+}
>+#endif
sizeof(char) is defined to be 1, so it's a little redundant.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink
2010-09-24 20:45 ` [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink Eric Paris
2010-09-24 21:08 ` Jan Engelhardt
@ 2010-09-27 11:01 ` Pablo Neira Ayuso
2010-09-27 16:51 ` Eric Paris
1 sibling, 1 reply; 25+ messages in thread
From: Pablo Neira Ayuso @ 2010-09-27 11:01 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On 24/09/10 22:45, Eric Paris wrote:
> @@ -172,4 +173,11 @@ enum ctattr_help {
> };
> #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
>
> +enum ctattr_secctx {
> + CTA_SECCTX_UNSPEC,
> + CTA_SECCTX_NAME,
> + __CTA_SECCTX_MAX
> +};
> +#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)
I guess that you have included this nest for consistency with CTA_HELP.
My question: do you think that we'll include more attributes in that
nest in the future? Otherwise, I would remove that nest and put
CTA_SECCTX_NAME in the first level, since the nest would increase the
message size.
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink
2010-09-27 11:01 ` Pablo Neira Ayuso
@ 2010-09-27 16:51 ` Eric Paris
0 siblings, 0 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-27 16:51 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, jengelh,
paul.moore, casey, linux-security-module, netfilter, mr.dash.four
On Mon, 2010-09-27 at 13:01 +0200, Pablo Neira Ayuso wrote:
> On 24/09/10 22:45, Eric Paris wrote:
> > @@ -172,4 +173,11 @@ enum ctattr_help {
> > };
> > #define CTA_HELP_MAX (__CTA_HELP_MAX - 1)
> >
> > +enum ctattr_secctx {
> > + CTA_SECCTX_UNSPEC,
> > + CTA_SECCTX_NAME,
> > + __CTA_SECCTX_MAX
> > +};
> > +#define CTA_SECCTX_MAX (__CTA_SECCTX_MAX - 1)
>
> I guess that you have included this nest for consistency with CTA_HELP.
> My question: do you think that we'll include more attributes in that
> nest in the future? Otherwise, I would remove that nest and put
> CTA_SECCTX_NAME in the first level, since the nest would increase the
> message size.
My initial thought was that you were right (and I did just copy the
CTA_HELP implementation), but now I've decided that I like that nest. I
have no idea what future representation an LSM might use. The only 2
LSMs that make sense to use this interface SELinux and SMACK both use a
string, but I don't know why we have to limit it to that....
-Eric
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 6/6] secmark: export secctx, drop secmark in procfs
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
` (3 preceding siblings ...)
2010-09-24 20:45 ` [PATCH 5/6] conntrack: export lsm context rather than internal secid via netlink Eric Paris
@ 2010-09-24 20:45 ` Eric Paris
2010-09-24 21:01 ` [PATCH 1/6] secmark: do not return early if there was no error Jan Engelhardt
5 siblings, 0 replies; 25+ messages in thread
From: Eric Paris @ 2010-09-24 20:45 UTC (permalink / raw)
To: linux-kernel, selinux, netfilter-devel
Cc: jmorris, sds, jengelh, paul.moore, casey, linux-security-module,
netfilter, mr.dash.four
The current secmark code exports a secmark= field which just indicates if
there is special labeling on a packet or not. We drop this field as it
isn't particularly useful and instead export a new field secctx= which is
the actual human readable text label.
Signed-off-by: Eric Paris <eparis@redhat.com>
---
.../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 27 ++++++++++++++++++--
net/netfilter/nf_conntrack_standalone.c | 27 ++++++++++++++++++--
2 files changed, 48 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
index 053d7d3..2ca510e 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4_compat.c
@@ -11,6 +11,7 @@
#include <linux/proc_fs.h>
#include <linux/seq_file.h>
#include <linux/percpu.h>
+#include <linux/security.h>
#include <net/net_namespace.h>
#include <linux/netfilter.h>
@@ -87,6 +88,28 @@ static void ct_seq_stop(struct seq_file *s, void *v)
rcu_read_unlock();
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+ int len, ret;
+ char *secctx;
+
+ ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+ if (ret)
+ return ret;
+
+ ret = seq_printf(s, "secctx=%s ", secctx);
+
+ security_release_secctx(secctx, len);
+ return ret;
+}
+#else
+static inline int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+ return 0;
+}
+#endif
+
static int ct_seq_show(struct seq_file *s, void *v)
{
struct nf_conntrack_tuple_hash *hash = v;
@@ -148,10 +171,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
goto release;
#endif
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
- if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
+ if (ct_show_secctx(s, ct))
goto release;
-#endif
if (seq_printf(s, "use=%u\n", atomic_read(&ct->ct_general.use)))
goto release;
diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
index a5761d3..a6985da 100644
--- a/net/netfilter/nf_conntrack_standalone.c
+++ b/net/netfilter/nf_conntrack_standalone.c
@@ -15,6 +15,7 @@
#include <linux/seq_file.h>
#include <linux/percpu.h>
#include <linux/netdevice.h>
+#include <linux/security.h>
#include <net/net_namespace.h>
#ifdef CONFIG_SYSCTL
#include <linux/sysctl.h>
@@ -108,6 +109,28 @@ static void ct_seq_stop(struct seq_file *s, void *v)
rcu_read_unlock();
}
+#ifdef CONFIG_NF_CONNTRACK_SECMARK
+static int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+ int len, ret;
+ char *secctx;
+
+ ret = security_secid_to_secctx(ct->secmark, &secctx, &len);
+ if (ret)
+ return ret;
+
+ ret = seq_printf(s, "secctx=%s ", secctx);
+
+ security_release_secctx(secctx, len);
+ return ret;
+}
+#else
+static inline int ct_show_secctx(struct seq_file *s, const struct nf_conn *ct)
+{
+ return 0;
+}
+#endif
+
/* return 0 on success, 1 in case of error */
static int ct_seq_show(struct seq_file *s, void *v)
{
@@ -168,10 +191,8 @@ static int ct_seq_show(struct seq_file *s, void *v)
goto release;
#endif
-#ifdef CONFIG_NF_CONNTRACK_SECMARK
- if (seq_printf(s, "secmark=%u ", ct->secmark ? 1 : 0))
+ if (ct_show_secctx(s, ct))
goto release;
-#endif
#ifdef CONFIG_NF_CONNTRACK_ZONES
if (seq_printf(s, "zone=%u ", nf_ct_zone(ct)))
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 1/6] secmark: do not return early if there was no error
2010-09-24 20:45 [PATCH 1/6] secmark: do not return early if there was no error Eric Paris
` (4 preceding siblings ...)
2010-09-24 20:45 ` [PATCH 6/6] secmark: export secctx, drop secmark in procfs Eric Paris
@ 2010-09-24 21:01 ` Jan Engelhardt
5 siblings, 0 replies; 25+ messages in thread
From: Jan Engelhardt @ 2010-09-24 21:01 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, selinux, netfilter-devel, jmorris, sds, paul.moore,
casey, linux-security-module, netfilter, mr.dash.four
On Friday 2010-09-24 22:45, Eric Paris wrote:
>Commit 4a5a5c73 attempted to pass decent error messages back to userspace for
>netfilter errors. In xt_SECMARK.c however the patch screwed up and returned
>on 0 (aka no error) early and didn't finish setting up secmark. This results
>in a kernel BUG if you use SECMARK.
>+++ b/net/netfilter/xt_SECMARK.c
>@@ -101,7 +101,7 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
> switch (info->mode) {
> case SECMARK_MODE_SEL:
> err = checkentry_selinux(info);
>- if (err <= 0)
>+ if (err)
> return err;
Indeed the = is unwanted and err < 0 was intended here.
Sorry for the slip.
^ permalink raw reply [flat|nested] 25+ messages in thread