Re: [PATCH] revert- sys_setaltroot

From: David Woodhouse
Date: Wed Dec 22 2004 - 05:21:26 EST

On Tue, 2004-10-26 at 15:22 +0000, Linux Kernel Mailing List wrote:
> ChangeSet 1.2187, 2004/10/26 08:22:01-07:00, akpm@xxxxxxxx
>
> [PATCH] revert- sys_setaltroot
> We decided to do this a different way.

Can you elaborate? We don't have open_exec() in userspace, so I'm
not sure it's possible to do this properly for setuid binaries and
execute-only binaries in userspace without being seriously exploitable.

I assume the problem with it was that you can set the altroot to a
directory containing a bogus /etc/shadow and then 'su' with a password
of your own choosing, or something like that?

There's no reason to make the user-specified altroot survive across
exec() though -- and as long it _doesn't_ persist, it should be
perfectly safe to let users set it. I'm working on a patch which puts
sys_setaltroot back but discards it on exec. This allows me to ditch all
the horrid path walking stuff from qemu, and as soon as I make qemu
understand AT_EXECFD it should let me run execute-only i386 binaries on
my ppc box too.

T'would be useful to confirm the original objections to it and that I
haven't missed a viable 'different way' to do it, though.

--
dwmw2
===== arch/i386/kernel/entry.S 1.88 vs edited =====
--- 1.88/arch/i386/kernel/entry.S 2004-12-12 18:37:55 +00:00
+++ edited/arch/i386/kernel/entry.S 2004-12-20 13:08:02 +00:00
@@ -861,7 +861,7 @@
.long sys_mq_getsetattr
.long sys_ni_syscall /* reserved for kexec */
.long sys_waitid
- .long sys_ni_syscall /* 285 */ /* available */
+ .long sys_setaltroot /* 285 */
.long sys_add_key
.long sys_request_key
.long sys_keyctl
===== arch/ia64/kernel/entry.S 1.69 vs edited =====
--- 1.69/arch/ia64/kernel/entry.S 2004-11-23 20:18:30 +00:00
+++ edited/arch/ia64/kernel/entry.S 2004-12-20 13:08:02 +00:00
@@ -1531,7 +1531,7 @@
data8 sys_add_key
data8 sys_request_key
data8 sys_keyctl
- data8 sys_ni_syscall
+ data8 sys_setaltroot
data8 sys_ni_syscall // 1275
data8 sys_ni_syscall
data8 sys_ni_syscall
===== arch/ppc/kernel/misc.S 1.63 vs edited =====
--- 1.63/arch/ppc/kernel/misc.S 2004-10-22 10:27:40 +01:00
+++ edited/arch/ppc/kernel/misc.S 2004-12-20 13:08:02 +00:00
@@ -1450,3 +1450,4 @@
.long sys_add_key
.long sys_request_key /* 270 */
.long sys_keyctl
+ .long sys_setaltroot
===== arch/ppc64/kernel/misc.S 1.93 vs edited =====
--- 1.93/arch/ppc64/kernel/misc.S 2004-11-11 08:23:03 +00:00
+++ edited/arch/ppc64/kernel/misc.S 2004-12-20 13:08:02 +00:00
@@ -966,7 +966,8 @@
.llong .sys32_add_key
.llong .sys32_request_key
.llong .compat_sys_keyctl
-
+ .llong .sys_setaltroot
+
.balign 8
_GLOBAL(sys_call_table)
.llong .sys_restart_syscall /* 0 */
@@ -1241,3 +1242,4 @@
.llong .sys_add_key
.llong .sys_request_key /* 270 */
.llong .sys_keyctl
+ .llong .sys_setaltroot
===== arch/sparc/kernel/systbls.S 1.29 vs edited =====
--- 1.29/arch/sparc/kernel/systbls.S 2004-11-08 02:08:03 +00:00
+++ edited/arch/sparc/kernel/systbls.S 2004-12-20 13:08:02 +00:00
@@ -75,7 +75,7 @@
/*265*/ .long sys_timer_delete, sys_timer_create, sys_nis_syscall, sys_io_setup, sys_io_destroy
/*270*/ .long sys_io_submit, sys_io_cancel, sys_io_getevents, sys_mq_open, sys_mq_unlink
/*275*/ .long sys_mq_timedsend, sys_mq_timedreceive, sys_mq_notify, sys_mq_getsetattr, sys_waitid
-/*280*/ .long sys_ni_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/ .long sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl

#ifdef CONFIG_SUNOS_EMUL
/* Now the SunOS syscall table. */
===== arch/sparc64/kernel/systbls.S 1.66 vs edited =====
--- 1.66/arch/sparc64/kernel/systbls.S 2004-11-11 08:23:03 +00:00
+++ edited/arch/sparc64/kernel/systbls.S 2004-12-20 13:08:02 +00:00
@@ -76,7 +76,7 @@
.word sys_timer_delete, sys32_timer_create, sys_ni_syscall, compat_sys_io_setup, sys_io_destroy
/*270*/ .word sys32_io_submit, sys_io_cancel, compat_sys_io_getevents, sys32_mq_open, sys_mq_unlink
.word sys_mq_timedsend, sys_mq_timedreceive, compat_sys_mq_notify, compat_sys_mq_getsetattr, compat_sys_waitid
-/*280*/ .word sys_ni_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/ .word sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl

#endif /* CONFIG_COMPAT */

@@ -142,7 +142,7 @@
.word sys_timer_delete, sys_timer_create, sys_ni_syscall, sys_io_setup, sys_io_destroy
/*270*/ .word sys_io_submit, sys_io_cancel, sys_io_getevents, sys_mq_open, sys_mq_unlink
.word sys_mq_timedsend, sys_mq_timedreceive, sys_mq_notify, sys_mq_getsetattr, sys_waitid
-/*280*/ .word sys_nis_syscall, sys_add_key, sys_request_key, sys_keyctl
+/*280*/ .word sys_setaltroot, sys_add_key, sys_request_key, sys_keyctl

#if defined(CONFIG_SUNOS_EMUL) || defined(CONFIG_SOLARIS_EMUL) || \
defined(CONFIG_SOLARIS_EMUL_MODULE)
===== fs/binfmt_elf.c 1.92 vs edited =====
--- 1.92/fs/binfmt_elf.c 2004-11-16 19:01:21 +00:00
+++ edited/fs/binfmt_elf.c 2004-12-20 13:08:02 +00:00
@@ -37,6 +37,7 @@
#include <linux/pagemap.h>
#include <linux/security.h>
#include <linux/syscalls.h>
+#include <linux/namei.h>

#include <asm/uaccess.h>
#include <asm/param.h>
@@ -626,7 +627,7 @@
*/
SET_PERSONALITY(loc->elf_ex, ibcs2_interpreter);

- interpreter = open_exec(elf_interpreter);
+ interpreter = open_exec(elf_interpreter, LOOKUP_NOALT);
retval = PTR_ERR(interpreter);
if (IS_ERR(interpreter))
goto out_free_interp;
===== fs/binfmt_em86.c 1.8 vs edited =====
--- 1.8/fs/binfmt_em86.c 2004-05-10 12:25:55 +01:00
+++ edited/fs/binfmt_em86.c 2004-12-20 13:08:02 +00:00
@@ -19,6 +19,7 @@
#include <linux/fs.h>
#include <linux/file.h>
#include <linux/errno.h>
+#include <linux/namei.h>


#define EM86_INTERP "/usr/bin/em86"
@@ -82,7 +83,7 @@
* Note that we use open_exec() as the name is now in kernel
* space, and we don't need to copy it.
*/
- file = open_exec(interp);
+ file = open_exec(interp, LOOKUP_NOALT);
if (IS_ERR(file))
return PTR_ERR(file);

===== fs/binfmt_flat.c 1.11 vs edited =====
--- 1.11/fs/binfmt_flat.c 2004-10-19 06:26:36 +01:00
+++ edited/fs/binfmt_flat.c 2004-12-20 13:08:02 +00:00
@@ -36,6 +36,7 @@
#include <linux/personality.h>
#include <linux/init.h>
#include <linux/flat.h>
+#include <linux/namei.h>

#include <asm/byteorder.h>
#include <asm/system.h>
@@ -774,7 +775,7 @@

/* Open the file up */
bprm.filename = buf;
- bprm.file = open_exec(bprm.filename);
+ bprm.file = open_exec(bprm.filename, LOOKUP_NOALT);
res = PTR_ERR(bprm.file);
if (IS_ERR(bprm.file))
return res;
===== fs/binfmt_misc.c 1.30 vs edited =====
--- 1.30/fs/binfmt_misc.c 2004-06-29 15:43:10 +01:00
+++ edited/fs/binfmt_misc.c 2004-12-20 13:08:02 +00:00
@@ -179,7 +179,7 @@

bprm->interp = iname; /* for binfmt_script */

- interp_file = open_exec (iname);
+ interp_file = open_exec (iname, LOOKUP_NOALT);
retval = PTR_ERR (interp_file);
if (IS_ERR (interp_file))
goto _error;
===== fs/binfmt_script.c 1.9 vs edited =====
--- 1.9/fs/binfmt_script.c 2004-06-29 22:35:34 +01:00
+++ edited/fs/binfmt_script.c 2004-12-20 13:08:03 +00:00
@@ -85,9 +85,14 @@
/*
* OK, now restart the process with the interpreter's dentry.
*/
- file = open_exec(interp);
+ file = open_exec(interp, 0);
if (IS_ERR(file))
return PTR_ERR(file);
+
+ /* If we had an altroot (which may be untrusted) we can reset it
+ to the default for the personality now that we've got the
+ script's interpreter itself open. */
+ set_fs_altroot();

bprm->file = file;
retval = prepare_binprm(bprm);
===== fs/compat.c 1.47 vs edited =====
--- 1.47/fs/compat.c 2004-12-10 17:57:46 +00:00
+++ edited/fs/compat.c 2004-12-20 13:08:03 +00:00
@@ -1393,7 +1393,7 @@
goto out_ret;
memset(bprm, 0, sizeof(*bprm));

- file = open_exec(filename);
+ file = open_exec(filename, 0);
retval = PTR_ERR(file);
if (IS_ERR(file))
goto out_kfree;
@@ -1447,6 +1447,7 @@
free_arg_pages(bprm);

/* execve success */
+ set_fs_altroot();
security_bprm_free(bprm);
kfree(bprm);
return retval;
===== fs/exec.c 1.146 vs edited =====
--- 1.146/fs/exec.c 2004-12-10 17:57:46 +00:00
+++ edited/fs/exec.c 2004-12-20 13:08:03 +00:00
@@ -473,14 +473,14 @@

#endif /* CONFIG_MMU */

-struct file *open_exec(const char *name)
+struct file *open_exec(const char *name, int flags)
{
struct nameidata nd;
int err;
struct file *file;

nd.intent.open.flags = FMODE_READ;
- err = path_lookup(name, LOOKUP_FOLLOW|LOOKUP_OPEN, &nd);
+ err = path_lookup(name, LOOKUP_FOLLOW|LOOKUP_OPEN|flags, &nd);
file = ERR_PTR(err);

if (!err) {
@@ -1007,7 +1007,7 @@

loader = PAGE_SIZE*MAX_ARG_PAGES-sizeof(void *);

- file = open_exec("/sbin/loader");
+ file = open_exec("/sbin/loader", LOOKUP_NOALT);
retval = PTR_ERR(file);
if (IS_ERR(file))
return retval;
@@ -1100,7 +1100,7 @@
goto out_ret;
memset(bprm, 0, sizeof(*bprm));

- file = open_exec(filename);
+ file = open_exec(filename, 0);
retval = PTR_ERR(file);
if (IS_ERR(file))
goto out_kfree;
@@ -1155,6 +1155,7 @@
free_arg_pages(bprm);

/* execve success */
+ set_fs_altroot();
security_bprm_free(bprm);
kfree(bprm);
return retval;
===== fs/namei.c 1.115 vs edited =====
--- 1.115/fs/namei.c 2004-10-28 08:39:57 +01:00
+++ edited/fs/namei.c 2004-12-20 13:08:03 +00:00
@@ -913,20 +913,20 @@
return 1;
}

-void set_fs_altroot(void)
+int __set_fs_altroot(const char *altroot)
{
- char *emul = __emul_prefix();
struct nameidata nd;
struct vfsmount *mnt = NULL, *oldmnt;
struct dentry *dentry = NULL, *olddentry;
int err;
-
- if (!emul)
+ if (!altroot)
goto set_it;
- err = path_lookup(emul, LOOKUP_FOLLOW|LOOKUP_DIRECTORY|LOOKUP_NOALT, &nd);
+ err = path_lookup(altroot, LOOKUP_FOLLOW|LOOKUP_DIRECTORY|LOOKUP_NOALT, &nd);
if (!err) {
mnt = nd.mnt;
dentry = nd.dentry;
+ } else {
+ return err;
}
set_it:
write_lock(&current->fs->lock);
@@ -939,6 +939,58 @@
dput(olddentry);
mntput(oldmnt);
}
+ return 0;
+}
+
+void set_fs_altroot(void)
+{
+ char *emul = __emul_prefix();
+
+ __set_fs_altroot(emul);
+}
+
+asmlinkage long sys_setaltroot(const char __user * altroot)
+{
+ char *emul = NULL;
+ int ret;
+
+ if (altroot) {
+ emul = getname(altroot);
+ if (IS_ERR(emul)) {
+ ret = PTR_ERR(emul);
+ goto out;
+ }
+ }
+
+ if (atomic_read(&current->fs->count) != 1) {
+ struct fs_struct *fsp, *ofsp;
+
+ fsp = copy_fs_struct(current->fs);
+ if (fsp == NULL) {
+ ret = -ENOMEM;
+ goto out_putname;
+ }
+
+ task_lock(current);
+ ofsp = current->fs;
+ current->fs = fsp;
+ task_unlock(current);
+
+ put_fs_struct(ofsp);
+ }
+
+ /*
+ * At that point we are guaranteed to be the sole owner of
+ * current->fs.
+ */
+
+ ret = __set_fs_altroot(emul);
+
+out_putname:
+ if (emul)
+ putname(emul);
+out:
+ return ret;
}

int fastcall path_lookup(const char *name, unsigned int flags, struct nameidata *nd)
===== include/asm-i386/unistd.h 1.43 vs edited =====
--- 1.43/include/asm-i386/unistd.h 2004-10-24 11:32:46 +01:00
+++ edited/include/asm-i386/unistd.h 2004-12-20 13:08:41 +00:00
@@ -290,7 +290,7 @@
#define __NR_mq_getsetattr (__NR_mq_open+5)
#define __NR_sys_kexec_load 283
#define __NR_waitid 284
-/* #define __NR_sys_setaltroot 285 */
+#define __NR_setaltroot 285
#define __NR_add_key 286
#define __NR_request_key 287
#define __NR_keyctl 288
===== include/asm-ia64/unistd.h 1.53 vs edited =====
--- 1.53/include/asm-ia64/unistd.h 2004-11-23 20:18:59 +00:00
+++ edited/include/asm-ia64/unistd.h 2004-12-20 13:08:03 +00:00
@@ -263,6 +263,7 @@
#define __NR_add_key 1271
#define __NR_request_key 1272
#define __NR_keyctl 1273
+#define __NR_setaltroot 1274

#ifdef __KERNEL__

===== include/asm-ppc/unistd.h 1.34 vs edited =====
--- 1.34/include/asm-ppc/unistd.h 2004-10-22 10:27:40 +01:00
+++ edited/include/asm-ppc/unistd.h 2004-12-20 13:08:03 +00:00
@@ -276,8 +276,9 @@
#define __NR_add_key 269
#define __NR_request_key 270
#define __NR_keyctl 271
+#define __NR_setaltroot 272

-#define __NR_syscalls 272
+#define __NR_syscalls 273

#define __NR(n) #n

===== include/asm-ppc64/unistd.h 1.36 vs edited =====
--- 1.36/include/asm-ppc64/unistd.h 2004-11-16 03:29:08 +00:00
+++ edited/include/asm-ppc64/unistd.h 2004-12-20 13:08:03 +00:00
@@ -282,8 +282,9 @@
#define __NR_add_key 269
#define __NR_request_key 270
#define __NR_keyctl 271
+#define __NR_setaltroot 272

-#define __NR_syscalls 272
+#define __NR_syscalls 273
#ifdef __KERNEL__
#define NR_syscalls __NR_syscalls
#endif
===== include/asm-sparc64/unistd.h 1.36 vs edited =====
--- 1.36/include/asm-sparc64/unistd.h 2004-10-24 10:47:46 +01:00
+++ edited/include/asm-sparc64/unistd.h 2004-12-20 13:09:06 +00:00
@@ -298,7 +298,7 @@
#define __NR_mq_notify 277
#define __NR_mq_getsetattr 278
#define __NR_waitid 279
-/*#define __NR_sys_setaltroot 280 available (was setaltroot) */
+#define __NR_setaltroot 280
#define __NR_add_key 281
#define __NR_request_key 282
#define __NR_keyctl 283
===== include/linux/fs.h 1.362 vs edited =====
--- 1.362/include/linux/fs.h 2004-10-29 09:14:03 +01:00
+++ edited/include/linux/fs.h 2004-12-20 13:08:03 +00:00
@@ -1376,7 +1376,7 @@
extern int may_open(struct nameidata *, int, int);

extern int kernel_read(struct file *, unsigned long, char *, unsigned long);
-extern struct file * open_exec(const char *);
+extern struct file * open_exec(const char *, int);

/* fs/dcache.c -- generic fs support functions */
extern int is_subdir(struct dentry *, struct dentry *);
===== include/linux/syscalls.h 1.17 vs edited =====
--- 1.17/include/linux/syscalls.h 2004-10-24 10:47:46 +01:00
+++ edited/include/linux/syscalls.h 2004-12-20 13:08:03 +00:00
@@ -490,6 +490,7 @@
void __user *res);
asmlinkage long sys_syslog(int type, char __user *buf, int len);
asmlinkage long sys_uselib(const char __user *library);
+asmlinkage long sys_setaltroot(const char __user *altroot);
asmlinkage long sys_ni_syscall(void);

asmlinkage long sys_add_key(const char __user *_type,