Re: [RFC PATCH 1/1] selinux-testsuite: Add binder tests

[Richard Haines <richard_c_haines@btinternet.com>]

On Tue, 2018-05-15 at 13:45 -0400, Stephen Smalley wrote:
> On 05/15/2018 01:34 PM, Richard Haines wrote:
> > On Tue, 2018-05-15 at 12:56 -0400, Stephen Smalley wrote:
> > > On 05/15/2018 12:38 PM, Stephen Smalley wrote:
> > > > On 05/15/2018 09:43 AM, Stephen Smalley wrote:
> > > > > On 05/15/2018 09:36 AM, Stephen Smalley wrote:
> > > > > > This test is failing for me (with or without -v):
> > > > > > # ./test -v
> > > > > > 1..6
> > > > > > Manager PID: 5608 Process context:
> > > > > > 	unconfined_u:unconfined_r:test_binder_mgr_t:s0-
> > > > > > s0:c0.c1023
> > > > > > Client PID: 5609 Process context:
> > > > > > 	unconfined_u:unconfined_r:test_binder_client_t:s0-
> > > > > > s0:c0.c1023
> > > > > > Client read_consumed: 28
> > > > > > Manager read_consumed: 72
> > > > > > Client command: BR_NOOP
> > > > > > Manager command: BR_NOOP
> > > > > > Client command: BR_INCREFS
> > > > > > Manager command: BR_TRANSACTION
> > > > > > Client command: BR_TRANSACTION_COMPLETE
> > > > > > BR_TRANSACTION data:
> > > > > > 	handle: 0
> > > > > > 	cookie: 0
> > > > > > 	code: 0
> > > > > > 	flag: TF_ACCEPT_FDS
> > > > > > 	sender pid: 5609
> > > > > > 	sender euid: 0
> > > > > > 	data_size: 24
> > > > > > 	offsets_size: 8
> > > > > > Sending BC_REPLY
> > > > > > Manager read_consumed: 8
> > > > > > Manager command: BR_NOOP
> > > > > > Manager command: BR_TRANSACTION_COMPLETE
> > > > > > Client read_consumed: 72
> > > > > > Client command: BR_NOOP
> > > > > > Client command: BR_REPLY
> > > > > > BR_REPLY data:
> > > > > > 	handle: 0
> > > > > > 	cookie: 0
> > > > > > 	code: 0
> > > > > > 	flag: TF_ACCEPT_FDS
> > > > > > 	sender pid: 0
> > > > > > 	sender euid: 0
> > > > > > 	data_size: 24
> > > > > > 	offsets_size: 8
> > > > > > Retrieved Managers fd: 4 st_dev: 6
> > > > > > Client read_consumed: 8
> > > > > > Client using Managers FD command: BR_NOOP
> > > > > > Client using Managers FD command: BR_FAILED_REPLY
> > > > > > Client using Managers received FD failed response
> > > > > > Manager read_consumed: 4
> > > > > > Manager command: BR_NOOP
> > > > > > not ok 1
> > > > > > #   Failed test at ./test line 36.
> > > > > 
> > > > > Just realized that I was testing with a kernel that still had
> > > > > Casey's stacking support enabled.
> > > > > Will re-try without that.
> > > > 
> > > > Still fails for me on F28 with stock/linus 4.17.0-rc5.  No AVC
> > > > messages from the failing test itself,
> > > > just the other ones.
> > > 
> > > Traced the client and saw that it was getting BR_FAILED_REPLY
> > > from
> > > the kernel.
> > > Looked at /sys/kernel/debug/binder/failed_transaction_log and saw
> > > that the failure
> > > occurs on line 2847.  Did a git blame on that line and found this
> > > commit,
> > > 
> > > commit 7aa135fcf26377f92dc0680a57566b4c7f3e281b
> > > Author: Martijn Coenen <maco@android.com>
> > > Date:   Wed Mar 28 11:14:50 2018 +0200
> > > 
> > >     ANDROID: binder: prevent transactions into own process.
> > >     
> > >     This can't happen with normal nodes (because you can't get a
> > > ref
> > >     to a node you own), but it could happen with the context
> > > manager;
> > >     to make the behavior consistent with regular nodes, reject
> > >     transactions into the context manager by the process owning
> > > it.
> > >     
> > >     Reported-by: syzbot+09e05aba06723a94d43d@syzkaller.appspotmai
> > > l.co
> > > m
> > >     Signed-off-by: Martijn Coenen <maco@android.com>
> > >     Cc: stable <stable@vger.kernel.org>
> > >     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org
> > > >
> > > 
> > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > > index 764b63a5aade..e578eee31589 100644
> > > --- a/drivers/android/binder.c
> > > +++ b/drivers/android/binder.c
> > > @@ -2839,6 +2839,14 @@ static void binder_transaction(struct
> > > binder_proc *proc,
> > >                         else
> > >                                 return_error = BR_DEAD_REPLY;
> > >                         mutex_unlock(&context-
> > > > context_mgr_node_lock);
> > > 
> > > +                       if (target_node && target_proc == proc) {
> > > +                               binder_user_error("%d:%d got
> > > transaction to context manager from process owning it\n",
> > > +                                                 proc->pid,
> > > thread-
> > > > pid);
> > > 
> > > +                               return_error = BR_FAILED_REPLY;
> > > +                               return_error_param = -EINVAL;
> > > +                               return_error_line = __LINE__;
> > > +                               goto err_invalid_target_handle;
> > > +                       }
> > >                 }
> > >                 if (!target_node) {
> > >                         /*
> > > 
> > > 
> > > So that's a change in kernel behavior in v4.17-rc3 and later that
> > > breaks your test code.
> > 
> > Thanks for the info. I'll get a new kernel and see how far I can
> > get.
> 
> Simplest fix is to just not do the impersonation test, i.e. don't
> pass the
> context manager's fd to the client at all.

Thanks for the advice, I'll do the simplest option for now and save the
other for a rainy day (probably longer !!)

> 
> Alternatively, you could introduce a third process ("server"), have
> it register
> a binder with the context manager, have the client get a reference to
> it from
> the context manager, have the client receive a ref to either the
> manager's or
> the server's binder fd via binder, and have the client try to use
> that in a call
> to the other one.  But that's a lot more complexity in your test.