Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels

From: Christian Brauner <christian.brauner@ubuntu.com>
To: Jann Horn <jannh@google.com>
Cc: "Todd Kjos" <tkjos@google.com>,
	"Martijn Coenen" <maco@android.com>,
	"Arve Hjønnevåg" <arve@android.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"open list:ANDROID DRIVERS" <devel@driverdev.osuosl.org>,
	"kernel list" <linux-kernel@vger.kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
Subject: Re: UAF read in print_binder_transaction_log_entry() on ANDROID_BINDERFS kernels
Date: Mon, 7 Oct 2019 23:05:35 +0200	[thread overview]
Message-ID: <20191007210534.wqnizdp6pl7gn5qe@wittgenstein> (raw)
In-Reply-To: <CAG48ez14Q0-F8LqsvcNbyR2o6gPW8SHXsm4u5jmD9MpsteM2Tw@mail.gmail.com>

On Mon, Oct 07, 2019 at 10:49:57PM +0200, Jann Horn wrote:
> Hi!
> 
> There is a use-after-free read in print_binder_transaction_log_entry()
> on ANDROID_BINDERFS kernels because
> print_binder_transaction_log_entry() prints the char* e->context_name
> as string, and if the transaction occurred on a binder device from
> binderfs, e->context_name belongs to the binder device and is freed
> when the inode disappears.
> 
> Luckily this shouldn't have security implications, since:
> 
> a) reading the binder transaction log is already a pretty privileged operation
> b) I can't find any actual users of ANDROID_BINDERFS
> 
> I guess there are three ways to fix it:
> 1) Create a new shared global spinlock for binderfs_evict_inode() and
> binder_transaction_log_show(), and let binderfs_evict_inode() scan the
> transaction log for pointers to its name and replace them with
> pointers to a statically-allocated string "{DELETED}" or something
> like that.
> 2) Let the transaction log contain non-reusable device identifiers
> instead of name pointers, and let print_binder_transaction_log_entry()
> look them up in something like a hashtable.
> 3) Just copy the name into the transaction log every time.
> 
> I'm not sure which one is better, or whether there's a nicer fourth
> option, so I'm leaving writing a patch for this to y'all.

Moin,

Thanks for the report.
Android binderfs is enabled by default on Ubuntu and - iirc - Debian
kernels. It is actively used by Anbox to run Android in containers.

The codepath you're referring to is specific to the stats=global mount
option. This was contributed by the Android team for the 5.4 cycle (cf.
[1]). That means there is no released kernel that supports the
stats=global mount option. So all current users cannot be affected by
this bug.

I'll take a look at this tomorrow and see what makes the most sense. I
agree that this is not a security issue. Thanks for catching this early.

If you already have a script that trivially reproduces the bug it'd be
nice if you could paste it. Otherwise we can just add a reproducer based
on your snippet from below. I want to add a regression test for this.

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f00834518ed3194b866f5f3d63b71e0ed7f6bc00
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0e13e452dafc009049a9a5a4153e2f9e51b23915
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03e2e07e38147917482d595ad3cf193212ded8ac
     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4feb80faf428a02d407a9ea1952004af01308765

Thanks!
Christian