Message-ID: <bebdacdf-2ecb-8e07-1b0e-6e6bfb5960d0@canonical.com>
Date:   Tue, 28 Jul 2020 04:11:27 -0700
From:   John Johansen <john.johansen@...onical.com>
To:     Casey Schaufler <casey@...aufler-ca.com>,
        casey.schaufler@...el.com, jmorris@...ei.org,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org
Cc:     linux-audit@...hat.com, keescook@...omium.org,
        penguin-kernel@...ove.sakura.ne.jp, paul@...l-moore.com,
        sds@...ho.nsa.gov, netdev@...r.kernel.org
Subject: Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid

On 7/24/20 1:32 PM, Casey Schaufler wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. Change security_secmark_relabel_packet() to use a
> lsmblob instead of a u32 secid. In some other cases there is
> scaffolding where interfaces have yet to be converted.
> 
> Reviewed-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> Cc: netdev@...r.kernel.org

one comment below, but its a nice to have so

Reviewed-by: John Johansen <john.johansen@...onical.com>


> ---
>  include/linux/security.h          | 30 +++++++++++++++++++++++----
>  include/net/scm.h                 |  7 +++++--
>  kernel/cred.c                     |  4 +---
>  net/ipv4/ip_sockglue.c            |  6 ++++--
>  net/netfilter/nft_meta.c          | 18 +++++++++-------
>  net/netfilter/xt_SECMARK.c        |  9 ++++++--
>  net/netlabel/netlabel_unlabeled.c | 23 +++++++++++++--------
>  security/security.c               | 34 ++++++++++++++++++++++++++-----
>  8 files changed, 98 insertions(+), 33 deletions(-)
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index d81e8886d799..98176faaaba5 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,6 +189,27 @@ static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
>  	return !memcmp(bloba, blobb, sizeof(*bloba));
>  }
>  
> +/**
> + * lsmblob_value - find the first non-zero value in an lsmblob structure.
> + * @blob: Pointer to the data
> + *
> + * This needs to be used with extreme caution, as the cases where
> + * it is appropriate are rare.
> + *
> + * Return the first secid value set in the lsmblob.
> + * There should only be one.

It would be really nice if we could have an LSM debug config, that would
do things like checking there is indeed only one value when this fn
is called.

> + */
> +static inline u32 lsmblob_value(const struct lsmblob *blob)
> +{
> +	int i;
> +
> +	for (i = 0; i < LSMBLOB_ENTRIES; i++)
> +		if (blob->secid[i])
> +			return blob->secid[i];
> +
> +	return 0;
> +}
> +
>  /* These functions are in security/commoncap.c */
>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>  		       int cap, unsigned int opts);
> @@ -502,7 +523,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>  int security_ismaclabel(const char *name);
>  int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> +			     struct lsmblob *blob);
>  void security_release_secctx(char *secdata, u32 seclen);
>  void security_inode_invalidate_secctx(struct inode *inode);
>  int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1321,7 +1343,7 @@ static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *secle
>  
>  static inline int security_secctx_to_secid(const char *secdata,
>  					   u32 seclen,
> -					   u32 *secid)
> +					   struct lsmblob *blob)
>  {
>  	return -EOPNOTSUPP;
>  }
> @@ -1411,7 +1433,7 @@ 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);
> +int security_secmark_relabel_packet(struct lsmblob *blob);
>  void security_secmark_refcount_inc(void);
>  void security_secmark_refcount_dec(void);
>  int security_tun_dev_alloc_security(void **security);
> @@ -1584,7 +1606,7 @@ static inline void security_inet_conn_established(struct sock *sk,
>  {
>  }
>  
> -static inline int security_secmark_relabel_packet(u32 secid)
> +static inline int security_secmark_relabel_packet(struct lsmblob *blob)
>  {
>  	return 0;
>  }
> diff --git a/include/net/scm.h b/include/net/scm.h
> index e2e71c4bf9d0..c09f2dfeec88 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -97,8 +97,11 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>  	int err;
>  
>  	if (test_bit(SOCK_PASSSEC, &sock->flags)) {
> -		/* Scaffolding - it has to be element 0 for now */
> -		err = security_secid_to_secctx(scm->lsmblob.secid[0],
> +		/* There can currently be only one value in the lsmblob,
> +		 * so getting it from lsmblob_value is appropriate until
> +		 * security_secid_to_secctx() is converted to taking a
> +		 * lsmblob directly. */
> +		err = security_secid_to_secctx(lsmblob_value(&scm->lsmblob),
>  					       &secdata, &seclen);
>  
>  		if (!err) {
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 22e0e7cbefde..848306c7d823 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -757,14 +757,12 @@ EXPORT_SYMBOL(set_security_override);
>  int set_security_override_from_ctx(struct cred *new, const char *secctx)
>  {
>  	struct lsmblob blob;
> -	u32 secid;
>  	int ret;
>  
> -	ret = security_secctx_to_secid(secctx, strlen(secctx), &secid);
> +	ret = security_secctx_to_secid(secctx, strlen(secctx), &blob);
>  	if (ret < 0)
>  		return ret;
>  
> -	lsmblob_init(&blob, secid);
>  	return set_security_override(new, &blob);
>  }
>  EXPORT_SYMBOL(set_security_override_from_ctx);
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index 3ea1103b4c29..6bdac5f87a1e 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -139,8 +139,10 @@ static void ip_cmsg_recv_security(struct msghdr *msg, struct sk_buff *skb)
>  	if (err)
>  		return;
>  
> -	/* Scaffolding - it has to be element 0 */
> -	err = security_secid_to_secctx(lb.secid[0], &secdata, &seclen);
> +	/* There can only be one secid in the lsmblob at this point,
> +	 * so getting it using lsmblob_value() is sufficient until
> +	 * security_secid_to_secctx() is changed to use a lsmblob */
> +	err = security_secid_to_secctx(lsmblob_value(&lb), &secdata, &seclen);
>  	if (err)
>  		return;
>  
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 951b6e87ed5d..5875222aeac5 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -801,7 +801,7 @@ struct nft_expr_type nft_meta_type __read_mostly = {
>  
>  #ifdef CONFIG_NETWORK_SECMARK
>  struct nft_secmark {
> -	u32 secid;
> +	struct lsmblob lsmdata;
>  	char *ctx;
>  };
>  
> @@ -811,21 +811,21 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>  
>  static int nft_secmark_compute_secid(struct nft_secmark *priv)
>  {
> -	u32 tmp_secid = 0;
> +	struct lsmblob blob;
>  	int err;
>  
> -	err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
> +	err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
>  	if (err)
>  		return err;
>  
> -	if (!tmp_secid)
> +	if (!lsmblob_is_set(&blob))
>  		return -ENOENT;
>  
> -	err = security_secmark_relabel_packet(tmp_secid);
> +	err = security_secmark_relabel_packet(&blob);
>  	if (err)
>  		return err;
>  
> -	priv->secid = tmp_secid;
> +	priv->lsmdata = blob;
>  	return 0;
>  }
>  
> @@ -835,7 +835,11 @@ static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs *regs,
>  	const struct nft_secmark *priv = nft_obj_data(obj);
>  	struct sk_buff *skb = pkt->skb;
>  
> -	skb->secmark = priv->secid;
> +	/* It is not possible for more than one secid to be set in
> +	 * the lsmblob structure because it is set using
> +	 * security_secctx_to_secid(). Any secid that is set must therefore
> +	 * be the one that should go in the secmark. */
> +	skb->secmark = lsmblob_value(&priv->lsmdata);
>  }
>  
>  static int nft_secmark_obj_init(const struct nft_ctx *ctx,
> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index 75625d13e976..5a268707eeda 100644
> --- a/net/netfilter/xt_SECMARK.c
> +++ b/net/netfilter/xt_SECMARK.c
> @@ -43,13 +43,14 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>  
>  static int checkentry_lsm(struct xt_secmark_target_info *info)
>  {
> +	struct lsmblob blob;
>  	int err;
>  
>  	info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
>  	info->secid = 0;
>  
>  	err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
> -				       &info->secid);
> +				       &blob);
>  	if (err) {
>  		if (err == -EINVAL)
>  			pr_info_ratelimited("invalid security context \'%s\'\n",
> @@ -57,13 +58,17 @@ static int checkentry_lsm(struct xt_secmark_target_info *info)
>  		return err;
>  	}
>  
> +	/* xt_secmark_target_info can't be changed to use lsmblobs because
> +	 * it is exposed as an API. Use lsmblob_value() to get the one
> +	 * value that got set by security_secctx_to_secid(). */
> +	info->secid = lsmblob_value(&blob);
>  	if (!info->secid) {
>  		pr_info_ratelimited("unable to map security context \'%s\'\n",
>  				    info->secctx);
>  		return -ENOENT;
>  	}
>  
> -	err = security_secmark_relabel_packet(info->secid);
> +	err = security_secmark_relabel_packet(&blob);
>  	if (err) {
>  		pr_info_ratelimited("unable to obtain relabeling permission\n");
>  		return err;
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index 77bb1bb22c3b..8948557eaebb 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -882,7 +882,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
>  	void *addr;
>  	void *mask;
>  	u32 addr_len;
> -	u32 secid;
> +	struct lsmblob blob;
>  	struct netlbl_audit audit_info;
>  
>  	/* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -906,13 +906,18 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
>  	ret_val = security_secctx_to_secid(
>  		                  nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
>  				  nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> -				  &secid);
> +				  &blob);
>  	if (ret_val != 0)
>  		return ret_val;
>  
> +	/* netlbl_unlhsh_add will be changed to pass a struct lsmblob *
> +	 * instead of a u32 later in this patch set. security_secctx_to_secid()
> +	 * will only be setting one entry in the lsmblob struct, so it is
> +	 * safe to use lsmblob_value() to get that one value. */
> +
>  	return netlbl_unlhsh_add(&init_net,
> -				 dev_name, addr, mask, addr_len, secid,
> -				 &audit_info);
> +				 dev_name, addr, mask, addr_len,
> +				 lsmblob_value(&blob), &audit_info);
>  }
>  
>  /**
> @@ -933,7 +938,7 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
>  	void *addr;
>  	void *mask;
>  	u32 addr_len;
> -	u32 secid;
> +	struct lsmblob blob;
>  	struct netlbl_audit audit_info;
>  
>  	/* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -955,13 +960,15 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
>  	ret_val = security_secctx_to_secid(
>  		                  nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
>  				  nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> -				  &secid);
> +				  &blob);
>  	if (ret_val != 0)
>  		return ret_val;
>  
> +	/* security_secctx_to_secid() will only put one secid into the lsmblob
> +	 * so it's safe to use lsmblob_value() to get the secid. */
>  	return netlbl_unlhsh_add(&init_net,
> -				 NULL, addr, mask, addr_len, secid,
> -				 &audit_info);
> +				 NULL, addr, mask, addr_len,
> +				 lsmblob_value(&blob), &audit_info);
>  }
>  
>  /**
> diff --git a/security/security.c b/security/security.c
> index c42873876954..5c2ed1db0658 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -2065,10 +2065,22 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
>  }
>  EXPORT_SYMBOL(security_secid_to_secctx);
>  
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> +			     struct lsmblob *blob)
>  {
> -	*secid = 0;
> -	return call_int_hook(secctx_to_secid, 0, secdata, seclen, secid);
> +	struct security_hook_list *hp;
> +	int rc;
> +
> +	lsmblob_init(blob, 0);
> +	hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> +		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +			continue;
> +		rc = hp->hook.secctx_to_secid(secdata, seclen,
> +					      &blob->secid[hp->lsmid->slot]);
> +		if (rc != 0)
> +			return rc;
> +	}
> +	return 0;
>  }
>  EXPORT_SYMBOL(security_secctx_to_secid);
>  
> @@ -2301,9 +2313,21 @@ void security_inet_conn_established(struct sock *sk,
>  }
>  EXPORT_SYMBOL(security_inet_conn_established);
>  
> -int security_secmark_relabel_packet(u32 secid)
> +int security_secmark_relabel_packet(struct lsmblob *blob)
>  {
> -	return call_int_hook(secmark_relabel_packet, 0, secid);
> +	struct security_hook_list *hp;
> +	int rc = 0;
> +
> +	hlist_for_each_entry(hp, &security_hook_heads.secmark_relabel_packet,
> +			     list) {
> +		if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> +			continue;
> +		rc = hp->hook.secmark_relabel_packet(
> +						blob->secid[hp->lsmid->slot]);
> +		if (rc != 0)
> +			break;
> +	}
> +	return rc;
>  }
>  EXPORT_SYMBOL(security_secmark_relabel_packet);
>  
> 

Powered by blists - more mailing lists