[PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[Josh Max]

This patch allows binfmt_misc to select the interpeter for arbitrary
binaries by comparing a specified registered keyword with the value
of a specified binary's extended attribute (user.binfmt.interp),
and then launching the program with the registered interpreter.

This is useful when wanting to launch a collection of binaries under
the same interpreter, even when they do not necessarily share a common
extension or magic bits, or when their magic conflics with the operation
of binfmt_elf. Some examples of its use would be to launch some executables
of various different architectures in a directory, or for running some
native binaries under a sandbox (like firejail) automatically during their
launch.

Signed-off-by: Josh Max <JMax@mail.greenriver.edu>
---
 fs/binfmt_misc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index 6103a63..86d93c7 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -24,6 +24,7 @@
 #include <linux/mount.h>
 #include <linux/syscalls.h>
 #include <linux/fs.h>
+#include <linux/xattr.h>
 #include <linux/uaccess.h>
 
 #include "internal.h"
@@ -41,12 +42,17 @@ enum {
 static LIST_HEAD(entries);
 static int enabled = 1;
 
-enum {Enabled, Magic};
+enum {Enabled, Magic, Keyword};
 #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
 #define MISC_FMT_OPEN_BINARY (1 << 30)
 #define MISC_FMT_CREDENTIALS (1 << 29)
 #define MISC_FMT_OPEN_FILE (1 << 28)
 
+#define XATTR_BINFMT_PREFIX XATTR_USER_PREFIX "binfmt."
+#define XATTR_BINFMT_INTERPRETER_SUFFIX "interp"
+#define XATTR_NAME_BINFMT (XATTR_BINFMT_PREFIX XATTR_BINFMT_INTERPRETER_SUFFIX)
+#define XATTR_VALUE_MAX_LENGTH 128
+
 typedef struct {
 	struct list_head list;
 	unsigned long flags;		/* type, status, etc. */
@@ -61,6 +67,7 @@ typedef struct {
 } Node;
 
 static DEFINE_RWLOCK(entries_lock);
+static int get_xattr_interp_keyword(struct file *file, char *buf, size_t count);
 static struct file_system_type bm_fs_type;
 static struct vfsmount *bm_mnt;
 static int entry_count;
@@ -87,6 +94,8 @@ static int entry_count;
  */
 static Node *check_file(struct linux_binprm *bprm)
 {
+	char k[XATTR_VALUE_MAX_LENGTH];
+	int k_len = get_xattr_interp_keyword(bprm->file, k, sizeof(k)-1);
 	char *p = strrchr(bprm->interp, '.');
 	struct list_head *l;
 
@@ -100,6 +109,16 @@ static Node *check_file(struct linux_binprm *bprm)
 		if (!test_bit(Enabled, &e->flags))
 			continue;
 
+		/* Do matching based on xattrs keyword */
+		if (test_bit(Keyword, &e->flags)) {
+			if (k_len <= 0)
+				continue;
+			k[k_len] = 0;
+			if (!strcmp(e->magic, k))
+				return e;
+			continue;
+		}
+
 		/* Do matching based on extension if applicable. */
 		if (!test_bit(Magic, &e->flags)) {
 			if (p && !strcmp(e->magic, p + 1))
@@ -309,6 +328,20 @@ static char *check_special_flags(char *sfs, Node *e)
 }
 
 /*
+ * Check to see if the filesystem supports xattrs
+ * and grab the value so it can be checked against
+ * the list of keywords in binfmt_misc for a match
+ */
+static int get_xattr_interp_keyword(struct file *file, char *buf, size_t count)
+{
+
+	if (unlikely(!file->f_inode->i_op->getxattr))
+		return -ENOENT;
+	return file->f_inode->i_op->getxattr(file->f_path.dentry, file->f_inode,
+		XATTR_NAME_BINFMT, buf, count);
+}
+
+/*
  * This registers a new binary format, it recognises the syntax
  * ':name:type:offset:magic:mask:interpreter:flags'
  * where the ':' is the IFS, that can be chosen with the first char
@@ -366,6 +399,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		pr_debug("register: type: E (extension)\n");
 		e->flags = 1 << Enabled;
 		break;
+	case 'K':
+		pr_debug("register: type: K (xattrs keyword)\n");
+		e->flags = (1 << Enabled) | (1 << Keyword);
+		break;
 	case 'M':
 		pr_debug("register: type: M (magic)\n");
 		e->flags = (1 << Enabled) | (1 << Magic);
@@ -453,7 +490,7 @@ static Node *create_entry(const char __user *buffer, size_t count)
 			}
 		}
 	} else {
-		/* Handle the 'E' (extension) format. */
+		/* Handle the 'E' (extension) and 'K' (keyword) format. */
 
 		/* Skip the 'offset' field. */
 		p = strchr(p, del);
@@ -469,7 +506,10 @@ static Node *create_entry(const char __user *buffer, size_t count)
 		*p++ = '\0';
 		if (!e->magic[0] || strchr(e->magic, '/'))
 			goto einval;
-		pr_debug("register: extension: {%s}\n", e->magic);
+		if (test_bit(Keyword, &e->flags))
+			pr_debug("register: keyword: {%s}\n", e->magic);
+		else
+			pr_debug("register: extension: {%s}\n", e->magic);
 
 		/* Skip the 'mask' field. */
 		p = strchr(p, del);
@@ -563,7 +603,10 @@ static void entry_status(Node *e, char *page)
 	*dp++ = '\n';
 
 	if (!test_bit(Magic, &e->flags)) {
-		sprintf(dp, "extension .%s\n", e->magic);
+		if (test_bit(Keyword, &e->flags))
+			sprintf(dp, "keyword %s\n", e->magic);
+		else
+			sprintf(dp, "extension .%s\n", e->magic);
 	} else {
 		dp += sprintf(dp, "offset %i\nmagic ", e->offset);
 		dp = bin2hex(dp, e->magic, e->size);
-- 
2.8.1

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[James Bottomley]

On Sun, 2016-08-21 at 21:01 -0700, Josh Max wrote:
> This patch allows binfmt_misc to select the interpeter for arbitrary
> binaries by comparing a specified registered keyword with the value
> of a specified binary's extended attribute (user.binfmt.interp),
> and then launching the program with the registered interpreter.
> 
> This is useful when wanting to launch a collection of binaries under
> the same interpreter, even when they do not necessarily share a 
> common extension or magic bits, or when their magic conflics with the
> operation of binfmt_elf. Some examples of its use would be to launch 
> some executables of various different architectures in a directory, 
> or for running some native binaries under a sandbox (like firejail) 
> automatically during their launch.

Could you expand on the use cases?  The patch set looks OK; the issue
with extended attributes is lack of universal support on filesystems,
but that may not be a problem because they're definitely supported on
all the standard ones.  I think the current F flag solves the foreign
binary in chroot or container.  Self sandboxing sounds reasonable, but
if this is a security feature, doesn't having the label under the user.
EAs mean that the confined binary can simply remove the label and
unconfine itself?

James

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[Josh Max]

On Thu, 25 Aug 2016 16:15:40 +0000, James Bottomley wrote: 
> Could you expand on the use cases?  The patch set looks OK; the issue
> with extended attributes is lack of universal support on filesystems,
> but that may not be a problem because they're definitely supported on
> all the standard ones.  I think the current F flag solves the foreign
> binary in chroot or container.  Self sandboxing sounds reasonable, but
> if this is a security feature, doesn't having the label under the user.
> EAs mean that the confined binary can simply remove the label and
> unconfine itself?

Regarding sandboxing, my intent on this patch was to sandbox "trustworthy"
binaries (e.g. Apache, ssh, _insert_web_browser_here_, etc.) to reduce their
attack surface, rather than reduce the chances of a malicious process
compromising the system (without the need of maintaining a bunch of wrapper
scripts to launch said binaries under a sandbox). As such I'm using this patch
to sandbox Android's "app_process" service, as well as my personal web
browser. Of couse, it also has other benefits as well. For instance, an
observatory control software I use has a master daemon and several dome
control drivers, which are also native executables. Normally, you would need
to provide the correct driver as a parameter to the daemon, but with this patch
simply loading the driver also loads the daemon. I know of a few other
applications that do something similar to this that could benefit from xattr-based
interpreter selection.

Josh

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[Florian Weimer]

On 08/25/2016 06:15 PM, James Bottomley wrote:
> On Sun, 2016-08-21 at 21:01 -0700, Josh Max wrote:
>> This patch allows binfmt_misc to select the interpeter for arbitrary
>> binaries by comparing a specified registered keyword with the value
>> of a specified binary's extended attribute (user.binfmt.interp),
>> and then launching the program with the registered interpreter.
>>
>> This is useful when wanting to launch a collection of binaries under
>> the same interpreter, even when they do not necessarily share a
>> common extension or magic bits, or when their magic conflics with the
>> operation of binfmt_elf. Some examples of its use would be to launch
>> some executables of various different architectures in a directory,
>> or for running some native binaries under a sandbox (like firejail)
>> automatically during their launch.
>
> Could you expand on the use cases?

A non-security use case would be to run the binary (without 
modification) with a different ELF interpreter (assuming this allows to 
override binfmt_elf, but self-sandboxing would need that as well).  This 
would make it easier to use older or newer libcs for select binaries on 
the system.  Right now, one has to write wrappers for that, and the 
explicit dynamic linker invocation is not completely transparent to the 
application.

Florian

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[Carlos O'Donell]

On 08/26/2016 10:55 AM, Florian Weimer wrote:
> On 08/25/2016 06:15 PM, James Bottomley wrote:
>> On Sun, 2016-08-21 at 21:01 -0700, Josh Max wrote:
>>> This patch allows binfmt_misc to select the interpeter for
>>> arbitrary binaries by comparing a specified registered keyword
>>> with the value of a specified binary's extended attribute
>>> (user.binfmt.interp), and then launching the program with the
>>> registered interpreter.
>>> 
>>> This is useful when wanting to launch a collection of binaries
>>> under the same interpreter, even when they do not necessarily
>>> share a common extension or magic bits, or when their magic
>>> conflics with the operation of binfmt_elf. Some examples of its
>>> use would be to launch some executables of various different
>>> architectures in a directory, or for running some native binaries
>>> under a sandbox (like firejail) automatically during their
>>> launch.
>> 
>> Could you expand on the use cases?

Likewise, I would also like to hear about the intended use cases.

> A non-security use case would be to run the binary (without
> modification) with a different ELF interpreter (assuming this allows
> to override binfmt_elf, but self-sandboxing would need that as well).
> This would make it easier to use older or newer libcs for select
> binaries on the system.  Right now, one has to write wrappers for
> that, and the explicit dynamic linker invocation is not completely
> transparent to the application.

I think this is a slightly contrived use case. Normally you would just
use patchelf, or build the binary with a specific dynamic loader e.g.
-Wl,--dynamic-linker=file. What is restricting the use case from modifying
the binary or running under the new loader? Or to put it another way,
what requires the interpreter selection to be fully dynamic?

Why isn't the answer: Use ELF everywhere and specify the interpreter
you actually want? Likewise if you know you want to one-off run a native
binary in a sandbox or alternate loader then use a userspace tool to do
that?

Does the convenience gained justify the complexity we now have to explain
to our users "Oh, and watch out for the xattr keywords that change how
your application is started?" I'm doubtful. I haven't seen enough use
cases here to justify a fully dynamic interpreter selection.

It feels like a better use of our energy would be to make exec by the
kernel and exec by the dynamic loader look as similar as possible so you
can use one or the other without the application knowing the difference.

My worry is that this is a new knob for something we have previously
all expected to be a part of the binaries static definition.
What impact will have on security validation? Is the binary running
with the intended runtime? I assume that setting the xattr keyword 
requires write for the file in question, and that no capabilities
mismatch means we could set the keyword but have less than write
permissions on the file.

There would also be a non-zero performance cost to this and it would
be borne by all ELF/misc binaries at startup for what is a debugging
feature. This assumes we extend binfmt_elf to look for the same xattr
keyword to override the loader.

This ignores the fact that the alternate loader also needs to have
it's own ldconfig cache, implementation-dependent lookup paths etc,
all of which have to be keyed off the xattr keyword specified dynamic
loader. All of that is tractable though and can be done in userspace
keyed from the selected dynamic loader. Buy why? Why not use a mount
namespace and different loader e.g. lxc, docker, etc, or specify a
loader that is a wrapper and does this for you?

I'm not convinced this is a good idea, but I'm open to learning about
more use cases.

-- 
Cheers,
Carlos.

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

> A non-security use case would be to run the binary (without 
> modification) with a different ELF interpreter (assuming this allows to 
> override binfmt_elf, but self-sandboxing would need that as well).  This 
> would make it easier to use older or newer libcs for select binaries on 
> the system.  Right now, one has to write wrappers for that, and the 
> explicit dynamic linker invocation is not completely transparent to the 
> application.

If it gets in I'll be using it to label CP/M COM files so that they can
be auto-run nicely when crossbuilding stuff in part with the original
tools but a modern build environment 8)

Sandboxing is an obvious use but there are more bizarre ones such as
marking a file system image to get auto-run under a virtual machine or
make containers fire up as if they were commands.

It's effectively also the long missing but much needed "this document
belongs to this app" meta data that MacOS and the like have had for
decades.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[James Bottomley]

On Fri, 2016-08-26 at 22:12 +0100, One Thousand Gnomes wrote:
> > A non-security use case would be to run the binary (without 
> > modification) with a different ELF interpreter (assuming this 
> > allows to override binfmt_elf, but self-sandboxing would need that 
> > as well).  This would make it easier to use older or newer libcs 
> > for select binaries on the system.  Right now, one has to write 
> > wrappers for that, and the explicit dynamic linker invocation is 
> > not completely transparent to the application.
> 
> If it gets in I'll be using it to label CP/M COM files so that they 
> can be auto-run nicely when crossbuilding stuff in part with the 
> original tools but a modern build environment 8)
> 
> Sandboxing is an obvious use but there are more bizarre ones such as
> marking a file system image to get auto-run under a virtual machine 
> or make containers fire up as if they were commands.

So I asked previously but didn't get an answer.  If this is useful for
sandboxing and being in the sandbox depends on the xattr value,
shouldn't it be in one of the privileged xattr namespaces, not the
user. one?

James


> It's effectively also the long missing but much needed "this document
> belongs to this app" meta data that MacOS and the like have had for
> decades.
> 
> Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux
> -fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[James Bottomley]

On Fri, 2016-08-26 at 13:59 -0400, Carlos O'Donell wrote:
> On 08/26/2016 10:55 AM, Florian Weimer wrote:
> > On 08/25/2016 06:15 PM, James Bottomley wrote:
> > > On Sun, 2016-08-21 at 21:01 -0700, Josh Max wrote:
> > > > This patch allows binfmt_misc to select the interpeter for
> > > > arbitrary binaries by comparing a specified registered keyword
> > > > with the value of a specified binary's extended attribute
> > > > (user.binfmt.interp), and then launching the program with the
> > > > registered interpreter.
> > > > 
> > > > This is useful when wanting to launch a collection of binaries
> > > > under the same interpreter, even when they do not necessarily
> > > > share a common extension or magic bits, or when their magic
> > > > conflics with the operation of binfmt_elf. Some examples of its
> > > > use would be to launch some executables of various different
> > > > architectures in a directory, or for running some native
> > > > binaries
> > > > under a sandbox (like firejail) automatically during their
> > > > launch.
> > > 
> > > Could you expand on the use cases?
> 
> Likewise, I would also like to hear about the intended use cases.
> 
> > A non-security use case would be to run the binary (without
> > modification) with a different ELF interpreter (assuming this
> > allows
> > to override binfmt_elf, but self-sandboxing would need that as
> > well).
> > This would make it easier to use older or newer libcs for select
> > binaries on the system.  Right now, one has to write wrappers for
> > that, and the explicit dynamic linker invocation is not completely
> > transparent to the application.
> 
> I think this is a slightly contrived use case. Normally you would 
> just use patchelf, or build the binary with a specific dynamic loader 
> e.g. -Wl,--dynamic-linker=file. What is restricting the use case from
> modifying the binary or running under the new loader? Or to put it 
> another way, what requires the interpreter selection to be fully
> dynamic?
> 
> Why isn't the answer: Use ELF everywhere and specify the interpreter
> you actually want? Likewise if you know you want to one-off run a 
> native binary in a sandbox or alternate loader then use a userspace 
> tool to do that?

So I'm now worried about the stacking case: if you're emulating the
architecture for the binary, which is the traditional binfmt_misc use
case, you can't use this xattr trick to override the elf interpreter
because the sequence goes

binfmt_misc -> qemu-user -> elf_interp

meaning qemu-user has to know to look in the xattr.  Doing patchelf
fixes this case as well, so it sounds like the better solution.

James

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[One Thousand Gnomes]

On Fri, 26 Aug 2016 17:26:18 -0400
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Fri, 2016-08-26 at 22:12 +0100, One Thousand Gnomes wrote:
> > > A non-security use case would be to run the binary (without 
> > > modification) with a different ELF interpreter (assuming this 
> > > allows to override binfmt_elf, but self-sandboxing would need that 
> > > as well).  This would make it easier to use older or newer libcs 
> > > for select binaries on the system.  Right now, one has to write 
> > > wrappers for that, and the explicit dynamic linker invocation is 
> > > not completely transparent to the application.  
> > 
> > If it gets in I'll be using it to label CP/M COM files so that they 
> > can be auto-run nicely when crossbuilding stuff in part with the 
> > original tools but a modern build environment 8)
> > 
> > Sandboxing is an obvious use but there are more bizarre ones such as
> > marking a file system image to get auto-run under a virtual machine 
> > or make containers fire up as if they were commands.  
> 
> So I asked previously but didn't get an answer.  If this is useful for
> sandboxing and being in the sandbox depends on the xattr value,
> shouldn't it be in one of the privileged xattr namespaces, not the
> user. one?

IMHO no 

- because it's not giving additional rights, it is taking rights away
  voluntarily
- because as a user I can simply cp the file to get an unsandboxed version

If it was a setuid like bit then yes it would matter.

Alan

Re: [PATCH] binfmt_misc: allow selecting the interpreter based on xattr keywords

[Alex Bennée]

Carlos O'Donell <carlos@redhat.com> writes:

> On 08/26/2016 10:55 AM, Florian Weimer wrote:
>> On 08/25/2016 06:15 PM, James Bottomley wrote:
>>> On Sun, 2016-08-21 at 21:01 -0700, Josh Max wrote:
<snip>
>
> This ignores the fact that the alternate loader also needs to have
> it's own ldconfig cache, implementation-dependent lookup paths etc,
> all of which have to be keyed off the xattr keyword specified dynamic
> loader. All of that is tractable though and can be done in userspace
> keyed from the selected dynamic loader. Buy why? Why not use a mount
> namespace and different loader e.g. lxc, docker, etc, or specify a
> loader that is a wrapper and does this for you?

Is binfmt_misc actually containerise-able yet? At the moment when using
qemu-user inside a docker container I still have to ensure the root
binfmt_misc points to the same location as I place qemu-user in the
docker container.

>
> I'm not convinced this is a good idea, but I'm open to learning about
> more use cases.


--
Alex Bennée