[PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Zach, Yoav]

The proposed patch solves a problem for interpreters that need to
execute a non-readable file, which cannot be read in userland. To handle
such cases the interpreter must have the kernel load the binary on its
behalf. The proposed patch handles this case by telling binfmt_misc, by
a special flag in the registration string, to open the binary for
reading and pass its descriptor as argv[1], instead of passing the
binary's path. Old behavior of binfmt_misc is kept for interpreters
which do not specify this special flag. The patch is against
linux-2.6.0-test4. A similar one was posted twice on the list, on Aug.
14 and 21, without significant response.


========================== start patch
==================================
diff -r -U 3 linux-2.6.0-test4/fs/binfmt_misc.c linux/fs/binfmt_misc.c
--- linux-2.6.0-test4/fs/binfmt_misc.c	Sat Aug 23 02:54:23 2003
+++ linux/fs/binfmt_misc.c	Mon Sep  1 00:17:01 2003
@@ -38,6 +38,8 @@
 
 enum {Enabled, Magic};
 #define MISC_FMT_PRESERVE_ARGV0 (1<<31)
+#define MISC_FMT_OPEN_BINARY (1<<30)
+
 
 typedef struct {
 	struct list_head list;
@@ -106,6 +108,10 @@
 	char *iname_addr = iname;
 	int retval;
 
+	int fd;
+	char fd_str[32];
+	char * fdsp = fd_str;
+
 	retval = -ENOEXEC;
 	if (!enabled)
 		goto _ret;
@@ -119,16 +125,41 @@
 	if (!fmt)
 		goto _ret;
 
-	allow_write_access(bprm->file);
-	fput(bprm->file);
-	bprm->file = NULL;
+ 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
+		/* if the binary should be opened on behalf of the
+		 * interpreter than keep it open and assign it a
descriptor */
+ 		fd = get_unused_fd ();
+ 		if (fd < 0) {
+ 			allow_write_access(bprm->file);
+ 			fput(bprm->file);
+ 			bprm->file = NULL;
+ 			retval = fd;
+ 			goto _ret;
+ 		} else {
+ 			fd_install (fd, bprm->file);
+ 			sprintf (fd_str, "/dev/fd/%d", fd);
+ 		}
+ 	} else {
+ 		allow_write_access(bprm->file);
+ 		fput(bprm->file);
+ 		bprm->file = NULL;
+ 	}
+ 
 
 	/* Build args for interpreter */
 	if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) {
 		remove_arg_zero(bprm);
 	}
-	retval = copy_strings_kernel(1, &bprm->interp, bprm);
+
+ 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
+		/* make argv[1] be the file descriptor */
+ 		retval = copy_strings_kernel(1, &fdsp, bprm);
+ 	} else {
+		/* make argv[1] be the path to the file */
+ 		retval = copy_strings_kernel(1, &bprm->interp, bprm);
+ 	}
 	if (retval < 0) goto _ret; 
+
 	bprm->argc++;
 	retval = copy_strings_kernel(1, &iname_addr, bprm);
 	if (retval < 0) goto _ret; 
@@ -139,9 +170,18 @@
 	retval = PTR_ERR(file);
 	if (IS_ERR(file))
 		goto _ret;
-	bprm->file = file;
 
-	retval = prepare_binprm(bprm);
+	/* get the file permissions and read its header */
+	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
+		retval = prepare_binprm(bprm);
+		bprm->file = file;
+		memset(bprm->buf,0,BINPRM_BUF_SIZE);
+		retval =
kernel_read(bprm->file,0,bprm->buf,BINPRM_BUF_SIZE);
+	} else {
+		bprm->file = file;
+		retval = prepare_binprm(bprm);
+	}
+
 	if (retval >= 0)
 		retval = search_binary_handler(bprm, regs);
 _ret:
@@ -296,6 +336,10 @@
 		p++;
 		e->flags |= MISC_FMT_PRESERVE_ARGV0;
 	}
+	if (*p == 'O') {
+		p++;
+		e->flags |= MISC_FMT_OPEN_BINARY;
+	}
 
 	if (*p == '\n')
 		p++;
========================== end patch ==================================


Yoav Zach
Performance Tools Lab
Intel Corp.

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Herbert Poetzl]

On Mon, Sep 01, 2003 at 12:41:23AM +0300, Zach, Yoav wrote:
> The proposed patch solves a problem for interpreters that need to
> execute a non-readable file, which cannot be read in userland. To handle
> such cases the interpreter must have the kernel load the binary on its
> behalf. The proposed patch handles this case by telling binfmt_misc, by
> a special flag in the registration string, to open the binary for
> reading and pass its descriptor as argv[1], instead of passing the
> binary's path. Old behavior of binfmt_misc is kept for interpreters
> which do not specify this special flag. The patch is against
> linux-2.6.0-test4. A similar one was posted twice on the list, on Aug.
> 14 and 21, without significant response.

okay, here is your response!

what non-readable files need to be interpreted/executed?
why is this case relevant?
why not simply make it user-land readable?

best,
Herbert

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Alan Cox]

On Sul, 2003-08-31 at 23:12, Herbert Poetzl wrote:
> what non-readable files need to be interpreted/executed?
> why is this case relevant?
> why not simply make it user-land readable?

If you are running binaries for another architecture via software
emulation (for example qemu running x86 binaries on your S/390 
transparently) then you want exec only binaries to work with an
otherwise trusted interpreter [Getting the interpreter trust stuff
right is really hard btw - so ironically it probably has to be setuid
anyway so you can't steal the handle]

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Alan Cox]

On Sul, 2003-08-31 at 22:41, Zach, Yoav wrote:
> The proposed patch solves a problem for interpreters that need to
> execute a non-readable file, which cannot be read in userland. To handle

You've added a security hole.

> + 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
> +		/* if the binary should be opened on behalf of the
> +		 * interpreter than keep it open and assign it a
> descriptor */
> + 		fd = get_unused_fd ();
> + 		if (fd < 0) {

At this point your file table might be shared with another thread (see
binfmt_elf in 2.4 - I dunno if 2.6 has been fixed for this exploit yet).
You need to unshare the file table before you modify it during the exec.

Otherwise it looks fairly sane.

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Alan Cox]

On Sul, 2003-08-31 at 22:41, Zach, Yoav wrote:
> binary's path. Old behavior of binfmt_misc is kept for interpreters
> which do not specify this special flag. The patch is against
> linux-2.6.0-test4. A similar one was posted twice on the list, on Aug.
> 14 and 21, without significant response.

Aside from the general unshare fixes here is the other small problem you
need to look at 

#1 You can't assume /dev/fd/0 so why not just pass the filehandle number
as argv1 instead like the a.out loader did years ago

#2 Use snprintf not sprintf (Im sure sprintf is safe here but its easier
to audit code if you use snprintf)

#3 The instant you pass control to the user space loader I can steal the
handle via /proc

#4 The instant you pass control to the user space loader I can take it
over via ptrace

#5 After you pass control I can core dump the app and recover the data
using a signal

3, 4 and 5 require you make the userspace loader undumpable in the case 
where the fd being passed on is executable only. If you do this then it
certainly fixes 4 (permission denied) and 5 (no dump) and I think it
fixes #3

Alan

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Willy Tarreau]

On Sun, Aug 31, 2003 at 11:58:40PM +0100, Alan Cox wrote:
 
> #3 The instant you pass control to the user space loader I can steal the
> handle via /proc
> 
> #4 The instant you pass control to the user space loader I can take it
> over via ptrace
> 
> #5 After you pass control I can core dump the app and recover the data
> using a signal
> 
> 3, 4 and 5 require you make the userspace loader undumpable in the case 
> where the fd being passed on is executable only. If you do this then it
> certainly fixes 4 (permission denied) and 5 (no dump) and I think it
> fixes #3

I confirm that it fixes #3 since I had a problem with /dev/fd/N not working
in my scripts, until I realized that it was because an exec only bash would
render /proc/self/fd/ unreadable.

Willy

Re: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Linus Torvalds]

On Mon, 1 Sep 2003, Zach, Yoav wrote:
>
> The proposed patch solves a problem for interpreters that need to
> execute a non-readable file, which cannot be read in userland. To handle
> such cases the interpreter must have the kernel load the binary on its
> behalf.

I don't like the security issues here. Sure, you "trust" the interpreter, 
and clearly only root can set the flag, but to me that just makes me 
wonder why the interpreter itself can't be a simple suid wrapper that does 
the mapping rather than having it done in kernel space..

		Linus

RE: [PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4

[Zach, Yoav]

> You've added a security hole.
> 
> > + 	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
> > +		/* if the binary should be opened on behalf of the
> > +		 * interpreter than keep it open and assign it a
> > descriptor */
> > + 		fd = get_unused_fd ();
> > + 		if (fd < 0) {
> 
> At this point your file table might be shared with another thread (see
> binfmt_elf in 2.4 - I dunno if 2.6 has been fixed for this 
> exploit yet).
> You need to unshare the file table before you modify it 
> during the exec.
> 
> Otherwise it looks fairly sane.
> 

Do you know whether the 'unshare_files' patch was already prepared and
sent to the 2.6 maintainer ? If not then I would like to do it.

Thanks,
Yoav.