Message-ID: <20191009142129.GD143258@google.com>
Date:   Wed, 9 Oct 2019 10:21:29 -0400
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Christian Brauner <christian.brauner@...ntu.com>
Cc:     Todd Kjos <tkjos@...roid.com>, jannh@...gle.com, arve@...roid.com,
        christian@...uner.io, devel@...verdev.osuosl.org,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        maco@...roid.com, tkjos@...gle.com,
        Hridya Valsaraju <hridya@...gle.com>
Subject: Re: [PATCH] binder: prevent UAF read in
 print_binder_transaction_log_entry()

On Wed, Oct 09, 2019 at 12:40:12PM +0200, Christian Brauner wrote:
> On Tue, Oct 08, 2019 at 02:05:16PM -0400, Joel Fernandes wrote:
> > On Tue, Oct 08, 2019 at 03:01:59PM +0200, Christian Brauner wrote:
> > > When a binder transaction is initiated on a binder device coming from a
> > > binderfs instance, a pointer to the name of the binder device is stashed
> > > in the binder_transaction_log_entry's context_name member. Later on it
> > > is used to print the name in print_binder_transaction_log_entry(). By
> > > the time print_binder_transaction_log_entry() accesses context_name
> > > binderfs_evict_inode() might have already freed the associated memory
> > > thereby causing a UAF. Do the simple thing and prevent this by copying
> > > the name of the binder device instead of stashing a pointer to it.
> > > 
> > > Reported-by: Jann Horn <jannh@...gle.com>
> > > Fixes: 03e2e07e3814 ("binder: Make transaction_log available in binderfs")
> > > Link: https://lore.kernel.org/r/CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com
> > > Cc: Joel Fernandes <joel@...lfernandes.org>
> > > Cc: Todd Kjos <tkjos@...roid.com>
> > > Cc: Hridya Valsaraju <hridya@...gle.com>
> > > Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
> > > ---
> > >  drivers/android/binder.c          | 4 +++-
> > >  drivers/android/binder_internal.h | 2 +-
> > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index c0a491277aca..5b9ac2122e89 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -57,6 +57,7 @@
> > >  #include <linux/sched/signal.h>
> > >  #include <linux/sched/mm.h>
> > >  #include <linux/seq_file.h>
> > > +#include <linux/string.h>
> > >  #include <linux/uaccess.h>
> > >  #include <linux/pid_namespace.h>
> > >  #include <linux/security.h>
> > > @@ -66,6 +67,7 @@
> > >  #include <linux/task_work.h>
> > >  
> > >  #include <uapi/linux/android/binder.h>
> > > +#include <uapi/linux/android/binderfs.h>
> > >  
> > >  #include <asm/cacheflush.h>
> > >  
> > > @@ -2876,7 +2878,7 @@ static void binder_transaction(struct binder_proc *proc,
> > >  	e->target_handle = tr->target.handle;
> > >  	e->data_size = tr->data_size;
> > >  	e->offsets_size = tr->offsets_size;
> > > -	e->context_name = proc->context->name;
> > > +	strscpy(e->context_name, proc->context->name, BINDERFS_MAX_NAME);
> > 
> > Strictly speaking, proc-context->name can also be initialized for !BINDERFS
> > so the BINDERFS in the MAX_NAME macro is misleading. So probably there should
> > be a BINDER_MAX_NAME (and associated checks for whether non BINDERFS names
> > fit within the MAX.
> 
> I know but I don't think it's worth special-casing non-binderfs devices.
> First, non-binderfs devices can only be created through a KCONFIG option
> determined at compile time. For stock Android the names are the same for
> all vendors afaik.

I am just talking about the name of weirdly named macro here.

> Second, BINDERFS_MAX_NAME is set to the maximum path name component
> length that nearly all filesystems support (256 chars). If you exceed
> that then you run afoul of a bunch of other assumptions already and will
> cause trouble.

Again, just talking about the name.

> Third, even if there is someone crazy and uses more than 256 chars for a
> non-binderfs device at KCONFIG time strscpy will do the right thing and
> truncate and you'd see a truncated binder device name. This doesn't seem
> to be a big deal for a debugfs interface.

Sure I never said the patch has a bug.

> Fourth, the check for non-binderfs devices technically has nothing to do
> with this patch. This patch should really just do the minimal thing and
> fix the UAF. Which it does.

Again, never said the patch is buggy.

> Fifth, I already tried to push for validation of non-binderfs binder
> devices a while back when I wrote binderfs and was told that it's not
> needed. Hrydia tried the same and we decided the same thing. So you get
> to be the next person to send a patch. :)

I don't follow why we are talking about non-binderfs validation. I am just
saying a memcpy of the name could have been avoided for regular binder
devices. But since Todd Acked it, I wont stand in the way..

> > One more thought, this can be made dependent on CONFIG_BINDERFS since regular
> > binder devices cannot be unregistered AFAICS and as Jann said, the problem is
> > BINDERFS specific. That way we avoid the memcpy for _every_ transaction.
> > These can be thundering when Android starts up.
> 
> Unless Todd sees this as a real performance problem I'm weary to
> introduce additional checking and record a pointer for non-binderfs and
> a memcpy() for binderfs devices. :)

Ok.

> > (I secretly wish C strings could be refcounted to avoid exactly this issue,
> > that should not be hard to develop but I am not sure if it is worth it for
> > this problem :) - For one, it will avoid having to do the strcpy for _every_
> > transaction).
> > 
> > Other than these nits, please add my tag on whichever is the final solution:
> > 
> > Reviewed-by: Joel Fernandes (Google) <joel@...lfernandes.org>
> 
> Thanks for the review, Joel. :)

My duty!! ;-)

thanks,

 - Joel

Powered by blists - more mailing lists