netdev - Re: [PATCH v17 05/23] net: Prepare UDS for security module stacking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEjxPJ4TExFpm0KJSodLSEG0J+YNYBE4KdKyd=1g-Qs-qgPHpA@mail.gmail.com>
Date:   Mon, 18 May 2020 13:02:23 -0400
From:   Stephen Smalley <stephen.smalley.work@...il.com>
To:     Casey Schaufler <casey@...aufler-ca.com>
Cc:     casey.schaufler@...el.com, James Morris <jmorris@...ei.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>,
        Kees Cook <keescook@...omium.org>,
        John Johansen <john.johansen@...onical.com>,
        penguin-kernel@...ove.sakura.ne.jp,
        Paul Moore <paul@...l-moore.com>,
        Stephen Smalley <sds@...ho.nsa.gov>, linux-audit@...hat.com,
        netdev@...r.kernel.org
Subject: Re: [PATCH v17 05/23] net: Prepare UDS for security module stacking

On Thu, May 14, 2020 at 7:25 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>
> Change the data used in UDS SO_PEERSEC processing from a
> secid to a more general struct lsmblob. Update the
> security_socket_getpeersec_dgram() interface to use the
> lsmblob. There is a small amount of scaffolding code
> that will come out when the security_secid_to_secctx()
> code is brought in line with the lsmblob.
>
> The secid field of the unix_skb_parms structure has been
> replaced with a pointer to an lsmblob structure, and the
> lsmblob is allocated as needed. This is similar to how the
> list of passed files is managed. While an lsmblob structure
> will fit in the available space today, there is no guarantee
> that the addition of other data to the unix_skb_parms or
> support for additional security modules wouldn't exceed what
> is available.

I preferred the previous approach (in v15 and earlier) but I see that
this was suggested by Paul.  Lifecycle management of lsmdata seems
rather tenuous. I guess the real question is what does netdev prefer.
Regardless, you need to check for memory allocation failure below if
this approach stands.

> Reviewed-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
> cc: netdev@...r.kernel.org
> ---

> diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
> index 3385a7a0b231..a5c1a029095d 100644
> --- a/net/unix/af_unix.c
> +++ b/net/unix/af_unix.c
> @@ -138,17 +138,18 @@ static struct hlist_head *unix_sockets_unbound(void *addr)
>  #ifdef CONFIG_SECURITY_NETWORK
>  static void unix_get_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       UNIXCB(skb).secid = scm->secid;
> +       UNIXCB(skb).lsmdata = kmemdup(&scm->lsmblob, sizeof(scm->lsmblob),
> +                                     GFP_KERNEL);
>  }

Somewhere you need to check for and handle kmemdup() failure here.

>
>  static inline void unix_set_secdata(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       scm->secid = UNIXCB(skb).secid;
> +       scm->lsmblob = *(UNIXCB(skb).lsmdata);
>  }

Lest we have a bad day here.

>
>  static inline bool unix_secdata_eq(struct scm_cookie *scm, struct sk_buff *skb)
>  {
> -       return (scm->secid == UNIXCB(skb).secid);
> +       return lsmblob_equal(&scm->lsmblob, UNIXCB(skb).lsmdata);
>  }

Or here.

> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index 8c40f2b32392..3094323935a4 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -142,6 +142,12 @@ void unix_destruct_scm(struct sk_buff *skb)
>         scm.pid  = UNIXCB(skb).pid;
>         if (UNIXCB(skb).fp)
>                 unix_detach_fds(&scm, skb);
> +#ifdef CONFIG_SECURITY_NETWORK
> +       if (UNIXCB(skb).lsmdata) {
> +               kfree(UNIXCB(skb).lsmdata);
> +               UNIXCB(skb).lsmdata = NULL;
> +       }
> +#endif

Does this suffice to ensure that lsmdata is always freed?  Seems
weakly connected to the allocation.