Re: [PATCH v4 17/23] LSM: Use lsmcontext in security_secid_to_secctx

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/27/2019 10:17 AM, Kees Cook wrote:
> On Thu, Jun 27, 2019 at 09:29:25AM -0700, Casey Schaufler wrote:
>> On 6/26/2019 8:53 PM, Kees Cook wrote:
>>> On Wed, Jun 26, 2019 at 12:22:28PM -0700, Casey Schaufler wrote:
>>>> Replace the (secctx,seclen) pointer pair with a single
>>>> lsmcontext pointer to allow return of the LSM identifier
>>>> along with the context and context length. This allows
>>>> security_release_secctx() to know how to release the
>>>> context. Callers have been modified to use or save the
>>>> returned data from the new structure.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> ---
>>>>  drivers/android/binder.c                | 24 ++++++---------
>>>>  include/linux/security.h                |  4 +--
>>>>  include/net/scm.h                       |  9 ++----
>>>>  kernel/audit.c                          | 29 +++++++-----------
>>>>  kernel/auditsc.c                        | 31 +++++++------------
>>>>  net/ipv4/ip_sockglue.c                  |  7 ++---
>>>>  net/netfilter/nf_conntrack_netlink.c    | 14 +++++----
>>>>  net/netfilter/nf_conntrack_standalone.c |  7 ++---
>>>>  net/netfilter/nfnetlink_queue.c         |  5 +++-
>>>>  net/netlabel/netlabel_unlabeled.c       | 40 ++++++++-----------------
>>>>  net/netlabel/netlabel_user.c            |  7 ++---
>>>>  security/security.c                     |  9 ++++--
>>>>  12 files changed, 72 insertions(+), 114 deletions(-)
>>>>
>>>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>>>> index 89e574be34cc..5d417a7b9bb3 100644
>>>> --- a/drivers/android/binder.c
>>>> +++ b/drivers/android/binder.c
>>>> @@ -2874,9 +2874,7 @@ static void binder_transaction(struct binder_proc *proc,
>>>>  	binder_size_t last_fixup_min_off = 0;
>>>>  	struct binder_context *context = proc->context;
>>>>  	int t_debug_id = atomic_inc_return(&binder_last_id);
>>>> -	char *secctx = NULL;
>>>> -	u32 secctx_sz = 0;
>>>> -	struct lsmcontext scaff; /* scaffolding */
>>>> +	struct lsmcontext lsmctx;
>>> As James found, this needs to be zero initialized:
>>>
>>> struct lsmcontext lsmctx = { };
>> Thanks! I'll incorporate this in v5. It's great to
>> have y'all checking it out.
> I looked through other removed NULL assignments, and I think I see some
> other issues...
>
>                 binder_alloc_copy_to_buffer(&target_proc->alloc,
>                                             t->buffer, buf_offset,
> -                                           secctx, secctx_sz);
> -               security_release_secctx(secctx, secctx_sz);
> -               secctx = NULL;
> +                                           lsmctx.context, lsmctx.len);
> +               security_release_secctx(&lsmctx);
>
> The new security_release_secctx() performs the zeroing if there is a
> slot match... should it be unconditional? (And should the no-op version
> also zero?)

I don't see a reason not to zero in all cases. I'll change that.

>
> @@ -2420,8 +2420,7 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct
> svc_fh *fhp,
>         __be32 status;
>         int err;
>         struct nfs4_acl *acl = NULL;
> -       void *context = NULL;
> -       int contextlen;
> +       struct lsmcontext context;
>
> This needs the same = { } to retain the same meaning.

I'm going to have security_secid_to_secctx() initialize
the lsmcontext. There's no case where it should be used
to update an existing context, and it makes more sense to
do it in one place than to make all the callers worry
about it.

> And here:
>
> @@ -1191,8 +1191,8 @@ static int audit_receive_msg(struct sk_buff *skb,
> struct nlmsghdr *nlh)
>         struct audit_buffer     *ab;
>         u16                     msg_type = nlh->nlmsg_type;
>         struct audit_sig_info   *sig_data;
> -       char                    *ctx = NULL;
>         u32                     len;
> +       struct lsmcontext       context;
>
> And here:
>
> @@ -2069,24 +2070,23 @@ void audit_log_key(struct audit_buffer *ab, char
> *key)
>  
>  int audit_log_task_context(struct audit_buffer *ab)
>  {
> -       char *ctx = NULL;
> -       unsigned len;
>         int error;
> -       u32 sid;
> +       struct lsmblob le;
> +       struct lsmcontext context;
>
> and here:
>
>  static int audit_log_pid_context(struct audit_context *context, pid_t
> pid,
> -                                kuid_t auid, kuid_t uid, unsigned int
>                                  sessionid,
> -                                u32 sid, char *comm)
> +                                kuid_t auid, kuid_t uid,
> +                                unsigned int sessionid,
> +                                struct lsmblob *l, char *comm)
>  {
>         struct audit_buffer *ab;
> -       char *ctx = NULL;
> -       u32 len;
> +       struct lsmcontext lsmctx;
>
> Maybe here?
>
> -               if (osid) {
> -                       char *ctx = NULL;
> -                       u32 len;
> -                       if (security_secid_to_secctx(osid, &ctx, &len))
>                         {
> -                               audit_log_format(ab, " osid=%u", osid);
> +               if (lsmblob_is_set(olsm)) {
> +                       struct lsmcontext lsmcxt;
> +                       if (security_secid_to_secctx(olsm, &lsmcxt))
>                                 *call_panic = 1;
>
> and:
>
> -       if (n->osid != 0) {
> -               char *ctx = NULL;
> -               u32 len;
> +       if (lsmblob_is_set(&n->olsm)) {
> +               struct lsmcontext lsmctx;
>  
>
> and:
>
> @@ -395,7 +401,7 @@ nfqnl_build_packet_message(struct net *net, struct
> nfqnl_instance *queue,
>         enum ip_conntrack_info uninitialized_var(ctinfo);
>         struct nfnl_ct_hook *nfnl_ct;
>         bool csum_verify;
> -       char *secdata = NULL;
> +       struct lsmcontext context;
>
> and:
>
> @@ -387,8 +387,7 @@ int netlbl_unlhsh_add(struct net *net,
>         struct net_device *dev;
>         struct netlbl_unlhsh_iface *iface;
>         struct audit_buffer *audit_buf = NULL;
> -       char *secctx = NULL;
> -       u32 secctx_len;
> +       struct lsmcontext context;
>
>
> Sorry I forgot to check those the first time through!
>