Date:	Tue, 12 Oct 2010 18:55:43 -0400
From:	Paul Moore <paul.moore@...com>
To:	Eric Paris <eparis@...hat.com>
Cc:	linux-kernel@...r.kernel.org, netfilter-devel@...r.kernel.org,
	netfilter@...r.kernel.org, jmorris@...ei.org,
	selinux@...ho.nsa.gov, sds@...ho.nsa.gov, jengelh@...ozas.de,
	linux-security-module@...r.kernel.org, mr.dash.four@...glemail.com,
	pablo@...filter.org
Subject: Re: [PATCH 2/5] secmark: make secmark object handling generic

On Tue, 2010-10-12 at 11:40 -0400, 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)

I'm sure you have, but I just want to make sure - you've tested this
change (and the others for that matter) against the existing iptables
userspace to make sure everything still works, right?

> Signed-off-by: Eric Paris <eparis@...hat.com>
> ---
> 
>  include/linux/netfilter/xt_SECMARK.h |   12 ++----
>  include/linux/security.h             |   25 +++++++++++++
>  include/linux/selinux.h              |   63 ----------------------------------
>  net/netfilter/xt_CT.c                |    1 -
>  net/netfilter/xt_SECMARK.c           |   51 +++++++++++++---------------
>  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, 112 insertions(+), 149 deletions(-)
> 
> diff --git a/include/linux/netfilter/xt_SECMARK.h b/include/linux/netfilter/xt_SECMARK.h
> index 6fcd344..989092b 100644
> --- a/include/linux/netfilter/xt_SECMARK.h
> +++ b/include/linux/netfilter/xt_SECMARK.h
> @@ -11,18 +11,12 @@
>   * 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_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 43771c8..f050119 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:
> @@ -1596,6 +1602,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);
> @@ -2556,6 +2565,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);
> @@ -2710,6 +2722,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..295054e 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>
>  
> @@ -39,9 +39,8 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  
>  	switch (mode) {
>  	case SECMARK_MODE_SEL:
> -		secmark = info->u.sel.selsid;
> +		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;
>  }
>  
> @@ -100,16 +99,16 @@ 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;
>  		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;
> @@ -119,19 +118,19 @@ static void secmark_tg_destroy(const struct xt_tgdtor_param *par)
>  {
>  	switch (mode) {
>  	case SECMARK_MODE_SEL:
> -		selinux_secmark_refcount_dec();
> +		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 fe2da98..a8aa414 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1143,6 +1143,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 f31b0a3..a6c063b 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 79df4a2..671273e 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 */
> 

-- 
paul moore
linux @ hp


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists