Modify extended attribute protection model to authorize based on

attribute namespace and DAC protection on file:
	- Attribute names beginning with '$' are in the system namespace
	- The attribute name "$" is reserved
	- System namespace attributes may only be read/set by suser()
	  or by kernel (cred == NULL)
	- Other attribute names are in the application namespace
	- The attribute name "" is reserved
	- Application namespace attributes are protected in the manner
	  of the target file permission

o Kernel changes
	- Add ufs_extattr_valid_attrname() to check whether the requested
	  attribute "set" or "enable" is appropriate (i.e., non-reserved)
	- Modify ufs_extattr_credcheck() to accept target file vnode, not
	  to take inode uid
	- Modify ufs_extattr_credcheck() to check namespace, then enforce
	  either kernel/suser for system namespace, or vaccess() for
	  application namespace
o EA backing file format changes
	- Remove permission fields from extended attribute backing file
	  header
	- Bump extended attribute backing file header version to 3
o Update extattrctl.c and extattrctl.8
	- Remove now deprecated -r and -w arguments to initattr, as
	  permissions are now implicit
	- (unrelated) fix error reporting and unlinking during failed
	  initattr to remove duplicate/inaccurate error messages, and to
	  only unlink if the failure wasn't in the backing file open()

Obtained from:	TrustedBSD Project

sys/ufs/ufs/extattr.h

@@ -33,7 +33,7 @@
 #define	_UFS_UFS_EXTATTR_H_
 
 #define	UFS_EXTATTR_MAGIC		0x00b5d5ec
-#define	UFS_EXTATTR_VERSION		0x00000002
+#define	UFS_EXTATTR_VERSION		0x00000003
 #define	UFS_EXTATTR_FSROOTSUBDIR	".attribute"
 #define	UFS_EXTATTR_MAXEXTATTRNAME	65	/* including null */
 
@@ -55,8 +55,6 @@ struct ufs_extattr_fileheader {
 	u_int	uef_magic;	/* magic number for sanity checking */
 	u_int	uef_version;	/* version of attribute file */
 	u_int	uef_size;	/* size of attributes, w/o header */
-	u_int	uef_read_perm;	/* permissions to read attribute */
-	u_int	uef_write_perm;	/* permissions to write attribute */
 };
 
 struct ufs_extattr_header {

sys/ufs/ufs/ufs_extattr.c

@@ -49,8 +49,10 @@
 
 static MALLOC_DEFINE(M_UFS_EXTATTR, "ufs_extattr", "ufs extended attribute");
 
-static int	ufs_extattr_credcheck(struct ufs_extattr_list_entry *uele,
+static int	ufs_extattr_valid_attrname(const char *attrname);
-    u_int32_t fowner, struct ucred *cred, struct proc *p, int access);
+static int	ufs_extattr_credcheck(struct vnode *vp,
+    struct ufs_extattr_list_entry *uele, struct ucred *cred, struct proc *p,
+    int access);
 static int	ufs_extattr_enable(struct ufsmount *ump, const char *attrname,
     struct vnode *backing_vnode, struct proc *p);
 static int	ufs_extattr_disable(struct ufsmount *ump, const char *attrname,
@@ -83,6 +85,28 @@ ufs_extattr_uepm_unlock(struct ufsmount *ump, struct proc *p)
 	lockmgr(&ump->um_extattr.uepm_lock, LK_RELEASE, 0, p);
 }
 
+/*
+ * Determine whether the name passed is a valid name for an actual
+ * attribute.
+ *
+ * Invalid currently consists of:
+ *         NULL pointer for attrname
+ *         zero-length attrname (used to retrieve application attr list)
+ *         attrname consisting of "$" (used to treive system attr list)
+ */
+static int
+ufs_extattr_valid_attrname(const char *attrname)
+{
+
+	if (attrname == NULL)
+		return (0);
+	if (strlen(attrname) == 0)
+		return (0);
+	if (strlen(attrname) == 1 && attrname[0] == '$')
+		return (0);
+	return (1);
+}
+
 /*
  * Locate an attribute given a name and mountpoint.
  * Must be holding uepm lock for the mount point.
@@ -199,6 +223,8 @@ ufs_extattr_enable(struct ufsmount *ump, const char *attrname,
 	struct uio	auio;
 	int	error = 0;
 
+	if (!ufs_extattr_valid_attrname(attrname))
+		return (EINVAL);
 	if (backing_vnode->v_type != VREG)
 		return (EINVAL);
 
@@ -280,6 +306,9 @@ ufs_extattr_disable(struct ufsmount *ump, const char *attrname, struct proc *p)
 	struct ufs_extattr_list_entry	*uele;
 	int	error = 0;
 
+	if (!ufs_extattr_valid_attrname(attrname))
+		return (EINVAL);
+
 	uele = ufs_extattr_find_attr(ump, attrname);
 	if (!uele)
 		return (ENOENT);
@@ -363,41 +392,27 @@ ufs_extattrctl(struct mount *mp, int cmd, const char *attrname,
  * permissions.
  */
 static int
-ufs_extattr_credcheck(struct ufs_extattr_list_entry *uele, u_int32_t fowner,
+ufs_extattr_credcheck(struct vnode *vp, struct ufs_extattr_list_entry *uele,
-		      struct ucred *cred, struct proc *p, int access)
+    struct ucred *cred, struct proc *p, int access)
 {
-	u_int	uef_perm;
+	int	system_namespace;
 
-	switch(access) {
+	system_namespace = (strlen(uele->uele_attrname) >= 1 &&
-	case IREAD:
+	    uele->uele_attrname[0] == '$');
-		uef_perm = uele->uele_fileheader.uef_read_perm;
-		break;
-	case IWRITE:
-		uef_perm = uele->uele_fileheader.uef_write_perm;
-		break;
-	default:
-		return (EACCES);
-	}
 
-	/* Kernel sponsoring request does so without passing a cred */
+	/*
-	if (!cred)
+	 * Kernel-invoked always succeeds
+	 */
+	if (cred == NULL)
 		return (0);
 
-	/* XXX there might eventually be a capability check here */
+	/*
-
+	 * XXX What capability should apply here?
-	/* If it's set to root-only, check for suser(p) */
+	 */
-	if (uef_perm == UFS_EXTATTR_PERM_ROOT && !suser(p))
+	if (system_namespace)
-		return (0);
+		return (suser_xxx(cred, p, PRISON_ROOT));
-
+	else
-	/* Allow the owner if appropriate */
+		return (VOP_ACCESS(vp, access, cred, p));
-	if (uef_perm == UFS_EXTATTR_PERM_OWNER && cred->cr_uid == fowner)
-		return (0);
-
-	/* Allow anyone if appropriate */
-	if (uef_perm == UFS_EXTATTR_PERM_ANYONE)
-		return (0);
-
-	return (EACCES);
 }
 
 /*
@@ -451,12 +466,16 @@ ufs_extattr_get(struct vnode *vp, const char *name, struct uio *uio,
 	if (!(ump->um_extattr.uepm_flags & UFS_EXTATTR_UEPM_STARTED))
 		return (EOPNOTSUPP);
 
+	if (strlen(name) == 0 || (strlen(name) == 1 && name[0] == '$')) {
+		/* XXX retrieve attribute lists */
+		return (EINVAL);
+	}
+
 	attribute = ufs_extattr_find_attr(ump, name);
 	if (!attribute)
 		return (ENOENT);
 
-	if ((error = ufs_extattr_credcheck(attribute, ip->i_uid, cred, p,
+	if ((error = ufs_extattr_credcheck(vp, attribute, cred, p, IREAD)))
-	    IREAD)))
 		return (error);
 
 	/*
@@ -613,16 +632,16 @@ ufs_extattr_set(struct vnode *vp, const char *name, struct uio *uio,
 
 	if (vp->v_mount->mnt_flag & MNT_RDONLY)
 		return (EROFS);
-
 	if (!(ump->um_extattr.uepm_flags & UFS_EXTATTR_UEPM_STARTED))
 		return (EOPNOTSUPP);
+	if (!ufs_extattr_valid_attrname(name))
+		return (EINVAL);
 
 	attribute = ufs_extattr_find_attr(ump, name);
 	if (!attribute)
 		return (ENOENT);
 
-	if ((error = ufs_extattr_credcheck(attribute, ip->i_uid, cred,
+	if ((error = ufs_extattr_credcheck(vp, attribute, cred, p, IWRITE)))
-	    p, IWRITE)))
 		return (error);
 
 	/*
@@ -718,16 +737,16 @@ ufs_extattr_rm(struct vnode *vp, const char *name, struct ucred *cred,
 
 	if (vp->v_mount->mnt_flag & MNT_RDONLY)  
 		return (EROFS);
-
 	if (!(ump->um_extattr.uepm_flags & UFS_EXTATTR_UEPM_STARTED))
 		return (EOPNOTSUPP);
+	if (!ufs_extattr_valid_attrname(name))
+		return (EINVAL);
 
 	attribute = ufs_extattr_find_attr(ump, name);
 	if (!attribute)
 		return (ENOENT);
 
-	if ((error = ufs_extattr_credcheck(attribute, ip->i_uid, cred, p,
+	if ((error = ufs_extattr_credcheck(vp, attribute, cred, p, IWRITE)))
-	    IWRITE)))
 		return (error);
 
 	/*

usr.sbin/extattrctl/extattrctl.8

@@ -41,8 +41,6 @@
 .Nm extattrctl
 .Cm initattr
 .Op Fl p Ar path
-.Op Fl r Ar kroa
-.Op Fl w Ar kroa
 .Ar attrsize
 .Ar attrfile
 .Nm extattrctl
@@ -63,7 +61,7 @@ as well as initialization of attribute backing files, and enabling and
 disabling of specific extended attributes on a file system.
 .Pp
 The first argument on the command line indicates the operation to be
-performend.  Operation must be one of the following:
+performed.  Operation must be one of the following:
 .Bl -tag -width indent
 .It Cm start Ar path
 Start extended attribute support on the file system named using
@@ -77,8 +75,6 @@ Extended attribute support must previously have been started.
 .It Xo
 .Cm initattr
 .Op Fl p Ar path
-.Op Fl r Ar kroa
-.Op Fl w Ar kroa
 .Ar attrsize attrfile
 .Xc
 Create and initialize a file to use as an attribute backing file.
@@ -95,25 +91,6 @@ This has the advantage of guaranteeing that space will be available
 for attributes when they are written, preventing low disk space conditions
 from denying attribute service.
 .Pp
-The
-.Fl r
-and
-.Fl w
-options can be used to set the read and write permissions on the named
-attribute, respectively.
-There are four levels possible for both read and write:
-.Dq k
-limits reading or writing to the kernel,
-.Dq r
-limits activities to root,
-.Dq o
-limits activities to root and the owner of the file having the attribute
-read or written, and
-.Dq q
-allows any user to perform the attribute operation.
-The default is to limit activities to the root user, or
-.Dq r .
-.Pp
 This file should not exist before running
 .Cm initattr.
 .It Cm enable Ar path Ar attrname Ar attrfile
@@ -145,9 +122,8 @@ Start extended attributes on the root file system.
 .Dl extattrctl initattr 17 /.attribute/md5
 .Pp
 Create an attribute backing file in /.attribute/md5, and set the maximum
-size of each attribute to 17 bytes.  Sparse files are used for storing the
+size of each attribute to 17 bytes, with a sparse file used for storing
-attributes, and the default permissions limiting access to the root user
+the attributes.
-are implied.
 .Pp
 .Dl extattrctl enable / md5 /.attribute/md5
 .Pp

usr.sbin/extattrctl/extattrctl.c

@@ -54,41 +54,12 @@ usage(void)
 	    "usage:\n"
 	    "  extattrctl start [path]\n"
 	    "  extattrctl stop [path]\n"
-	    "  extattrctl initattr [-p path] [-r [kroa]] [-w [kroa]] "
+	    "  extattrctl initattr [-p path] [attrsize] [attrfile]\n"
-	    "[attrsize] [attrfile]\n"
 	    "  extattrctl enable [path] [attrname] [attrfile]\n"
 	    "  extattrctl disable [path] [attrname]\n");
 	exit(-1);
 }
 
-/*
- * Return a level, or -1
- */
-int
-extattr_level_from_string(char *string)
-{
-
-	if (strlen(string) != 1)
-		return (-1);
-
-	switch(string[0]) {
-	case 'k':
-	case 'K':
-		return (UFS_EXTATTR_PERM_KERNEL);
-	case 'r':
-	case 'R':
-		return (UFS_EXTATTR_PERM_ROOT);
-	case 'o':
-	case 'O':
-		return (UFS_EXTATTR_PERM_OWNER);
-	case 'a':
-	case 'A':
-		return (UFS_EXTATTR_PERM_ANYONE);
-	default:
-		return (-1);
-	}
-}
-
 long
 num_inodes_by_path(char *path)
 {
@@ -111,8 +82,6 @@ initattr(int argc, char *argv[])
 	char	*fs_path = NULL;
 	char	*zero_buf = NULL;
 	long	loop, num_inodes;
-	int	initattr_rlevel = UFS_EXTATTR_PERM_ROOT;
-	int	initattr_wlevel = UFS_EXTATTR_PERM_ROOT;
 	int	ch, i, error;
 
 	optind = 0;
@@ -121,16 +90,6 @@ initattr(int argc, char *argv[])
 		case 'p':
 			fs_path = strdup(optarg);
 			break;
-		case 'r':
-			initattr_rlevel = extattr_level_from_string(optarg);
-			if (initattr_rlevel == -1)
-				usage();
-			break;
-		case 'w':
-			initattr_wlevel = extattr_level_from_string(optarg);
-			if (initattr_wlevel == -1)
-				usage();
-			break;
 		case '?':
 		default:
 			usage();
@@ -146,8 +105,6 @@ initattr(int argc, char *argv[])
 	if ((i = open(argv[1], O_CREAT | O_EXCL | O_WRONLY, 0600)) != -1) {
 		uef.uef_magic = UFS_EXTATTR_MAGIC;
 		uef.uef_version = UFS_EXTATTR_VERSION;
-		uef.uef_write_perm = initattr_wlevel;
-		uef.uef_read_perm = initattr_rlevel;
 		uef.uef_size = atoi(argv[0]);
 		if (write(i, &uef, sizeof(uef)) == -1)
 			error = -1;
@@ -170,7 +127,12 @@ initattr(int argc, char *argv[])
 			}
 		}
 	}
-	if (i == -1 || error == -1) {
+	if (i == -1) {
+		/* unable to open file */
+		perror(argv[1]);
+		return (-1);
+	}
+	if (error == -1) {
 		perror(argv[1]);
 		unlink(argv[1]);
 		return (-1);
@@ -191,29 +153,33 @@ main(int argc, char *argv[])
 		if (argc != 3)
 			usage();
 		error = extattrctl(argv[2], UFS_EXTATTR_CMD_START, 0, 0);
+		if (error)
+			perror("extattrctl start");
 	} else if (!strcmp(argv[1], "stop")) {
 		if (argc != 3)
 			usage();
 		error = extattrctl(argv[2], UFS_EXTATTR_CMD_STOP, 0, 0);
+		if (error)
+			perror("extattrctl stop");
 	} else if (!strcmp(argv[1], "enable")) {
 		if (argc != 5)
 			usage();
 		error = extattrctl(argv[2], UFS_EXTATTR_CMD_ENABLE, argv[3],
 		    argv[4]);
+		if (error)
+			perror("extattrctl enable");
 	} else if (!strcmp(argv[1], "disable")) {
 		if (argc != 4)
 			usage();
 		error = extattrctl(argv[2], UFS_EXTATTR_CMD_DISABLE, argv[3],
 		    NULL);
+		if (error)
+			perror("extattrctl disable");
 	} else if (!strcmp(argv[1], "initattr")) {
 		argc -= 2;
 		argv += 2;
 		error = initattr(argc, argv);
 	} else
 		usage();
-
-	if (error)
-		perror(argv[1]);
-
 	return(error);
 }