linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: John Johansen <john.johansen@canonical.com>
To: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: linux-audit@redhat.com, keescook@chromium.org,
	penguin-kernel@i-love.sakura.ne.jp, paul@paul-moore.com,
	sds@tycho.nsa.gov, netdev@vger.kernel.org
Subject: Re: [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid
Date: Tue, 28 Jul 2020 04:11:27 -0700	[thread overview]
Message-ID: <bebdacdf-2ecb-8e07-1b0e-6e6bfb5960d0@canonical.com> (raw)
In-Reply-To: <20200724203226.16374-7-casey@schaufler-ca.com>

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@chromium.org>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: netdev@vger.kernel.org

one comment below, but its a nice to have so

Reviewed-by: John Johansen <john.johansen@canonical.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);
>  
> 


  reply	other threads:[~2020-07-28 20:29 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20200724203226.16374-1-casey.ref@schaufler-ca.com>
2020-07-24 20:32 ` [PATCH v19 00/23] LSM: Module stacking for AppArmor Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 01/23] LSM: Infrastructure management of the sock security Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 02/23] LSM: Create and manage the lsmblob data structure Casey Schaufler
2020-07-27 16:12     ` Stephen Smalley
2020-07-27 21:04       ` Casey Schaufler
2020-07-28 19:50     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 03/23] LSM: Use lsmblob in security_audit_rule_match Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 04/23] LSM: Use lsmblob in security_kernel_act_as Casey Schaufler
2020-07-28 10:34     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 05/23] net: Prepare UDS for security module stacking Casey Schaufler
2020-07-28 10:57     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 06/23] LSM: Use lsmblob in security_secctx_to_secid Casey Schaufler
2020-07-28 11:11     ` John Johansen [this message]
2020-07-28 23:41       ` Casey Schaufler
2020-07-29  0:30         ` John Johansen
2020-07-24 20:32   ` [PATCH v19 07/23] LSM: Use lsmblob in security_secid_to_secctx Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 08/23] LSM: Use lsmblob in security_ipc_getsecid Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 09/23] LSM: Use lsmblob in security_task_getsecid Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 10/23] LSM: Use lsmblob in security_inode_getsecid Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 11/23] LSM: Use lsmblob in security_cred_getsecid Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 12/23] IMA: Change internal interfaces to use lsmblobs Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 13/23] LSM: Specify which LSM to display Casey Schaufler
2020-07-27 20:36     ` James Morris
2020-07-27 20:40       ` John Johansen
2020-07-28 18:29     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 14/23] LSM: Ensure the correct LSM context releaser Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 15/23] LSM: Use lsmcontext in security_secid_to_secctx Casey Schaufler
2020-07-28 20:13     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 16/23] LSM: Use lsmcontext in security_inode_getsecctx Casey Schaufler
2020-07-28 20:28     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 17/23] LSM: security_secid_to_secctx in netlink netfilter Casey Schaufler
2020-07-27 20:37     ` James Morris
2020-07-24 20:32   ` [PATCH v19 18/23] NET: Store LSM netlabel data in a lsmblob Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 19/23] LSM: Verify LSM display sanity in binder Casey Schaufler
2020-07-30  8:40     ` John Johansen
2020-07-24 20:32   ` [PATCH v19 20/23] Audit: Add new record for multiple process LSM attributes Casey Schaufler
2020-07-27 19:04     ` Stephen Smalley
2020-07-24 20:32   ` [PATCH v19 21/23] Audit: Add a new record for multiple object " Casey Schaufler
2020-07-27 20:40     ` James Morris
2020-07-24 20:32   ` [PATCH v19 22/23] LSM: Add /proc attr entry for full LSM context Casey Schaufler
2020-07-30 10:03     ` John Johansen
2020-07-30 20:44       ` Casey Schaufler
2020-07-30 20:57         ` John Johansen
2020-07-30 22:22           ` Casey Schaufler
2020-07-24 20:32   ` [PATCH v19 23/23] AppArmor: Remove the exclusive flag Casey Schaufler
2020-07-30  9:23     ` John Johansen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bebdacdf-2ecb-8e07-1b0e-6e6bfb5960d0@canonical.com \
    --to=john.johansen@canonical.com \
    --cc=casey.schaufler@intel.com \
    --cc=casey@schaufler-ca.com \
    --cc=jmorris@namei.org \
    --cc=keescook@chromium.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-security-module@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).