[PATCH] Handle non-readable binfmt_misc executables

<background>

I work in a group that works on enabling the IA-32 Execution Layer
(http://www.intel.com/pressroom/archive/releases/20040113comp.htm) on Linux.
In a few words - this is a dynamic translator for IA-32 binaries on IPF
platform.  Following David Mosberger's advice - we use the binfmt_misc
mechanism for the invocation of the translator whenever the user tries to
exec an IA-32 binary.

The EL is meant to help in the migration path from IA-32 to IPF.  From our
beta customers we learnt that at first stage - they tend to keep their
environment mostly intact, using the legacy IA-32 binaries.

Such an environment has, naturally, setuid and non-readable binaries.  It
will be useless to ask the administrator to change the settings of such an
environment - some of them are very complex, and the administrators are
reluctant to make any changes in a system that already proved itself to be
robust and secure.  So, our target with these patches is not to enhance the
support for scripts but rather to allow a translator to be integrated into a
working environment that is not (and should not be) aware to the fact it's
being emulated.

As I said before - it is practically hopeless to expect an administrator of
such a system to change it so that it will suit the current behavior of
binfmt_misc.  But, even if we could do that,

I'm not sure it would be a good idea - these changes are likely to be less
secure than the suggested patches -

- In order to execute non-readable binaries the binary will have to be made
  readable, which is obviously less secure than allowing only a trusted
  translator to read it

- There will be no way for the translator to calculate the accurate
  AT_SECURE value for the translated process.  This might end up with the
  translated process running in a non-secured mode when it actually needs to
  be secured.

</background>


I prepared a patch that solves a couple of problems that interpreters have
when invoked via binfmt_misc.  currently -

1) such interpreters cannot open non-readable binaries

2) the processes will have their credentials and security attributes
   calculated according to interpreter permissions and not those of the
   original binary

the proposed patch solves these problems by -

1) opening the binary on behalf of the interpreter and passing its fd
   instead of the path as argv[1] to the interpreter

2) calling prepare_binprm with the file struct of the binary and not the
   one of the interpreter

The new functionality is enabled by adding a special flag to the registration
string.  If this flag is not added then old behavior is not changed.

A preliminary version of this patch was sent to the list on 9/1/2003 with the
title "[PATCH]: non-readable binaries - binfmt_misc 2.6.0-test4".  This new
version fixes the concerns that were raised by the patch, except of calling
unshare_files() before allocating a new fd.  this is because this feature did
not enter 2.6 yet.


Arun Sharma <arun.sharma@intel.com> says:

We were going through an internal review of this patch:

http://marc.theaimsgroup.com/?l=linux-kernel&m=107424598901720&w=2

which is in your tree already.  I'm not sure if this line of code got
sufficient review.

+               /* call prepare_binprm before switching to interpreter's file
+                * so that all security calculation will be done according to
+                * binary and not interpreter */
+               retval = prepare_binprm(bprm);

The case that concerns me is: unprivileged interpreter and a privileged
binary.  One can use binfmt_misc to execute untrusted code (interpreter) with
elevated privileges.  One could argue that all binfmt_misc interpreters are
trusted, because only root can register them.  But that's a change from the
traditional behavior of binfmt_misc (and binfmt_script).


(Update):

Arun pointed out that calculating the process credentials according to the
binary that needs to be translated is a bit risky, since it requires the
administrator to pay extra attention not to register an interpreter which is
not intended to run with root credentials.

After discussing this issue with him, I would like to propose a modified
patch: The old patch did 2 things - 1) open the binary for reading and 2)
calculate the credentials according to the binary.

I removed the riskier part of changing the credentials calculation, so the
revised patch only opens the binary for reading.  It also includes few words
of warning in the description of the 'open-binary' feature in
binfmt_misc.txt, and makes the function entry_status print the flags in use.

As for the 'credentials' part of the patch, I will prepare a separate patch
for it and send it again to the LKML, describe the problem and ask for people
comments.
Signed-off-by: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>

Documentation/binfmt_misc.txt

@@ -15,7 +15,7 @@ First you must mount binfmt_misc:
 	mount binfmt_misc -t binfmt_misc /proc/sys/fs/binfmt_misc 
 
 To actually register a new binary type, you have to set up a string looking like
-:name:type:offset:magic:mask:interpreter: (where you can choose the ':' upon
+:name:type:offset:magic:mask:interpreter:flags (where you can choose the ':' upon
 your needs) and echo it to /proc/sys/fs/binfmt_misc/register.
 Here is what the fields mean:
  - 'name' is an identifier string. A new /proc file will be created with this
@@ -34,6 +34,21 @@ Here is what the fields mean:
    The mask is anded with the byte sequence of the file.
  - 'interpreter' is the program that should be invoked with the binary as first
    argument (specify the full path)
+ - 'flags' is an optional field that controls several aspects of the invocation
+   of the interpreter. It is a string of capital letters, each controls a certain
+   aspect. The following flags are supported -
+      'P' - preserve-argv[0].  Legacy behavior of binfmt_misc is to overwrite the
+            original argv[0] with the full path to the binary.  When this flag is
+            included, binfmt_misc will add an argument to the argument vector for
+            this purpose, thus preserving the original argv[0].
+      'O' - open-binary. Legacy behavior of binfmt_misc is to pass the full path
+            of the binary to the interpreter as an argument. When this flag is
+            included, binfmt_misc will open the file for reading and pass its
+            descriptor as an argument, instead of the full path, thus allowing
+            the interpreter to execute non-readable binaries. This feature should
+            be used with care - the interpreter has to be trusted not to emit
+            the contents of the non-readable binary.
+
 
 There are some restrictions:
  - the whole register string may not exceed 255 characters
@@ -83,9 +98,9 @@ If you want to pass special arguments to your interpreter, you can
 write a wrapper script for it. See Documentation/java.txt for an
 example.
 
-Your interpreter should NOT look in the PATH for the filename; the
-kernel passes it the full filename to use.  Using the PATH can cause
-unexpected behaviour and be a security hazard.
+Your interpreter should NOT look in the PATH for the filename; the kernel
+passes it the full filename (or the file descriptor) to use.  Using $PATH can
+cause unexpected behaviour and can be a security hazard.
 
 
 There is a web page about binfmt_misc at

fs/binfmt_misc.c

@@ -39,6 +39,7 @@ static int enabled = 1;
 
 enum {Enabled, Magic};
 #define MISC_FMT_PRESERVE_ARGV0 (1<<31)
+#define MISC_FMT_OPEN_BINARY (1<<30)
 
 typedef struct {
 	struct list_head list;
@@ -102,10 +103,15 @@ static Node *check_file(struct linux_binprm *bprm)
 static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 {
 	Node *fmt;
-	struct file * file;
+	struct file * interp_file = NULL;
+	struct file * binary_file = NULL;
 	char iname[BINPRM_BUF_SIZE];
 	char *iname_addr = iname;
 	int retval;
+	int fd_binary = -1;
+	char fd_str[32];
+	char * fdsp = fd_str;
+	int is_open_bin;
 
 	retval = -ENOEXEC;
 	if (!enabled)
@@ -120,33 +126,91 @@ static int load_misc_binary(struct linux_binprm *bprm, struct pt_regs *regs)
 	if (!fmt)
 		goto _ret;
 
-	allow_write_access(bprm->file);
-	fput(bprm->file);
+	is_open_bin = (fmt->flags & MISC_FMT_OPEN_BINARY) ? 1 : 0;
+
+ 	if (is_open_bin) {
+		/* if the binary should be opened on behalf of the
+		 * interpreter than keep it open and assign descriptor
+		 * to it */
+ 		fd_binary = get_unused_fd ();
+ 		if (fd_binary < 0) {
+ 			retval = fd_binary;
+ 			goto _ret;
+ 		}
+ 		snprintf (fd_str, sizeof(fd_str) - 1, "%d", fd_binary);
+ 	} 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 (retval < 0) goto _ret; 
-	bprm->argc++;
-	retval = copy_strings_kernel(1, &iname_addr, bprm);
-	if (retval < 0) goto _ret; 
-	bprm->argc++;
+
+ 	if (is_open_bin) {
+		/* make argv[1] be the file descriptor of the binary */
+ 		retval = copy_strings_kernel (1, &fdsp, bprm);
+ 	} else {
+		/* make argv[1] be the path to the binary */
+ 		retval = copy_strings_kernel (1, &bprm->interp, bprm);
+ 	}
+	if (retval < 0)
+		goto _error;
+	bprm->argc ++;
+	retval = copy_strings_kernel (1, &iname_addr, bprm);
+	if (retval < 0)
+		goto _error;
+	bprm->argc ++;
 	bprm->interp = iname;	/* for binfmt_script */
 
-	file = open_exec(iname);
-	retval = PTR_ERR(file);
-	if (IS_ERR(file))
-		goto _ret;
-	bprm->file = file;
+	interp_file = open_exec (iname);
+	retval = PTR_ERR (interp_file);
+	if (IS_ERR (interp_file))
+		goto _error;
 
+
+	binary_file = bprm->file;
+	bprm->file = interp_file;
 	retval = prepare_binprm(bprm);
-	if (retval >= 0)
-		retval = search_binary_handler(bprm, regs);
+
+	if (retval < 0)
+		goto _error;
+
+	if (is_open_bin) {
+		/* if the binary is not readable than enforce mm->dumpable=0
+		   regardless of the interpreter's permissions */
+		if (permission (binary_file->f_dentry->d_inode, MAY_READ, NULL)) {
+			bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP;
+		}
+		/* install the binary's fd. it is done at the latest possible point
+	 	 * because once it is installed it will need to be sys_close()ed
+	 	 * in case of error.
+	 	 */
+ 		fd_install (fd_binary, binary_file);
+	}
+
+	retval = search_binary_handler (bprm, regs);
+
+	if (retval < 0)
+		goto _error_close_file;
+
 _ret:
 	return retval;
+
+_error_close_file:
+	if (fd_binary > 0) {
+		sys_close (fd_binary);
+		fd_binary = -1;
+		bprm->file = NULL;
+	}
+_error:
+	if (fd_binary > 0)
+		put_unused_fd (fd_binary);
+	bprm->interp_flags = 0;
+	goto _ret;
+
 }
 
 /* Command parsers */
@@ -191,6 +255,29 @@ static int unquote(char *from)
 	return p - from;
 }
 
+static inline char * check_special_flags (char * sfs, Node * e)
+{
+	char * p = sfs;
+	int cont = 1;
+
+	/* special flags */
+	while (cont) {
+		switch (*p) {
+			case 'P':
+				p++;
+				e->flags |= MISC_FMT_PRESERVE_ARGV0;
+				break;
+			case 'O':
+				p++;
+				e->flags |= MISC_FMT_OPEN_BINARY;
+				break;
+			default:
+				cont = 0;
+		}
+	}
+
+	return p;
+}
 /*
  * This registers a new binary format, it recognises the syntax
  * ':name:type:offset:magic:mask:interpreter:'
@@ -293,10 +380,8 @@ static Node *create_entry(const char __user *buffer, size_t count)
 	if (!e->interpreter[0])
 		goto Einval;
 
-	if (*p == 'P') {
-		p++;
-		e->flags |= MISC_FMT_PRESERVE_ARGV0;
-	}
+
+	p = check_special_flags (p, e);
 
 	if (*p == '\n')
 		p++;
@@ -346,6 +431,7 @@ static void entry_status(Node *e, char *page)
 {
 	char *dp;
 	char *status = "disabled";
+	const char * flags = "flags: ";
 
 	if (test_bit(Enabled, &e->flags))
 		status = "enabled";
@@ -357,6 +443,19 @@ static void entry_status(Node *e, char *page)
 
 	sprintf(page, "%s\ninterpreter %s\n", status, e->interpreter);
 	dp = page + strlen(page);
+
+	/* print the special flags */
+	sprintf (dp, "%s", flags);
+	dp += strlen (flags);
+	if (e->flags & MISC_FMT_PRESERVE_ARGV0) {
+		*dp ++ = 'P';
+	}
+	if (e->flags & MISC_FMT_OPEN_BINARY) {
+		*dp ++ = 'O';
+	}
+	*dp ++ = '\n';
+
+
 	if (!test_bit(Magic, &e->flags)) {
 		sprintf(dp, "extension .%s\n", e->magic);
 	} else {

fs/exec.c

@@ -839,7 +839,8 @@ int flush_old_exec(struct linux_binprm * bprm)
 	flush_thread();
 
 	if (bprm->e_uid != current->euid || bprm->e_gid != current->egid || 
-	    permission(bprm->file->f_dentry->d_inode,MAY_READ, NULL))
+	    permission(bprm->file->f_dentry->d_inode,MAY_READ, NULL) ||
+	    (bprm->interp_flags & BINPRM_FLAGS_ENFORCE_NONDUMP))
 		current->mm->dumpable = 0;
 
 	/* An exec changes our domain. We are no longer part of the thread
@@ -1088,6 +1089,7 @@ int do_execve(char * filename,
 	bprm.file = file;
 	bprm.filename = filename;
 	bprm.interp = filename;
+	bprm.interp_flags = 0;
 	bprm.sh_bang = 0;
 	bprm.loader = 0;
 	bprm.exec = 0;

include/linux/binfmts.h

@@ -35,9 +35,13 @@ struct linux_binprm{
 	char * interp;		/* Name of the binary really executed. Most
 				   of the time same as filename, but could be
 				   different for binfmt_{misc,script} */
+	unsigned long interp_flags;
 	unsigned long loader, exec;
 };
 
+#define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
+#define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
+
 /*
  * This structure defines the functions that are used to load the binary formats that
  * linux accepts.