Re: Chroot bug

[Miloslav Semler <majkls@prepere.com>]

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

Kyle Moffett napsal(a):
> On Sep 26, 2007, at 06:27:38, David Newall wrote:
>> Kyle Moffett wrote:
>>> David, please do tell myself and Adrian how "locking down" chroot() 
>>> the way you want will avoid letting root break out through any of 
>>> the above ways?
>>
>> As has been said, there are thousands of ways to break out of a 
>> chroot.  It's just that one of them should not be that chroot lets 
>> you walk out.  I can't explain it clearer than that.  If you don't 
>> see it now you probably never will.
>
> Let me put it this way:  You *CANNOT* enforce chroot() the way you 
> want to without a completely unacceptable performance penalty.  Let's 
> start with the simplest example of:
>
> fd = open("/", O_DIRECTORY);
> chroot("/foo");
> fchdir(fd);
> chroot(".");
>
> If you had ever actually looked at the Linux VFS, it is completely 
> *impossible* to tell whether "fd" at the time of the chroot is inside 
> or outside of "/foo" without tracking an enormous amount of extra state.
so there *is* solution. It is possible. I solved it. I have patch and it 
is working. So if you find some way how to break it I woud glad if you 
tell me it.
> Even then, any such determination may not be valid since an FD may be 
> opened to an inode which is hardlinked at multiple locations in the 
> directory tree.  It could also be bind-mounted at multiple locations, 
> or it may not even be mounted at all in this namespace (CDROM that was 
> lazy-unmounted).  That FD may be later passed over an open UNIX-domain 
> socket from another process.  Moreover, arbitrarily closing FDs would 
> break a huge number of programs.  Furthermore, since you can't fix the 
> "trivial" case of 'fchdir()', then there's no point in even 
> *attempting* to fix the "cwd is outside of chroot" problem, although 
> that is basically equivalent in difficulty to fixing the "dir-fd is 
> outside of chroot" problem.
>
> As for the nested-chroot() bit, the root user inside of a chroot is 
> always allowed to chroot().  This is necessary for test-suites for 
> various distro installers, chroot once to enter the installer playpen, 
> installer chroots again to configure the test-installed-system.  Once 
> you allow a second chroot, you're back at the "can't reliably and 
> efficiently track directory sub-tree members" problem.
>
> So if you think it can and should be fixed, then PROVIDE THE CODE.
Miloslav Semler

[-- Attachment #2: sys_chroot+sys_fchdir.patch --]
[-- Type: text/x-diff, Size: 3265 bytes --]

diff -Nrp linux-2.6.16.53/fs/namei.c linux-2.6.16.53-new/fs/namei.c
*** linux-2.6.16.53/fs/namei.c	2007-07-25 23:05:45.000000000 +0200
--- linux-2.6.16.53-new/fs/namei.c	2007-09-15 16:13:50.000000000 +0200
*************** static __always_inline void follow_dotdo
*** 728,733 ****
--- 728,772 ----
  	}
  	follow_mount(&nd->mnt, &nd->dentry);
  }
+ long directory_is_out(struct vfsmount *wdmnt, struct dentry *wdentry,
+ 		struct vfsmount *rootmnt, struct dentry *root)
+ {
+ 	struct nameidata oldentry, newentry;
+ 	long ret = 1;
+ 	
+         read_lock(&current->fs->lock);
+ 	oldentry.dentry = dget(wdentry);
+ 	oldentry.mnt = mntget(wdmnt);
+         read_unlock(&current->fs->lock);
+ 	newentry.dentry = oldentry.dentry;
+ 	newentry.mnt = oldentry.mnt;
+ 	
+ 	follow_dotdot(&newentry);
+ 	/* check it */
+ 	if(newentry.dentry == root && 
+ 		newentry.mnt == rootmnt){
+ 		ret = 0;
+ 		goto out;
+ 	}
+ 	
+ 	while(oldentry.mnt != newentry.mnt ||
+ 		oldentry.dentry != newentry.dentry){
+ 		
+ 		memcpy(&oldentry, &newentry, sizeof(struct nameidata));
+ 		follow_dotdot(&newentry);
+ 		
+ 		/* check it */
+ 		if(newentry.dentry == root && 
+ 			newentry.mnt == rootmnt){
+ 			ret = 0;
+ 			goto out;
+ 		}
+ 	}
+ out:
+ 	dput(newentry.dentry);
+ 	mntput(newentry.mnt);
+ 	return ret;
+ }
  
  /*
   *  It's more convoluted than I'd like it to be, but... it's still fairly
diff -Nrp linux-2.6.16.53/fs/open.c linux-2.6.16.53-new/fs/open.c
*** linux-2.6.16.53/fs/open.c	2007-07-25 23:05:45.000000000 +0200
--- linux-2.6.16.53-new/fs/open.c	2007-09-15 16:14:52.000000000 +0200
*************** dput_and_out:
*** 560,565 ****
--- 560,567 ----
  out:
  	return error;
  }
+ long directory_is_out(struct vfsmount *, struct dentry*, 
+ 		struct vfsmount *, struct dentry *);
  
  asmlinkage long sys_fchdir(unsigned int fd)
  {
*************** asmlinkage long sys_fchdir(unsigned int 
*** 581,586 ****
--- 583,591 ----
  	error = -ENOTDIR;
  	if (!S_ISDIR(inode->i_mode))
  		goto out_putf;
+ 	if (directory_is_out(mnt, dentry, current->fs->rootmnt, 
+ 				current->fs->root))
+ 		goto out_putf;
  
  	error = file_permission(file, MAY_EXEC);
  	if (!error)
*************** out:
*** 594,600 ****
  asmlinkage long sys_chroot(const char __user * filename)
  {
  	struct nameidata nd;
! 	int error;
  
  	error = __user_walk(filename, LOOKUP_FOLLOW | LOOKUP_DIRECTORY | LOOKUP_NOALT, &nd);
  	if (error)
--- 599,605 ----
  asmlinkage long sys_chroot(const char __user * filename)
  {
  	struct nameidata nd;
! 	int error, set_wd = 0;
  
  	error = __user_walk(filename, LOOKUP_FOLLOW | LOOKUP_DIRECTORY | LOOKUP_NOALT, &nd);
  	if (error)
*************** asmlinkage long sys_chroot(const char __
*** 607,615 ****
--- 612,631 ----
  	error = -EPERM;
  	if (!capable(CAP_SYS_CHROOT))
  		goto dput_and_out;
+ 	error = -ENOTDIR;
+ 	/*
+ 	if (directory_is_out(nd.mnt, nd.dentry, current->fs->rootmnt,
+ 				current->fs->root))
+ 		goto dput_and_out;
+ 		*/
+ 	set_wd = directory_is_out(current->fs->pwdmnt, current->fs->pwd, 
+ 				nd.mnt, nd.dentry);
  
  	set_fs_root(current->fs, nd.mnt, nd.dentry);
  	set_fs_altroot();
+ 	/* if wd is out, reset it to . */
+ 	if(set_wd)
+ 		set_fs_pwd(current->fs, nd.mnt, nd.dentry);
  	error = 0;
  dput_and_out:
  	path_release(&nd);