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.
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
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]
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.
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
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
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
--- Linus Torvalds <[email protected]> wrote:
>
> 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..
>
If the binary resides on a NFS drive ( which is a very common practice )
then the suid-wrapper solution will not work because root permissions
are squashed on the remote drive.
On Wednesday 03 September 2003 10:16, Zach, Yoav wrote:
> --- Linus Torvalds <[email protected]> wrote:
> > 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..
>
> If the binary resides on a NFS drive ( which is a very common practice )
> then the suid-wrapper solution will not work because root permissions
> are squashed on the remote drive.
This is a NFS promlem. Do not work around it by adding crap elsewhere.
NFS has to get a decent user auth/crypto features.
I did not try it yet, but NFSv4 will address that.
--
vda
> 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.
> --- insecure <[email protected]> wrote:
>
> > If the binary resides on a NFS drive ( which
> > is a very common practice )
> > then the suid-wrapper solution will not work
> > because root permissions
> > are squashed on the remote drive.
>
>
> This is a NFS promlem. Do not work around it by
> adding crap elsewhere.
> NFS has to get a decent user auth/crypto features.
> I did not try it yet, but NFSv4 will address that.
> --
This is not a workaround - it's a solution for systems
that use the unix user identification mechanism.
Considering the conservative nature of system-administrators,
this mechanism will still be in use for quite a while.
Thanks,
Yoav.
On Thursday 04 September 2003 09:53, Zach, Yoav wrote:
> > --- insecure <[email protected]> wrote:
> > > If the binary resides on a NFS drive ( which
> > > is a very common practice )
> > > then the suid-wrapper solution will not work
> > > because root permissions
> > > are squashed on the remote drive.
> >
> > This is a NFS promlem. Do not work around it by
> > adding crap elsewhere.
> > NFS has to get a decent user auth/crypto features.
> > I did not try it yet, but NFSv4 will address that.
>
> This is not a workaround - it's a solution for systems
> that use the unix user identification mechanism.
In NFSv3 there is _no_ user identification mechanism.
ipaddr based /etc/exports does not count.
--
vda