Re: [PATCH v15 06/23] Use lsmblob in security_secctx_to_secid

[Paul Moore <paul@paul-moore.com>]

On Fri, Feb 14, 2020 at 6:43 PM Casey Schaufler <casey@schaufler-ca.com> 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. In some cases there is scaffolding where other
> interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/security.h          |  5 +++--
>  kernel/cred.c                     |  4 +---
>  net/netfilter/nft_meta.c          | 12 +++++++-----
>  net/netfilter/xt_SECMARK.c        |  5 ++++-
>  net/netlabel/netlabel_unlabeled.c | 14 ++++++++------
>  security/security.c               | 18 +++++++++++++++---
>  6 files changed, 38 insertions(+), 20 deletions(-)

...

> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 951b6e87ed5d..e12125b85035 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -811,21 +811,23 @@ 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);
> +       /* Using le[0] is scaffolding */
> +       err = security_secmark_relabel_packet(blob.secid[0]);
>         if (err)
>                 return err;

At the very least it looks like the comment above needs an update.
However, I would really like to see an explanation in this patch,
either in the comments or in the commit description, about how you
plan to handle secmarks.  If your plan is to always have it be the
first LSM, let's admit that and document it appropriately.  If there
is something much grander coming later in the patchset I guess
"scaffolding" is an okay term, but it would be good to mention in the
commit description that this will be replaced with something better
later in the patchset.

I'm worried about the case five years from know when we are changing
this code, either due to bugs or new features, and we stumble across
this commit.  Was it always intended to be this way?  Or was this
temporary?  Right now I don't know.

> -       priv->secid = tmp_secid;
> +       /* Using le[0] is scaffolding */
> +       priv->secid = blob.secid[0];
>         return 0;
>  }

...

> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index d2e4ab8d1cb1..7a5a87f15736 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -881,7 +881,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
> @@ -905,12 +905,13 @@ 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;
>
> +       /* scaffolding with the [0] */
>         return netlbl_unlhsh_add(&init_net,
> -                                dev_name, addr, mask, addr_len, secid,
> +                                dev_name, addr, mask, addr_len, blob.secid[0],
>                                  &audit_info);
>  }

Same as above, although this time with the peer label.

-- 
paul moore
www.paul-moore.com