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

From: Kees Cook <keescook@chromium.org>
To: Casey Schaufler <casey@schaufler-ca.com>
Cc: casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
	john.johansen@canonical.com, penguin-kernel@i-love.sakura.ne.jp,
	paul@paul-moore.com, sds@tycho.nsa.gov
Subject: Re: [PATCH v4 17/23] LSM: Use lsmcontext in security_secid_to_secctx
Date: Thu, 27 Jun 2019 10:17:10 -0700	[thread overview]
Message-ID: <201906271007.4248A3AE@keescook> (raw)
In-Reply-To: <cacf9eda-7486-80d3-f47d-b0dbb6872091@schaufler-ca.com>

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?)

@@ -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.

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!

-- 
Kees Cook
