Re: extended attributes security problem

[Andi Kleen <ak@suse.de>]

On Sun, Apr 07, 2002 at 03:56:31AM -0800, Ethan Benson wrote:
> On Sun, Apr 07, 2002 at 01:16:19PM +0200, Andi Kleen wrote:
> > 
> > I'm proposing this patch. As Andreas pointed out it doesn't make much sense
> > to set ACLs on symlinks or special devices. I still allow it for root.
> 
> sounds reasonable.
> 
> however this patch is not sufficent, we must do something about world
> writable directories like /tmp, otherwise my exploit will still work
> fine, just target /tmp instead of /dev/null.

In /tmp you can create files anyways and they're usually not even counted 
to your quota (at least few sites seem to run quota in /tmp) 

> 
> i think that restricting creation of user.* attrs to the owner when
> the sticky bit is set is really the only sensible solution here, i
> agree its not ideal but as its been stated the meaning of the sticky
> bit is a bit of a hack anyway.

Hmm. Ok. Revised patch.  Also do not allow setting on sticky bit.


--- linux-work/fs/xattr.c-o	Thu Mar 21 18:15:26 2002
+++ linux-work/fs/xattr.c	Sun Apr  7 16:41:15 2002
@@ -63,10 +63,22 @@
 	int error;
 	void *kvalue;
 	char kname[XATTR_NAME_MAX + 1];
+	struct inode *i = d->d_inode; 
 
 	if (flags & ~(XATTR_CREATE|XATTR_REPLACE))
 		return -EINVAL;
 
+	/* Some checks to prevent people abusing EAs to get over their quota: */ 
+	/* Do not allow creation of EAs on special files and symlinks */
+	if (!S_ISREG(i->i_mode)  && !S_ISDIR(i->i_mode) &&	    
+		!capable(CAP_SYS_ADMIN))	
+		return -EPERM; 
+	/* Do not allow creation EAs in directories with sticky bit unless you're
+	   the owner. */ 	   
+	if (S_ISDIR(i->i_mode) && (i->i_mode & S_ISVTX) && 
+	    (current->fsuid != i->i_uid) && !capable(CAP_FOWNER))
+		return -EPERM; 
+
 	error = strncpy_from_user(kname, name, sizeof(kname));
 	if (error == 0 || error == sizeof(kname))
 		error = -ERANGE;
@@ -83,9 +95,9 @@
 	}
 
 	error = -EOPNOTSUPP;
-	if (d->d_inode->i_op && d->d_inode->i_op->setxattr) {
+	if (i->i_op && i->i_op->setxattr) {
 		lock_kernel();
-		error = d->d_inode->i_op->setxattr(d, kname, kvalue, size, flags);
+		error = i->i_op->setxattr(d, kname, kvalue, size, flags);
 		unlock_kernel();
 	}
 




-Andi