AppArmor security module overview

This post contains patches to include the AppArmor application security framework, with request for inclusion.

The patch series consists of four areas:

(1) Pass struct vfsmount through to LSM hooks.

Tony Jones has posted almost all of these patches here before on February 5; the feedback given has been incorporated.

(2) Fixes and improvements to d_path:

(a) make it unambiguous and exclude unreachable paths from /proc/mounts,

(b) make its result consistent in the face of remounts,

(c) introduce d_namespace_path(), a variant of d_path that goes up to the namespace root instead of the chroot.

Part (a) has been in the -mm tree for more than a month; this series includes a copy of the -mm patch. Parts (b) and (c) shouldn't be too controversial.

(3) Be able to distinguish file descriptor access from access by name in LSM hooks.

Applications expect different behavior from file descriptor accesses and accesses by name in some cases. We need to pass this information down the LSM hooks to allow AppArmor to tell which is which.

(4) The AppArmor LSM itself.

(See below.)

A tarball of the kernel patches, base user-space utilities, example profiles, and technical documentation (including a walk-through) are available at:

http://forgeftp.novell.com//apparmor/LKML_Submission-April_07/

The rest of this introduction gives a brief overview of AppArmor's overall design. We will try to address the concerns people had when AppArmor was previously posted here in April 06 in a follow-up post.

AppArmor's Overall Design
=========================

AppArmor protects systems from vulnerable software by confining processes, giving them "least privilege" access to the system's
resources: with least privilege, processes are allowed exactly what they need, nothing more, and nothing less. Systems are thus protected from bugs in applications that would lead to privilege escalation, such as remote system access because of a buffer overflow in a web server, etc.

AppArmor does this by defining application profiles which list allowed accesses, and assigning those profiles to processes. AppArmor does *not* require the user to confine all processes on the system. Rather, you need to provide AppArmor profiles for every process that is potentially subject to manipulation by the attacker. For instance, to defend against network attack, confine all process that access the network.

The corollary to this is that attacks against AppArmor that start with "assume some unconfined process does ..." are outside the AppArmor threat model. Any process that might do something malicious to an AppArmor system should be confined by an AppArmor profile.

The kernel manages many different kinds of resources. AppArmor currently controls access to two key resources among them: files, and POSIX Capabilities. (Additional protection for things like rlimits, interprocess communication, and network access are being worked on, and are expected to become available in a future version.)

File Access Control
-------------------

Application profiles control file access by pathname: each profile contains a list of fully qualified pathnames (potentially containing globbing) and associated access modes: read (r), write (w), different kinds of execute (ix, Px, px, Ux, ux), create hard-link (l), and memory map for execution (m).

For example, the following two rules permit read access to any file below /srv/www/htdocs/**.html, and memory map for execution (m), execute inheriting the current profile (ix), and read (r) access to /usr/sbin/suexec2:

    /srv/www/htdocs/**.html r,
    /usr/sbin/suexec2 mixr,

POSIX Capabilities
------------------

Application profiles enumerate the POSIX capabilites that a process may use. (A process must have a capability both in its effective capabilities set and in its profile in order to use it. See the capabilities(7) manual page for details on capabilities.)

For example, the following rule allows processes to chroot(2):

capability sys_chroot,

An Example Profile
------------------

Here is an example profile for ntpd:

    /usr/sbin/ntpd {
      #include <abstractions/base>
      #include <abstractions/nameservice>

      capability ipc_lock,
      capability net_bind_service,
      capability setgid,
      capability setuid,
      capability sys_chroot,
      capability sys_resource,
      capability sys_time,

      /drift/ntp.drift rwl,
      /drift/ntp.drift.TEMP rwl,
      /etc/ntp.conf r,
      /etc/ntp/drift* rwl,
      /etc/ntp/keys r,
      /etc/ntp/step-tickers r,
      /tmp/ntp* rwl,
      /usr/sbin/ntpd rmix,
      /var/lib/ntp/etc/ntp.conf.iburst  r,
      /var/lib/ntp/drift rwl,
      /var/lib/ntp/drift.TEMP rwl,
      /var/lib/ntp/drift/ntp.drift r,
      /var/lib/ntp/var/run/ntp/ntpd.pid w,
      /var/log/ntp w,
      /var/log/ntp.log w,
      /var/run/ntpd.pid w,
    }

Briefly, the /usr/sbin/ntpd in line 1 tells AppArmor to use this profile for /usr/sbin/ntpd. The include lines pull in some predefined abstractions containing frequently used access patterns which allow to keep profiles short, and the capability and file access rules have been explained above.

Explaining the AppArmor design in detail would take by far too much space here, so let me refer you to the technical documentation for that. Included is a low-level walk-through of the system and basic tools, and some examples. The manual pages included in the apparmor-parser package are worth a read as well.

This is needed for computing pathnames in the AppArmor LSM.

---
 fs/namei.c               |    2 +-
 include/linux/security.h |    9 ++++++---
 security/dummy.c         |    2 +-
 security/selinux/hooks.c |    3 ++-
 4 files changed, 10 insertions(+), 6 deletions(-)

@@ -1503,7 +1503,7 @@ int vfs_create(struct inode *dir, struct
 		return -EACCES;	/* shouldn't it be ENOSYS? */
 	mode &= S_IALLUGO;
 	mode |= S_IFREG;
-	error = security_inode_create(dir, dentry, mode);
+	error = security_inode_create(dir, dentry, nd ? nd->mnt : NULL, mode);
 	if (error)
 		return error;
 	DQUOT_INIT(dir);

@@ -283,6 +283,7 @@ struct request_sock;
  *	Check permission to create a regular file.
  *	@dir contains inode structure of the parent of the new file.
  *	@dentry contains the dentry structure for the file to be created.
+ *	@mnt is the vfsmount corresponding to @dentry (may be NULL).
  *	@mode contains the file mode of the file to be created.
  *	Return 0 if permission is granted.
  * @inode_link:

@@ -1204,8 +1205,8 @@ struct security_operations {
 	void (*inode_free_security) (struct inode *inode);
 	int (*inode_init_security) (struct inode *inode, struct inode *dir,
 				    char **name, void **value, size_t *len);
-	int (*inode_create) (struct inode *dir,
-	                     struct dentry *dentry, int mode);
+	int (*inode_create) (struct inode *dir, struct dentry *dentry,
+			     struct vfsmount *mnt, int mode);
 	int (*inode_link) (struct dentry *old_dentry,
 	                   struct inode *dir, struct dentry *new_dentry);
 	int (*inode_unlink) (struct inode *dir, struct dentry *dentry);

@@ -1611,11 +1612,12 @@ static inline int security_inode_init_se
 	
 static inline int security_inode_create (struct inode *dir,
 					 struct dentry *dentry,
+					 struct vfsmount *mnt,
 					 int mode)
 {
 	if (unlikely (IS_PRIVATE (dir)))
 		return 0;
-	return security_ops->inode_create (dir, dentry, mode);
+	return security_ops->inode_create (dir, dentry, mnt, mode);
 }
 
 static inline int security_inode_link (struct dentry *old_dentry,

@@ -2338,6 +2340,7 @@ static inline int security_inode_init_se
 	
 static inline int security_inode_create (struct inode *dir,
 					 struct dentry *dentry,
+					 struct vfsmount *mnt,
 					 int mode)
 {
 	return 0;

@@ -265,7 +265,7 @@ static int dummy_inode_init_security (st
 }
 
 static int dummy_inode_create (struct inode *inode, struct dentry *dentry,
-			       int mask)
+			       struct vfsmount *mnt, int mask)
 {
 	return 0;
 }

@@ -2178,7 +2178,8 @@ static int selinux_inode_init_security(s
 	return 0;
 }
 
-static int selinux_inode_create(struct inode *dir, struct dentry *dentry, int mask)
+static int selinux_inode_create(struct inode *dir, struct dentry *dentry,
+				 struct vfsmount *mnt, int mask)
 {
 	return may_create(dir, dentry, SECCLASS_FILE);
 }

I don't consider it fair to NACK this patch just because some other part of the kernel uses vfs_create() in the wrong way -- those are really independent issues. The bug is not that hard to fix though; here is a proposed patch on top of the other AppArmor patches.

The offenders are nfsd and the mqueue filesystem. Instantiate a struct nameidata there.

The .dentry and .mnt fields can easily be filled in in nfsd. The mqueue filesystem is somewhat special: files that mq_open creates are on an internal mount and the mqueue filesystem is not generally visible (it may or may not be mounted). When passing a regular vfsmount to vfs_create the files would appear disconnected while they actually are kernel-internal objects. Use a NULL vfsmount there to avoid that -- passing the kernel-internal vfsmount there wouldn't help, anyway.

---
 fs/namei.c    |    7 ++++---
 fs/nfsd/vfs.c |   23 +++++++++++++++++++----
 ipc/mqueue.c  |    6 +++++-
 3 files changed, 28 insertions(+), 8 deletions(-)

@@ -1503,7 +1503,7 @@ int vfs_create(struct inode *dir, struct
 		return -EACCES;	/* shouldn't it be ENOSYS? */
 	mode &= S_IALLUGO;
 	mode |= S_IFREG;
-	error = security_inode_create(dir, dentry, nd ? nd->mnt : NULL, mode);
+	error = security_inode_create(dir, dentry, nd->mnt, mode);
 	if (error)
 		return error;
 	DQUOT_INIT(dir);

@@ -1108,6 +1108,18 @@ nfsd_commit(struct svc_rqst *rqstp, stru
 }
 #endif /* CONFIG_NFSD_V3 */
 
+static inline int
+nfsd_do_create(struct inode *dir, struct dentry *child, struct vfsmount *mnt,
+	       int mode)
+{
+	struct nameidata nd = {
+		.dentry = child,
+		.mnt = mnt,
+	};
+
+	return vfs_create(dir, child, mode, &nd);
+}
+
 /*
  * Create a file (regular, directory, device, fifo); UNIX sockets 
  * not yet implemented.

@@ -1192,7 +1204,8 @@ nfsd_create(struct svc_rqst *rqstp, stru
 	err = 0;
 	switch (type) {
 	case S_IFREG:
-		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
+		host_err = nfsd_do_create(dirp, dchild, exp->ex_mnt,
+					  iap->ia_mode);
 		break;
 	case S_IFDIR:
 		host_err = vfs_mkdir(dirp, dchild, exp->ex_mnt, iap->ia_mode);

@@ -1253,6 +1266,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	        int *truncp, int *created)
 {
 	struct dentry	*dentry, *dchild = NULL;
+	struct svc_export *exp;
 	struct inode	*dirp;
 	__be32		err;
 	int		host_err;

@@ -1271,6 +1285,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 		goto out;
 
 	dentry = fhp->fh_dentry;
+	exp = fhp->fh_export;
 	dirp = dentry->d_inode;
 
 	/* Get all the sanity checks out of the way before

@@ -1288,7 +1303,7 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 	if (IS_ERR(dchild))
 		goto out_nfserr;
 
-	err = fh_compose(resfhp, fhp->fh_export, dchild, fhp);
+	err = fh_compose(resfhp, exp, dchild, fhp);
 	if (err)
 		goto out;
 

@@ -1334,13 +1349,13 @@ nfsd_create_v3(struct svc_rqst *rqstp, s
 		goto out;
 	}
 
-	host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
+	host_err = nfsd_do_create(dirp, dchild, exp->ex_mnt, iap->ia_mode);
 	if (host_err < 0)
 		goto out_nfserr;
 	if (created)
 		*created = 1;
 
-	if (EX_ISSYNC(fhp->fh_export)) {
+	if (EX_ISSYNC(exp)) {
 		err = nfserrno(nfsd_sync_dir(dentry));
 		/* setattr will sync the child (or not) */
 	}

@@ -604,6 +604,10 @@ static int mq_attr_ok(struct mq_attr *at
 static struct file *do_create(struct dentry *dir, struct dentry *dentry,
 			int oflag, mode_t mode, struct mq_attr __user *u_attr)
 {
+	struct nameidata nd = {
+		.dentry = dentry,
+		/* Not a regular create, so set .mnt to NULL. */
+	};
 	struct mq_attr attr;
 	int ret;
 

@@ -619,7 +623,7 @@ static struct file *do_create(struct den
 	}
 
 	mode &= ~current->fs->umask;
-	ret = vfs_create(dir->d_inode, dentry, mode, NULL);
+	ret = vfs_create(dir->d_inode, dentry, mode, &nd);
 	dentry->d_fsdata = NULL;
 	if (ret)
 		goto out;

Note that it's not just this particular hunk, all these kinds of conditional passing are wrong.

But anyway, creating fake nameidata structures is not really helpful. If there is a nameidata passed people expect it to be complete, and if you pass them to an LSM people will e.g. try to look into lookup intents. But similar to the per-mountpoint r/o work I suspect you're simply operating on the wrong level, but then again this might not be fixable due to the braindead apparmor design.

I don't actually agree with that: when nfsd creates a file, it still is a file create no matter where it originates from, and so it does make sense to provide the appropriate intent information too. Struct nameidata contains other crap only needed during an actual lookup too --- that's a mess, and the nameidata2 patch shows one possibility how to deal with it. (It's the cleanest approach I could think of right now without cluttering the vfs code with insane amounts of dereferences.)

Thanks,
Andreas

You should provide intent information, yes - which your patch didn't :)

And yes, I didn't like doing the ugly intent in nameidata hack, it's creating loads of problems for various parties, e.g. the stackable filesystem folks. Now the basic intent in nameidata mistaken has been made even worse by passing back a struct file in it conditionally and doing lots of work in ->lookup that shouldn't be there. (Which btw, I expect to cause quite a few problems for apparmor or other lsms, but I guess so far no one has tried them on NFSv4)

Well, the information implicitly provided is "no intent": In do_create() in ipc/mqueue.c intents would be pretty pointless because the mqueue filesystem is local. In fs/nfsd/vfs.c, intents would make slightly more sense assuming that the underlying filesystem eported via nfsd is remote. That's an optimization, and not even a very important one.

Pathname wise, NFSv4 should look like any other filesystem on the client side. On the Server side, the concept of pathnames doesn't really fly for nfs: if a directory contains more than one link to the same file, there is no way to tell those aliases from each other from the file descriptor. In addition, computing even the somewhat ambiguous pathnames that can be computed would require subtree checking. But trying to confine nfsd is pretty pointless
anyway: the deamon is privileged and can do whatever it wants. It makes more sense to export the right directories with the right permissions in the first place.

Thanks,
Andreas

Yes, the vfs uses or passes through a NULL nameidata or vfsmount pointer and no intent information in several places:

(1) vfs_create() is called with a NULL nameidata in two places,

(2) lookup_one_len() and lookup_one_len_kern() pass a NULL nameidata to

     permission(), d_op->d_revalidate(), and i_op->lookup(),

(3) file_permission() passes a NULL nameidata to permission().

(4) permission() is directly called with NULL nameidata in some places, and

In general it is incorrect to use NULL nameidata or vfsmounts because then the vfsmount->mnt_flags cannot be checked, and no intent information is available. Intents are an optional optimization for remote filesystems; we can simply ignore them for local filesystems. There are some cases where we are passing NULL which are real bugs, but there are also other cases in which the operations are not mount related: for example, filesystems like pipefs have the MS_NOUSER flag set which indicates that they cannot be mounted at all. On filesystems like sysfs, files are created and removed whether or not that filesystem is mounted.

The places where we should pass a proper nameidata / vfsmount but don't should be fixed. Those are long-standing issues in the vfs though, and at least some are not easy to correct. AppArmor is affected by some of the NULL vfsmount passing, and we found a few more problems when looking into this more closely, but most of that NULL passing doesn't affect AppArmor:

* In some places, vfs functions are called without nameidata / vfsmount, followed by calling lsm hooks with a vfsmount. In those cases, the vfs functions cannot check the vfsmount flags, but the lsm hooks will still do the appropriate checks.

* In the AppArmor security model, directory searching is always allowed, and the access check are done once the leaf dentry + vfsmount has been looked up. For example, granting write access to /var/tmp/foo in a profile is sufficient to allow that access; search access to /var and /var/tmp does not need to be specified. (The regular inode permission checks are of course still performed.)

* For filesystems like nfsd, the concept of pathname based access control doesn't make sense because of properties of the protocol: there is no reliable mapping from an nfs file handle to a particular file; aliases to the same file cannot be distinguished. (The AppArmor techdoc [1] describes this in more detail.)

Not allowing to confine nfsd by AppArmor doesn't hurt much: nfsd provides its own export access control mechanisms. Also, nfsd cannot really be confined in a secure way because of how it is implemented in the kernel. Gfs2 may have similar properties; I didn't actually look into the protocol.

So I propose to separate the vfs NULL nameidata / vfsmount passing discussion from the AppArmor discussion. I have no problem working on the vfs fixes -- we could some nice cleanups there -- but that really is independent of AppArmor.

We almost have the revised AppArmor patches ready. We'll include all the fixes that are truly necessary, and everything beyond that will be posted separately.

Splitting up nameidata helps somewhat here. In cases where we don't have intent information available we can zero out the intent flags, and so in the worst case, we won't get the intent optimizations.

Thanks,
Andreas

Here is a patch with request for comment.

The create, lookup, and permission inode operations are all passed a full nameidata. This is unfortunate because in nfsd and the mqueue filesystem we must instantiate a struct nameidata, but cannot provide all of the same information of a regular lookup. The unused fields take up space on the stack, but more importantly, it is not obvious which fields have meaningful values and which don't, and so things might easily break.

This patch introduces struct nameidata2 with only the fields that make sense independent of an actual lookup, and uses that struct in those places where a full nameidata is not needed.

The entire patch is a little big so I'm only including the key parts here. The complete version is here:

http://forgeftp.novell.com/apparmor/LKML_Submission-April_07/split-up-nameidata.diff

@@ -14,21 +14,39 @@ struct open_intent {
 
 enum { MAX_NESTED_LINKS = 8 };
 
+/**
+ * Fields shared between nameidata and nameidata2 -- nameidata2 could
+ * be embedded in nameidata, but then the vfs code would become
+ * cluttered with dereferences.
+ */
+#define __NAMEIDATA2				\
+	struct dentry	*dentry;		\
+	struct vfsmount *mnt;			\
+	unsigned int	flags;			\
+						\
+	union {					\
+		struct open_intent open;	\
+	} intent;
+
 struct nameidata {
-	struct dentry	*dentry;
-	struct vfsmount *mnt;
+	__NAMEIDATA2
 	struct qstr	last;
-	unsigned int	flags;
 	int		last_type;
 	unsigned	depth;
 	char *saved_names[MAX_NESTED_LINKS + 1];
+};
 
-	/* Intent data */
-	union {
-		struct open_intent open;
-	} intent;
+struct nameidata2 {
+	__NAMEIDATA2
 };
 
+#undef __NAMEIDATA2
+
+static inline struct nameidata2 *ND2(struct nameidata *nd)
+{
+	return container_of(&nd->dentry, struct nameidata2, dentry);
+}
+
 struct path {
 	struct vfsmount *mnt;
 	struct dentry *dentry;

@@ -76,10 +94,9 @@ extern void path_release_on_umount(struc
 
 extern int __user_path_lookup_open(const char __user *, unsigned lookup_flags, struct nameidata *nd, int open_flags);
 extern int path_lookup_open(int dfd, const char *name, unsigned lookup_flags, struct nameidata *, int open_flags);
-extern struct file *lookup_instantiate_filp(struct nameidata *nd, struct dentry *dentry,
-		int (*open)(struct inode *, struct file *));
+extern struct file *lookup_instantiate_filp(struct nameidata2 *nd, struct dentry *dentry, int (*open)(struct inode *, struct file *));
 extern struct file *nameidata_to_filp(struct nameidata *nd, int flags);
-extern void release_open_intent(struct nameidata *);
+extern void release_open_intent(struct nameidata2 *);
 
 extern struct dentry * lookup_one_len(const char *, struct dentry *, int);
 

@@ -225,7 +225,7 @@ int generic_permission(struct inode *ino
 	return -EACCES;
 }
 
-int permission(struct inode *inode, int mask, struct nameidata *nd)
+int permission(struct inode *inode, int mask, struct nameidata2 *nd)
 {
 	umode_t mode = inode->i_mode;
 	int retval, submask;

@@ -278,7 +278,7 @@ int permission(struct inode *inode, int 
  * for filesystem access without changing the "normal" uids which
  * are used for other things.
  */
-int vfs_permission(struct nameidata *nd, int mask)
+int vfs_permission(struct nameidata2 *nd, int mask)
 {
 	return permission(nd->dentry->d_inode, mask, nd);
 }

@@ -366,7 +366,7 @@ void path_release_on_umount(struct namei
  * release_open_intent - free up open intent resources
  * @nd: pointer to nameidata
  */
-void release_open_intent(struct nameidata *nd)
+void release_open_intent(struct nameidata2 *nd)
 {
 	if (nd->intent.open.file->f_path.dentry == NULL)
 		put_filp(nd->intent.open.file);

@@ -377,7 +377,7 @@ void release_open_intent(struct nameidat
 static inline struct dentry *
 do_revalidate(struct dentry *dentry, struct nameidata *nd)
 {
-	int status = dentry->d_op->d_revalidate(dentry, nd);
+	int status = dentry->d_op->d_revalidate(dentry, ND2(nd));
 	if (unlikely(status <= 0)) {
 		/*
 		 * The dentry failed validation.

@@ -455,7 +455,7 @@ static int exec_permission_lite(struct i
 
 	return -EACCES;
 ok:
-	return security_inode_permission(inode, MAY_EXEC, nd);
+	return security_inode_permission(inode, MAY_EXEC, ND2(nd));
 }
 
 /*

@@ -491,7 +491,7 @@ static struct dentry * real_lookup(struc
 		struct dentry * dentry = d_alloc(parent, name);
 		result = ERR_PTR(-ENOMEM);
 		if (dentry) {
-			result = dir->i_op->lookup(dir, dentry, nd);
+			result = dir->i_op->lookup(dir, dentry, ND2(nd));
 			if (result)
 				dput(dentry);
 			else

@@ -832,7 +832,7 @@ static fastcall int __link_path_walk(con
 		nd->flags |= LOOKUP_CONTINUE;
 		err = exec_permission_lite(inode, nd);
 		if (err == -EAGAIN)
-			err = vfs_permission(nd, MAY_EXEC);
+			err = vfs_permission(ND2(nd), MAY_EXEC);
  		if (err)
 			break;
 

@@ -978,7 +978,8 @@ return_reval:
 		    (nd->dentry->d_sb->s_type->fs_flags & FS_REVAL_DOT)) {
 			err = -ESTALE;
 			/* Note: we do not d_invalidate() */
-			if (!nd->dentry->d_op->d_revalidate(nd->dentry, nd))
+			if (!nd->dentry->d_op->d_revalidate(nd->dentry,
+							    ND2(nd)))
 				break;
 		}
 return_base:

@@ -1194,7 +1195,7 @@ static int __path_lookup_intent_open(int
 			path_release(nd);
 		}
 	} else if (err != 0)
-		release_open_intent(nd);
+		release_open_intent(ND2(nd));
 	return err;
 }
 

@@ -1255,7 +1256,7 @@ static struct dentry * __lookup_hash(str
 	int err;
 
 	inode = base->d_inode;
-	err = permission(inode, MAY_EXEC, nd);
+	err = permission(inode, MAY_EXEC, ND2(nd));
 	dentry = ERR_PTR(err);
 	if (err)
 		goto out;

@@ -1277,7 +1278,7 @@ static struct dentry * __lookup_hash(str
 		dentry = ERR_PTR(-ENOMEM);
 		if (!new)
 			goto out;
-		dentry = inode->i_op->lookup(inode, new, nd);
+		dentry = inode->i_op->lookup(inode, new, ND2(nd));
 		if (!dentry)
 			dentry = new;
 		else

@@ -1422,7 +1423,7 @@ static int may_delete(struct inode *dir,
  *  4. We can't do it if dir is immutable (done in permission())
  */
 static inline int may_create(struct inode *dir, struct dentry *child,
-			     struct nameidata *nd)
+			     struct nameidata2 *nd)
 {
 	if (child->d_inode)
 		return -EEXIST;

@@ -1492,7 +1493,7 @@ void unlock_rename(struct dentry *p1, st
 }
 
 int vfs_create(struct inode *dir, struct dentry *dentry, int mode,
-		struct nameidata *nd)
+		struct nameidata2 *nd)
 {
 	int error = may_create(dir, dentry, nd);
 

@@ -1528,7 +1529,7 @@ int may_open(struct nameidata *nd, int a
 	if (S_ISDIR(inode->i_mode) && (flag & FMODE_WRITE))
 		return -EISDIR;
 
-	error = vfs_permission(nd, acc_mode);
+	error = vfs_permission(ND2(nd), acc_mode);
 	if (error)
 		return error;
 

@@ -1601,7 +1602,7 @@ static int open_namei_create(struct name
 
 	if (!IS_POSIXACL(dir->d_inode))
 		mode &= ~current->fs->umask;
-	error = vfs_create(dir->d_inode, path->dentry, mode, nd);
+	error = vfs_create(dir->d_inode, path->dentry, mode, ND2(nd));
 	mutex_unlock(&dir->d_inode->i_mutex);
 	dput(nd->dentry);
 	nd->dentry = path->dentry;

@@ -1734,7 +1735,7 @@ exit_dput:
 	dput_path(&path, nd);
 exit:
 	if (!IS_ERR(nd->intent.open.file))
-		release_open_intent(nd);
+		release_open_intent(ND2(nd));
 	path_release(nd);
 	return error;
 

@@ -1762,7 +1763,7 @@ do_link:
 		 * me so stupid? Anathema to whoever designed this non-sense
 		 * with "intent.open".
 		 */
-		release_open_intent(nd);
+		release_open_intent(ND2(nd));
 		return error;
 	}
 	nd->flags &= ~LOOKUP_PARENT;

@@ -1888,7 +1889,7 @@ asmlinkage long sys_mknodat(int dfd, con
 		switch (mode & S_IFMT) {
 		case 0: case S_IFREG:
 			error = vfs_create(nd.dentry->d_inode, dentry, mode,
-					   &nd);
+					   ND2(&nd));
 			break;
 		case S_IFCHR: case S_IFBLK:
 			error = vfs_mknod(nd.dentry->d_inode, dentry, nd.mnt,

@@ -289,6 +289,7 @@ extern int dir_notify_enable;
 struct hd_geometry;
 struct iovec;
 struct nameidata;
+struct nameidata2;
 struct kiocb;
 struct pipe_inode_info;
 struct poll_table_struct;

@@ -981,8 +982,8 @@ extern void unlock_super(struct super_bl
 /*
  * VFS helper functions..
  */
-extern int vfs_permission(struct nameidata *, int);
-extern int vfs_create(struct inode *, struct dentry *, int, struct nameidata *);
+extern int vfs_permission(struct nameidata2 *, int);
+extern int vfs_create(struct inode *, struct dentry *, int, struct nameidata2 *);
 extern int vfs_mkdir(struct inode *, struct dentry *, struct vfsmount *, int);
 extern int vfs_mknod(struct inode *, struct dentry *, struct vfsmount *, int, dev_t);
 extern int vfs_symlink(struct inode *, struct dentry *, struct vfsmount *, const char *, int);

@@ -1106,8 +1107,8 @@ struct file_operations {
 };
 
 struct inode_operations {
-	int (*create) (struct inode *,struct dentry *,int, struct nameidata *);
-	struct dentry * (*lookup) (struct inode *,struct dentry *, struct nameidata *);
+	int (*create) (struct inode *,struct dentry *,int, struct nameidata2 *);
+	struct dentry * (*lookup) (struct inode *,struct dentry *, struct nameidata2 *);
 	int (*link) (struct dentry *,struct inode *,struct dentry *);
 	int (*unlink) (struct inode *,struct dentry *);
 	int (*symlink) (struct inode *,struct dentry *,const char *);

@@ -1120,7 +1121,7 @@ struct inode_operations {
 	void * (*follow_link) (struct dentry *, struct nameidata *);
 	void (*put_link) (struct dentry *, struct nameidata *, void *);
 	void (*truncate) (struct inode *);
-	int (*permission) (struct inode *, int, struct nameidata *);
+	int (*permission) (struct inode *, int, struct nameidata2 *);
 	int (*setattr) (struct dentry *, struct iattr *);
 	int (*getattr) (struct vfsmount *mnt, struct dentry *, struct kstat *);
 	int (*setxattr) (struct dentry *, const char *,const void *,size_t,int);

@@ -1616,7 +1617,7 @@ extern int do_remount_sb(struct super_bl
 extern sector_t bmap(struct inode *, sector_t);
 #endif
 extern int notify_change(struct dentry *, struct vfsmount *, struct iattr *);
-extern int permission(struct inode *, int, struct nameidata *);
+extern int permission(struct inode *, int, struct nameidata2 *);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));
 

@@ -1873,7 +1874,7 @@ extern int simple_prepare_write(struct f
 extern int simple_commit_write(struct file *file, struct page *page,
 				unsigned offset, unsigned to);
 
-extern struct dentry *simple_lookup(struct inode *, struct dentry *, struct nameidata *);
+extern struct dentry *simple_lookup(struct inode *, struct dentry *, struct nameidata2 *);
 extern ssize_t generic_read_dir(struct file *, char __user *, size_t, loff_t *);
 extern const struct file_operations simple_dir_operations;
 extern const struct inode_operations simple_dir_inode_operations;

you could use an anonymous struct embedeed in both.

Sigh, kernel-doc should improve things, not get in the way ...

Well, I see it as sort of a language that is embedded in C source code.

But what are you suggesting?

~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Which is an argument against said LSM in current form.

The fundamental model of AppArmor is to perform access checks based on the location of a file in the filesystem namespace, i.e., the pathname, and this can only be done with both the dentry and vfsmount. My understanding was that there was agreement at the last kernel summit that pathname based approaches should be allowed.

No. There are callers of vfs_create() that use a NULL nameidata, and that's what causes the problem here. Struct nameidata is pretty big, and so we don't want to allocate temporary nameidata objects all over the place. So to me the above NULL check seems the lesser evil.

One way to deal with the nameidata size problem is to split it up into one part for the real path lookups, and a much smaller part that only contains the dentry, vfsmount, and intent flags. This would allow to pass around the smaller nameidata much more consistently.

John has posted patches for that on May 14; subject [RFD Patch 0/4]. Feedback appreciated.

In several places, the NULL nameidata is wrong because we can't check the vfsmount flags or intent. Not having this information is causing problems for nfs too for example -- it's not an AppArmor specific problem.

This is backwards from what AppArmor does. The policy defines which paths may be accessed; all paths not explicitly listed are denied. If files are mounted at multiple locations, then the policy may allow access to some locations but not to others. That's not a hole.

In fact this is not much different from traditional permissions on parent
directories: even if the same files are mounted at several locations, parent directory permissions may allow accessing only some of those locations.

Thanks,
Andreas

I don't know what else you'd call it.

Would you mind providing some concrete examples of how such a model would be useful?

- James

James Morris
<[email protected]>

AppArmor doesn't label files; it's a different model from SELinux. Its policy defines which processes may access which paths. Even if for some reson the same files were visible elsewhere, the policy wouldn't cover those other paths, and so accessing them would be denied. So again, that's not a security hole.

The model is explained, with examples, in the technical documentation at
http://forgeftp.novell.com//apparmor/LKML_Submission-May_07/.

Thanks,
Andreas

Hello.

I think bind mounts were discussed when shared subtree ( http://lwn.net/Articles/159092/ ) was introduced.

For systems that allow users mount their CD/DVDs freely, bind mounts are used and labeling files is a convenient way to deny accessing somebody else's files.

But systems that don't allow users mount their CD/DVDs freely, bind mounts needn't to be used and using pathnames is a convenient way to deny accessing somebody else's files.

Pathname based access control/auditing system
works if the system doesn't use bind mounts.

However, there are distributions (e.g. Debian Etch) that always use bind mounts. In such distributions, pathname based access control/auditing system doesn't work.

This is not the fault of distributions nor
pathname based access control/auditing system.
It is possible to solve by passing vfsmount to VFS and LSM functions.

SELinux users are having a lot of trouble because pathnames in audit logs are not always complete.
AppArmor users are having a lot of trouble because pathnames which a process requested are ambiguous when bind mounts are used.

Being able to report pathnames that a process requested is not surprising when considering user friendliness.
I beleive passing vfsmount makes both users happy.

Thanks.

I'm asking specifically about a model where you'd want to provide differing access to the same objects based on the pathname used for access to the objects, per:

"If files are mounted at multiple locations, then the policy may allow
access to some locations but not to others. That's not a hole."

I don't see specific examples of this kind of policy in that document, and the document seems to be saying quite the opposite -- that the AA policy is only valid in conjunction with further constraints on how the objects may be accessed:

Section 3.2:

"Pathnames are meaningful only within a namespace. Each namespace has a
root where all the files, directories, and mount points are hanging off from.

The privilege of creating new namespaces is bound to the CAP_ SYS_ ADMIN capability, which grants a multitude of other things that would allow a process to break out of AppArmor confinement, so confined processes are not supposed to have this privilege, and processes with this capability need to be considered trusted. "

I can restate my question and ask why you'd want a security policy like:

Subject 'sysadmin' has:
read access to /etc/shadow
read/write access to /views/sysadmin/etc/shadow

where the objects referenced by the paths are identical and visible to the subject along both paths, in keeping with your description of "policy may allow access to some locations but not to others" ?

- James

James Morris
<[email protected]>

I guess I see what you are getting at. If a user has read/write access to /views/sysadmin/etc/shadow and if that is an alias for /etc/shadow, then of course it doesn't help to give the user only read access to /etc/shadow to protect that file. So you can shoot yourself in the foot with mounts, and also with hardlinks. That's why confined processes are not allowed to mount stuff, and why hardlinks requires the appropriate profile permissions.

AppArmor depends on the namespace so it cares about namespace manipulations, just like SELinux depends on labels so labels matter there.

Read it like this: we don't have a good idea how to support multiple namespaces so far. Currently, we interpret all pathnames relative to the namespace a process is in. Confined processes don't have the privilege to create or manipulate namespaces, which makes this safe. We may find a better future solution.

I'm not aware of situations where giving different permissions to different paths to the same file would actually be useful. The security model doesn't prevent it though, and it's not a security hole.

Andreas

You also don't have a solution for multiple chroot jails, since they often have the same fs mounted in many places on given box *and* since the pathnames from the confined processes' POVs have fsck-all to do with each other.

It's really not kinder than multiple namespaces as far as your approach is concerned.

Well, the pathnames we check against are namespace relative, so no matter what pathnames the chrooted processes think they are looking at, we always know the actual pathnames up to ``outside the chroot''. Having the same filesystem mounted in multiple chroots or in multiple locations in the same chroot doesn't matter.

The main problem I see when it comes to defining per-namespace policy is that namespaces are inherently anonymous, and there is no obvious way of associating different sets of profiles with different namespaces. Implementing something to also handle multiple namespaces doesn't seem hard -- after all, it's not such a difficult concept -- but I don't have a good enough idea what would work best.

Thanks,
Andreas

On Fedora zcat, gzip and gunzip are all links to the same file. I can imagine (although it is a bit of a stretch) allowing a set of users access to gunzip but not gzip (or the other way around). There are probably more sophisticated programs that have different behavior based on the name they're invoked by that would provide a more compelling arguement, assuming of course that you buy into the behavior-based-on-name scheme. What I think I'm suggesting is that AppArmor might be useful in addressing the fact that a file with multiple hard links is necessarily constrained to have the same access control on each of those names. That assumes one believes that such behavior is flawwed, and I'm not going to try to argue that. The question was about an example, and there is one.

Casey Schaufler
[email protected]

Different policy for different names of the same binary makes more obvious sense with chroot environments. That's slightly different from having different permissions for the same file within a single profile though.

Thanks,
Andreas

This doesn't work. The behavior depends on argv[0], which is not necessarily the same as the name of the file.

Jeremy Maitin-Shepard

Sorry, but I don't understand your objection. If AppArmor is configured to allow everyone access to /bin/gzip but only some people access to /bin/gunzip and (important detail) the single binary uses argv[0] as documented and (another important detail) there aren't other links named gunzip to the binary (ok, that's lots of if's) you should be fine. I suppose you could make a shell that lies to exec, but the AppArmor code could certainly check for that in exec by enforcing the argv[0] convention. It would be perfectly reasonable for a system that is so dependent on pathnames to require that.

Casey Schaufler
[email protected]

Well, my point was exactly that App Armor doesn't (as far as I know) do anything to enforce the argv[0] convention, nor would it in general prevent a confined program from making a symlink or hard link. Even disregarding that, it seems very fragile in general to make an suid program (there would be no point in confining the execution of a non-suid program) perform essentially access control based on argv[0].

Jeremy Maitin-Shepard

Note that by "confining the execution of a non-suid program", I mean defining an App Armor profile that prevents the execution of a particular non-suid program, unless of course the program file itself contains secret information, which is irrelevant to this discussion.

Jeremy Maitin-Shepard

Sounds like an opportunity for improvement then.

I think that you're being generous calling it fragile, but that's my view, and I've seen much worse. I agree that it would be a Bad Idea, but the fact that I think it's a bad idea is not going to prevent very many people from trying it, and for those that do try it name based access control might seem like just the ticket to complete their nefarious schemes. Remember that security is a subjective thing, and using argv[0] and AppArmor together might make some people feel better.

Casey Schaufler
[email protected]

Hello.

The argv[0] defines the default behavior of hard linked or symbolic linked programs, but the behavior can be overridden using commandline options.
If you want to allow access to /bin/gzip but deny access to /bin/gunzip ,
you also need to deny access to "/bin/gzip -d" "/bin/gzip --decompress" "/bin/gzip --uncompress". It is impossible to do so because options to override the default behavior depends on program's design and you can't know
what programs and what options are there in the system.
Even if you know all programs and all options in the system,
it is a too tough job to find and reject options
that override the default behavior in the kernel space.

There are (I think) three types of program invocation.

(1) Invocation of hard linked programs.

/bin/gzip and /bin/gunzip and /bin/zcat are hard links.

There is no problem because you can know which pathname was requested using d_namespace_path() with "struct linux_binprm"->file .

(2) Invocation of symbolic linked programs.

/sbin/pidof is a symbolic link to /sbin/killall .

There is a problem because you can't know which pathname was requested using d_namespace_path() with "struct linux_binprm"->file because the symbolic links were already derefernced inside open_exec(). To know which pathname was requested, you need to lookup using "struct linux_binprm"->filename without LOOKUP_FOLLOW and then use d_namespace_path().
Although there is a race condition that the pathname
the symbolic link "struct linux_binprm"->filename points to may change, but it is inevitable because you can't get dentry and vfsmount of both "without LOOKUP_FOLLOW flag" and "with LOOKUP_FOLLOW flag" at the same time.

(3) Invocation of dynamically created programs with random names.

/usr/sbin/logrotate creates files patterned /tmp/logrotate.?????? and executes these dynamically created files.

To keep execution of these dynamically created files under control, you need to aggregate pathnames of these files.
AppArmor can't define profile if the pathname of programs is random, can it?

Usually the argv[0] and the "struct linux_binprm"->filename are the same, but if you want to do something with argv[0], you will need to handle the (2) case to see whether the argv[0] and "struct linux_binprm"->filename are the same.

Thanks.

Jeez, what argv[0] convention are you both talking about? argv[0] is not guaranteed to have any association with the name of the executable. Feel free to have any discussion about argv[0] you want, but *please* keep it away from AppArmor, which really has nothing to do with it.

It would be nice if you could stop calling argv[0] checks ``name-based access control'': from the point of view of the kernel no access control is involved, and even application-level argv[0] based access control makes no sense whatsoever.

Thanks,
Andreas

"The first argument, by convention, should point to the
file name associated with the file being executed."

since the man page calls it a convention, so do I.

As I pointed out, if you wanted to trust the argv[0] value (which I understand AppArmor makes no claims about) and you wanted to use the argv[0] value to determine application behavior (which several people claim is a Bad Idea) you could use Name Based Access Control to provide different access to the common binary. As I pointed out before, that's a lot of "if's".

Fair enough, I don't believe that an argv[0] check ought to be used as a security mechanism. I am not convinced that everyone would agree with us.

Casey Schaufler
[email protected]

Having seen my share of argv[0]-related security bugs in my years, I have to agree that it's a security crock. As to why some might not agree, you already put your finger on it earlier:

Some people would rather just feel better...

Hmm ... that's a strange idea. AppArmor cannot assume anything about argv[0], and it would be a really bad idea to change the well-established semantics of argv[0].

There is no actual need for looking at argv[0], though: AppArmor decides based on the actual pathname of the executable...

Thanks,
Andreas

Yeah, I get that a lot.

Right. My point was that if you wanted to use the gzip/gunzip example of a file with two names being treated differently based on the name accessed as an argument for AppArmor you could. If you don't want to, that's ok too. Jeremy raised a reasonable objection, and AppArmor could address it if y'all chose to do so. I seriously doubt that enforcing the argv[0] convention would break much, and I also expect that if it did there's a Consultant's Retirement to be made fixing the security hole it points out.

Casey Schaufler
[email protected]

AppArmor detects the pathname of the file exec'd at the time the parent exec's it, and not anything inside the child involving argv[0].

As such, AA can detect whether you did exec("gzip") or exec("gunzip") and apply the policy relevant to the program. It could apply different policies to each of them, so whether it has access to /tmp/mumble/barf depends on whether you called it 'gzip' or 'gunzip'. Caveat: it makes no sense to profile either gzip or gunzip in the AppArmor model, so I won't defend what kind of policy you would put on them.

Finally, AA doesn't care what the contents of the executable are. We assume that it is a copy of metasploit or something, and confine it to access only the resources that the policy says.

AppArmor does address it, and I hope this explains how we detect which of multiple hard links to a file you used to access the file without mucking about with argv[0].

Crispin

Crispin Cowan, Ph.D.               http://crispincowan.com/~crispin/
Director of Software Engineering   http://novell.com
		   Security: It's not linear

That's not actually useful for programs which link the same binary to multiple names because if you don't consider argv[0] as well I can run /usr/bin/gzip passing argv[0] of "gunzip" and get one set of policies and the other set of behaviour.

And then we have user added hardlinks of course.

I partially agree. Taken together with the policy of the calling process, things suddenly start to make more sense though (even if gzip/gunzip don't make good examples): if only allowed to execute /usr/bin/gzip, the calling process can still get the gunzip behavior, but it will be bound by the /usr/bin/gzip policy.

Controlling the policy is what we really care about; this limits the allowed behavior. We cannot really control the behavior of an application anyway (think of bugs alone), but we can set the bounds for that behavior.

Yes, allowing confined processes to change what they are allowed to execute under a more permissive policy is not such a good idea.

Thanks,
Andreas

As long as these resources are only files. There is no confinement beyond that.

- James

James Morris
<[email protected]>

That is a COMPLETE straw-man argument. I can override your "check" with this absolutely trivial perl code:

exec { "/usr/bin/gunzip" } "gzip", "-9", "some/file/to.gz";

Pathname-based checks are pretty fundamentally insecure. If you want to protect a "name", then you should tag the "name" with security attributes (IE: AppArmor). On the other hand, if you actually want to protect the _data_, then tagging the _name_ is flawed; tag the *DATA* instead.

Cheers,
Kyle Moffett

The above Perl code executes /usr/bin/gunzip and sets argv[0] to "gzip", so this confirms that the value of argv[0] is arbitrary. Well great, we already knew.

Please don't mix apples and pears. Some utilities change their behavior based on argv[0], which indeed is not secure. It doesn't have to be for those utilities' needs. This is different from pathname-based access control, which is based on the pathname of the binary actually being executed.

AppArmor does not look at argv[0] for anything, and doing so would be insane. So please don't jump to the wrong conclusions.

You can have application-level argv[0] checks in the binary that is hardlinked to /bin/gzip, /bin/gunzip, /bin/zcat. At the same time, the kernel knows which of these three versions it is executing, so from that point of view, they could be as well be separate copies, and different policy can be defined for each one of them. (This is ignoring the fact that for small utilities like gzip, profile inheritance is usually more useful than defining separate profiles.)

Thanks,
Andreas

Hello.

I agree that argv[0] checking is different from pathname-based access control or label-based access control, but I want to say argv[0] checking is still needed.

If you don't check argv[0], an attacker can request everything like

exec { "/bin/ls" } "/sbin/busybox", "cat", "/etc/shadow"; exec { "/bin/ls" } "/sbin/busybox", "rm", "/etc/shadow";

if /bin/ls and /bin/cat and /bin/rm are hardlinks of /sbin/busybox (e.g. embedded systems).

Therefore, TOMOYO Linux checks the combination of filename and argv[0] passed to execve().

Thanks.

So you are indeed trying to control the value of argv[0]? Well, good luck with that, but it's totally insane. You are guaranteed to break some applications. AppArmor is not going to go down that route.

If /bin/cat and /bin/rm are binaries or hardlinks to the same busybox binary (rather than symlinks), different profiles could be used for each of them. The cat profile could have no more than read access anywhere, so this would prevent it from removing files. That's an effective access control mechanism.

Conspiring with busybox in the kernel and making it do the right thing (and just that) at the application level is not an effective security mechanism:
the kernel would still allow busybox to do anything, and there could be bugs in the application, busybox's calling convention could change to allow more than one command per invocation (e.g., ``cat /etc/shadow ; rm /etc/shadow''), etc.

Andreas

Hello.

TOMOYO Linux ristricts argv[0] using allow_argv0 syntax.
"allow_argv0 /bin/bash -bash" to allow passing "/bin/bash" to filename and "-bash" to argv[0] . "allow_argv0 /bin/gzip gunzip" to allow passing "/bin/gzip" to filename and "gunzip" to argv[0] . "allow_argv0 /sbin/busybox cat" to allow passing "/sbin/busybox" to filename and "cat" to argv[0] . No need to use allow_argv0 syntax if the basename of filename and basename of argv[0] are the same (i.e. "allow_argv0 /bin/bash bash" is not required).
TOMOYO Linux doesn't unconditionally forbid passing different values for filename and argv[0]. TOMOYO Linux allows passing different values for filename and argv[0] only if it is allowed by allow_argv0 syntax. Could you please explain me why this approach breaks applications?

It is true if all processes are kept under control (e.g. strict policy in SELinux). If there is a process that is not kept under control (e.g. targeted policy in SELinux), you can't protect the application.
For example, an administrator may wish to allow users run /bin/ls without applying profiles because /bin/ls won't read/write the content of files. But a malicious user may pass "/bin/ls" to filename and "rm" to argv[0] and "/etc/shadow" to argv[1].
A malicious user may pass "/bin/ls" to filename and "/usr/sbin/httpd" to argv[0], resulting behave as /usr/sbin/httpd without applying profiles for /usr/sbin/httpd .

Thanks.

Alright, so it's configurable. This reduces it from being broken to being a truly bad idea.

No, forget that line of reasoning. As soon as you run anything unconfined that isn't trusted, you have basically lost control. Relying on the fact that /bin/ls won't do any harm is assuming to much. Features might exist that allow it to be abused, new features might get added, or there may be bugs. Just don't do it.

Andreas

One of my servers runs 3 different instances of the "kadmind" Kerberos daemon, one for each realm which I need to be able to modify/change-passwords/etc. In order to differentiate and stop/restart the appropriate daemon, I have a simple starter script which runs each kadmind process with a unique name derived from the realm (EG: "kadmind(EXAMPLE.COM)", "kadmind(OTHER.EXAMPLE.COM)"). Since this is a Kerberos server I use a very strict SELinux-based policy, yet my management tools need to be able to easily add and remove realms in a secure fashion. It sounds like TOMOYO Linux would not be able to handle this situation at all; I would either have to completely turn off that security "feature" and lose most of the functionality of TOMOYO Linux, or hard-code the list of realms into the policy file and have to completely reload policy every time I need to add/remove realms (big gaping security hole).

Cheers,
Kyle Moffett

Bingo.

(This is how traditional Unix DAC has always functioned, and is what SELinux does: object labeling).

- James

James Morris
<[email protected]>

2007/5/27, James Morris <[email protected]>:

Object labeling (or labeled security) looks simple and straight forward way, but it's not.

(1) Object labeling has a assumption that labels are always properly defined and maintained. This can not be easily achieved.
(2) Also, assigning a label is something like inventing and assigning a *new* name (label name) to objects which can cause flaws.

I'm not saying labeled security or SELinux is wrong. I just wanted to remind that the important part is the "process" not the "result". :-)

Toshiharu Harada
[email protected]

That's a circular argument, and a fairly trivial one at that. If you can't properly manage your labels, then how do you expect any security at all? If you can't manage your "labels", then pathname-based security won't work either. This is analogous to saying "Pathname-based security has an assumption that path-permissions are always properly defined and maintained", which is equally obvious. If you can't achieve the first with reasonable security, then you probably can't achieve the second either. Also, if you can't manage correct object labeling then I'm very interested in how you are maintaining secure Linux systems without standard DAC.

I don't understand how assigning new attributes to objects "can cause flaws", nor what flaws those might be; could you elaborate further? In particular, I don't see how this is really all that more complicated than defining additional access control in apache .htaccess files. The principle is the same: by stacking multiple independent security-verification mechanisms (Classical UNIX DAC and Apache permissions) you can increase security, albeit at an increased management cost. You might also note that ".htaccess" files are yet another form of successful "label-based" security; the security context for a directory depends on the .htaccess "label" file found within. The *exact* same principles apply to SELinux: you add additional attributes backed by a simple and powerful state-machine. The cross-checks are lower-level than those from .htaccess files, but the principles are the same.

Cheers,
Kyle Moffett

Unfortunately, it's not at all as simple as all that. Toshiharu is quite correct that it isn't always easy to actually implement. Consider how many ad-croc usages of 'restorecon' are needed to get a Fedora SELinux box through rc.sysinit:

% grep restorecon /etc/rc.sysinit
if [ -n "$SELINUX_STATE" -a -x /sbin/restorecon ] && LC_ALL=C fgrep -q " /dev " /proc/mounts ; then
/sbin/restorecon -R /dev 2>/dev/null
REBOOTFLAG=restorecon -v /sbin/init
[ -n "$SELINUX_STATE" ] && restorecon /dev/pts >/dev/null 2>&1
[ -n "$SELINUX_STATE" ] && restorecon /dev/mapper /dev/mapper/control >/dev/null 2>&1

                        [ -n "$SELINUX_STATE" -a -e "$path" ] && restorecon -R "$path"
                                [ -n "$SELINUX_STATE" -a -e "$path" ] && restorecon -R "$path"
                                [ -n "$SELINUX_STATE" -a -e "$path" ] && restorecon -R "$path"

restorecon /etc/mtab /etc/ld.so.cache /etc/blkid/blkid.tab /etc/resolv.conf >/dev/null 2>&1
[ -n "$SELINUX_STATE" ] && restorecon /tmp
[ -n "$SELINUX_STATE" ] && restorecon /tmp/.ICE-unix >/dev/null 2>&1

And that's just getting the system up to single-user. Things like sendmail and sshd require more restorecon handholding in their rc.init files.

Or just look at the creeping horror that is 'restorecond' (in particular, consider that the default restorcond.conf contains the strings '~/public_html' and '~/.mozilla/plugins/libflashplayer.so'. Yee. Frikkin. Hah. ;)

While I don't think restorecon is really necessary to properly boot SELinux-enabled (I've got a Debian box with some heavily customized policy which does so just fine), I am of the opinion that adding a "name" parameter to the file/directory create actions would be useful. For example, with such support you could actually specify a type-transition rule conditional on a specific name or substring:

named_type_transition sshd_t tmp_t:sock_file prefix "ssh-" ssh_sock_t;

Useful options for matching would be "prefix", "suffix", "substr(start,len)". "regex" would be nice but is sorta computationally intensive and would be likely to cause more problems than it solves.

These can go away if you modify your policy and pass "-o fscontext=system_u:object_r:dev_t" to the mount command for the /dev tmpfs, changing both the filesystem and the default directory labels from the default "system_u:object_r:tmpfs_t". This will work as long as your policy files contain appropriate transitions from the dev_t type.

I believe these are to handle rebooting from non-SELinux mode. There are two solutions to this kind of problem:
(1) Failing the boot if the labels are wrong
(2) Fixing the labels (and rebooting if necessary)
It would appear that FC does the latter, although for certain high-security systems (such as firewalls handling classified/confidential data), the first option is the only acceptable one.

I think this can be handled with some combination of appropriate SELinux policy and mount options. At least, I don't need this in the boot scripts on my heavily customized Debian SELinux box.

I don't know what the point of this generic line is; but I certainly don't have anything of the sort on my test system, and it appears to work just fine.

Part of the reason that Fedora has a large quantity of that restorecon and restorecond crap is that there is a certain amount of broken binary software needing executable stack/heap (such as flashplayer), programs without comprehensive or complete policies, or programs which by definition need extra support for SELinux.

For example, to really complete the SELinux model you need editors and tools which understand security labels the same way they understand UNIX permissions. With a bit of vim scripting I can probably make it run external commands to read file labels before it reads the file itself and modify /proc/self/attr/fscreate before writing out the file, similar to the way it would keep track of the standard DAC permissions on files it modifies.

There's also the fact that corporate products have fixed release schedules so remaining bugs in each release tend to get papered over instead of fixed properly (such as the restorecon in FC init-scripts). I haven't seen many problems with the SELinux model which aren't associated with working around buggy software or missing features.

Cheers,
Kyle Moffett

Hi!

Could someone implement this? AFAICT that prevents SELinux from being superset of AppArmor... Doing this should be significantly simpler than whole AA, and hopefully it will end up less ugly, too.

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Really it would need to extend all action-match items with new "named_" equivalents, and most callbacks would need to be extended to pass in an object name, if available. On the other hand, with such support implemented then the AppArmor policy compilation tools could be transformed into a simple SELinux policy generator. I estimate that the number of new lines of kernel code for such a modified SELinux would be 100x less than the kernel code in AppArmor.

Cheers,
Kyle Moffett

2007/5/27, Kyle Moffett <[email protected]>:

Sorry Kyle, I don't think it's a trivial one. The opposite.

Please read my message again. I didn't say, "This can never be achieved". I said, "This can not be easily achieved".

Yes,! You got the point.
Both label-based and pathname-based apporaches have the advantaes and difficluties.
In that sense they are equal. That's what I wanted to say. Both approaches can be improved and even can be used combined to overcome the difficulties. Let's not fight against and think together, then we can make Linux better.

I'm very interested in how you can know that you have the correct object labeling (this is my point). Could you tell? I assume your best efforts end up with
- have a proper fc definitions and guard them (this can be done)
- executes relabeling as needed (best efforts)
- hope everything work fine

I don't deny DAC at all. If we deny DAC, we can't live with Linux it's the base. MAC can be used to cover the shortages of DAC and Linux's simple user model, that's it.

Inode combined label is guaranteed to be a single at any point time. This is the most noticeable advantage of label-based security. But writing policy with labels are somewhat indirect way (I mean, we need "ls -Z" or "ps -Z"). Indirect way can cause flaw so we need a lot of work that is what I wanted to tell.

Cheers,
Toshiharu Harada
[email protected]

How is that argument not trivially circular? "Foo has an assumption that foo-property is always properly defined and maintained." That could be said about *anything*:
* Unix permissions have an assumption that mode bits are always
properly defined and maintained
* Apache .htaccess security has an assumtion that .htaccess files
are always properly defined and maintained.
* Functional email communication has an assumption that the email
servers are always properly defined and maintained

So you said "(data labels) can not be easily achieved". My question for you is: How do you manage secure UNIX systems without standard UNIX permission bits? Also: If you have problems with data labels then what makes pathname based labels "easier"? If there is something that could be done to improve SELinux and make it more readily configurable then it should probably be done.

I know that I have the correct object labeling because:
1) I rewrote/modified the default policy to be extremely strict on
the system where I wanted the extra security and hassle.
2) I ensured that the type transitions were in place for almost
everything that needed to be done to administer the system.
3) I wrote a file-contexts file and relabeled *once*
4) I loaded the customized policy plus policy for restorecon and
relabeled for the last time
5) I reloaded the customized policy without restorecon privileges
and without the ability to reload the policy again.
6) I never reboot the system without enforcing mode.
7) If there are unexpected errors or files have incorrect labels,
I have to get the security auditor to log in on the affected system and relabel the problematic files manually (rare occurrence which requires excessive amounts of paperwork).

I would argue that pathname-based security breaks the "simplicity is the best virtue (of a security system)" paradigm, because it attributes multiple potentially-conflicting labels to the same piece of data. It also cannot protect the secrecy of specific *data* as well as SELinux can. For example: In SELinux MLS a system could mark customer credit-card data as the "cust_private_info" category and it would be completely impossible for any program without the "cust_private_info" category to read that data, and even then it could only be written to files which also have "cust_private_info" set. While a few privileged programs may have "mlsread" or "mlswrite" attributes allowing them to override such restrictions, it's a much stronger security guarantee than pathname-based security could ever provide.

I don't really use "ls -Z" or "ps -Z" when writing SELinux policy; I do that only when I actually think I mislabeled files. Typically the SELinux-policy-development cycle is:
1) Modify and reload the policy
2) Relabel the affected files (either by hand or with some
automated tool like restorecon)
3) Rerun the problem program or daemon
4) Examine the errors in the audit logs. If there are no errors
and it works then you're finished.
5) Go back to step 1 and fix your policy

Cheers,
Kyle Moffett

2007/5/27, Kyle Moffett <[email protected]>:

What I wanted to mention was the difficulties or efforts to make assumptions real. I never meant a circular argument, but if you felt so I apologize sincerely.

Permission bits can be checked easily with "ls" command, but assuring the correctness of labels are not that easy. I'll try to explain.

The correctness of the permission bit for a given file can be judged solely by the result of "ls" command. The correctness of the label, on the other hand, can't be judged without understanding of whole policy including domain transitions. (see the attached figure)
I can imagine that once one get the complete SELinux policy, then it is able to modify and maintain it.

I don't say making a complete SELinux policy is impossible, and actually you said you did it. But to be frank, I don't think you are the average level user at all. ;-)

Do you mind if I add this?

0) I understood the default policy and perfectly understand the every behavior of my system.

this is where the difficulties exist.

Thank you for the procedures. It's quite helpful.

Every pathname-based security must provide the mechanism to prevent a conflicting/malicious access, otherwise it's junk.

I have a question for you. With current implementation of SELinux, only one label can be assigned. But there are cases that one object can be used in different context, so I think it might help if SELinux would allow objects to have multiple labels. (I'm not talking about conflicts here) What do you think?

I believe what you wrote, but it may not be as easy for average Linux users.

Cheers,
Toshiharu Harada

That's why there are a number of efforts to make modular SELinux policies. A good SELinux policy provides a few core system types and labels which a policy developer needs to understand, as well as some good macros to simplify the human-editable policy files. For instance, in my customized policy a daemon run by an initscript which reads a single config file in /etc needs this policy (Note that I use "_d" as a suffix for process domains instead of the usual "_t"):

initrc_daemon(foo_exec_t, foo_d)
daemon_config(foo_d, foo_conf_t)

Add maybe 2 lines for network port access, another 2 for database files in /var, plus maybe an "iptables" rule or two in your firewall file.

Average users are not supposed to be writing security policy. To be honest, even average-level system administrators should not be writing security policy. It's OK for such sysadmins to tweak existing policy to give access to additional web-docs or such, but only expert sysadmin/developers or security professionals should be writing security policy. It's just too damn easy to get completely wrong.

You don't have to understand the entire default policy; that's the point of modular policy. You only have to understand how to _use_ the interfaces of the system policy (which are documented) and how the particular daemon policy is supposed to work. The people developing the core system policy need to understand the inner workings of said policy, but they don't need to understand how the rest of the system works. The core functionality behind this separation is macro interfaces and attributes. By grouping types with attributes it is possible for arbitrary daemon types to categorize themselves under access rules defined by the base policy, and with interfaces the daemons don't really even need to know what those attributes are called.

This is the whole advantage of SELinux type attributes: you can define a type "var_foo_t" which has a specific list of attributes; rules which accept type specifiers can also accept attribute specifiers as well. If what you want is a label which may be accessed in two different ways, then you declare attributes for each access method and declare a type which has the attributes "filetype", "access1", and "access2" (assuming access1 and access2 are the names of the attributes you declared and used in your access rules).

As I said before, average Linux users should not be writing their own security policy. I have yet to meet an "average Linux user" who could reliably quote for me what the file permissions on the "/tmp" directory should be, or what the sticky bit was. A small percentage of average Linux system administrators don't get that right consistently, and if you don't understand the sticky bit then you should *definitely* not be controlling program permissions on a per-syscall basis.

Cheers,
Kyle Moffett

The single biggest challenge in computer security at the present time is how to build *and deploy* servers that stay reasonably secure even when run by the average wave-a-dead-chicken sysadmin, and desktop-class boxes that can survive the best attempts of Joe Sixpack's "Ooh shiny" reflex, and Joe's kid's attempts to evade the nannyware that Joe had somebody install.

(If you know how to build such things, don't bother replying. If you have actual field experience on getting significant percents of Joe Sixpacks to switch, I need to buy you a beer or something.. ;)

That explains so much! "SELinux: you're too dumb to use it, so just keep your hands in your pockets." :-)

AppArmor was designed to allow your average sys admin to write a security policy. It makes different design choices than SELinux to achieve that goal. As a result, AppArmor is an utter failure when compared to SELinux's goals, and SELinux in turn is an utter failure when compared to AppArmor's goals.

Which is why we have LSM: so we don't have to have this argument here, again.

That is a tall order. You can mostly achieve it by not giving the user the root password, but I'm not sure you would like the result :-)

Both SELinux and AppArmor can be configured so tightly that you are not going to get to install malware, by preventing the user from installing software. This isn't what users want, so they invariably bypass security and install shiny things if they own the box. SELinux and AppArmor can't help but fail if you put them in that kind of harm's way.

Crispin

Crispin Cowan, Ph.D.               http://crispincowan.com/~crispin/
Director of Software Engineering   http://novell.com
		   Security: It's not linear

Hardly. And there are helper tools

Which is a bit like giving a small child an Uzi 9mm and inviting them to teach themselves to shoot.

That depends who owns the admin password. That kind of thing (coupled with 'can install approved packages' and suitable policy) works very well in some environments where the user doesn't get the admin password - ie much of business.

Hi!

I'd not be that sure. SELinux can read AA config files, with some performance problems and bad problems with new files.

I bet solving the 'new files' problem is not going to take 20% of AA's size...
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

2007/5/29, Kyle Moffett <[email protected]>:

Thank you for your detailed and thoughtful explanation. Things are much clear now for me. Although your explanation was quite persuasive, I still have some concerns.

Linux is now being used literately everywhere. As devices, technologies and Linux itself is evolving so quickly, I'm afraid the way you showed was right but could never meet the every goal perfectly. So some areas, including embedded and special distro I guess, there must be solutions and help for average level administrators.

I think there are two ways to make secure systems. One is just you wrote: "ask it professionals" way, the other is "making practices". You might ask "how?" My answer to the question is pahtname-based systems such as AppAmor and TOMOYO Linux.
They can't be compared to SELinux, but they should be considered to supplemental tools. At least they are helpful to analyze how Linux works. Tweeking SELinux policy is not easy but writing policies for them is relatively easy (I'm not talking about security here).

Not everybody can be a professional administrators, but he/she can be a professional administrator of his/her system. I believe there must be solutions for non professional administrators. That's why we developed TOMOYO Linux (http://tomoyo.sourceforge.jp/) and so was AppArmor I guess. You might laugh, but we are doing this because we want to contribute to Linux and its community. :)

Thanks,
Toshiharu Harada

Would it make sense to label the data (resource) with a list of paths (names) that can be used to access it?

Therefore the data would be protected against being accessed via alternative arbitrary names. This may be a simple label to maintain and (possibly to) enforce, allowing path based confinement to protect a resource. This may allow for the benefits of pathname based confinement while avoiding some of its problems.

Obviously this would not protect against a pathname pointing to arbitrary data…

Just a thought.

Z. Cliffe Schreuders.

-
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

CC trimmed to remove a few poor overloaded inboxes from this tangent.

The primary problem with that is that "mv somefile otherfile" must change the labels, which means that every process that issues a rename() syscall needs to have special handling of labels. The other problem is that many of the features and capabilities of SELinux get left by the wayside. On an SELinux system 90% of the programs don't need to be modified to understand labels, since the policy can define automatic label transitions. SELinux also allows you to have conditional label privileges based on boolean variables, something that cannot be done if the privileges themselves are stored in the filesystem. Finally, such an approach does not allow you to differentiate between programs.

Cheers,
Kyle Moffett

Program Access Lists (PALs) were* a feature of UNICOS. PALs could contain not only the list of programs that could use them, but what attributes the processes required as well. Further, you could restrict or raise privilege based on the uid, gid, MAC label, and privilege state of the process during exec based on the PAL.

Yep, but you still have the label based system issues, the classic case being the text editor that uses "creat new", "unlink old", "rename new to old". When the labeling scheme is more sopisticated than "object gets label of subject" label management becomes a major issue.

Protecting special data is easy. Protecting arbitrary data is the problem.

Not a bad one, and it would be an easy and fun LSM to create. If I were teaching a Linux kernel programming course I would consider it for a class project.

-----
* I have used the past tense here in spite of the many instances of UNICOS still in operation. I don't believe that there is any current development on UNICOS.

Casey Schaufler
[email protected]
-
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eloquently put.

AppArmor actually does something similar to this, by mediating all of the ways that you can make an alias to a file. These are:

* Symbolic links: these actually don't work for making aliases with respect to LSM-based security systems such as AppArmor, SELinux, and TOMOYO Linux, because the symbolic link is resolved before the access request is presented to the LSM hooks. So if you create the symbolic link /tmp/harmless -> /etc/shadow and then the confined victim tries to access /tmp/harmless, what the LSM sees is an attempt to access /etc/shadow.
* Hard links: AppArmor explicitly mediates permission to make a hard link to a file. To be able to make a hard link /tmp/harmless -> /etc/shadow you have to have explicit permission to do so, or the attempt to make the link is blocked. This mediation is kept relatively simple by the fact that you can only hard link to files, and not to directories.
* Multiple mounts: Linux lets you mount the same file system multiple times at multiple mount points, so the same file could appear at two different places. AppArmor has very coarse mediation here, and simply prohibits all calls to mount() without special permission. So only highly privileged processes can create such an alias.
* Name spaces: Linux name spaces present a similar issue, and AppArmor's coarse blocking of mount() serves the same function; only highly privileged processes are permitted to create name spaces.

We are thinking about something kind of like this for finer-grained mediation of mount(). Currently it is all-or-nothing; we either totally block the ability to create aliases with multiple mount points or name spaces, or we totally trust the process with permission to do what it likes with multiple mount point and name space aliases.

What we want is a more nuanced mediation of mount point and name space manipulation, so that you can write a spec that says "/tmp/* can be aliased any way you like, but /etc/shadow cannot be aliased at all" or something like that.

However, not much work has been done on this, because at the moment the AppArmor team is concentrating on making the code for current functionality acceptable for upstreaming.

This is equivalent to the question of what kind of access to grant to the data. I don't see it as really difficult. What is harder is that the specification has to specify what you can alias, and where it can appear, and dealing with the two factors tends to make one's head hurt.

I am reminded of the pithy quote "Noalias must go. This is not negotiable" -- Dennis Ritchie, referring to a proposed 'noalias' type qualifier in ANSI C http://www.lysator.liu.se/c/dmr-on-noalias.html

Mediating permission to make aliases is complicated :(

.. if you use the approach of labeling the files :) In fact this problem is general: label-based security has a hard time dealing with new files in general, not just with respect to permission to create aliases. The security policy for a newly created file is determined by the label on the file, and so labeling new files is critical. SELinux uses a variety of smart heuristics to decide the label of a new file:

* the label of the parent directory
* the label of the creating process
* the domain for the creating process can do conditional things based on the label of the parent directory and the creating process
* applications that link to libselinux can overtly set the label according to application logic

AppArmor, not using labels, doesn't have this problem. You can write an AA profile with respect to files and directories that don't exist at all.

As I explained above, a lot of it is already in place with our mediation of hard links. Our mediation of multiple mount points and name spaces is lame, but the extent to which this needs to be improved will largely be determined by how application developers choose to use these features. Because AppArmor is intended primarily to confine applications, an administrative approach of just using unconfined (completely trusted) shells to manipulate name spaces and multiple mount points suffices, and application manipulation of name spaces and multiple mount points is the only concern.

That is the information flow problem. While important, it is fairly separate from the alias problem. The information flow problem is general to all access control systems, while the alias problem is special to name-based access control systems.

This is one of the name/label trade-offs. Aliasing is problematic in name based systems, while new file creation is problematic in label based systems. This follows directly from the semantics each system is using:

* Names are references. Name based systems are really mediating access to references. Thus creating new references and who can access these new references is problematic.
* Labels are equivalence classes of objects. Label based systems are really mediating access to objects, regardless of the reference. Thus creating new objects and who can access these new objects is problematic.

This depends on where you place the "may alias" access control. You can put it on the source (/tmp/harmless -> somewhere) on the target (somewhere -> /etc/shadow) or somewhere else (what AppArmor does, associating the policy with confined processes).

It could be done, but that has implications. That would mean that you get the "belt and suspenders" effect of all of the security benefits of (say) AppArmor and SELinux. But the non-overlapping security benefits are relatively small (SELinux advocates may dispute this, but whatever, it is some magnitude). On the other hand, you would get the management problems of both AppArmor and SELinux; you would have to solve both the new file problem, and the file alias problem. Think about what you would have to do to accommodate (say) a text editor's file save procedure of "write new file.tmp, rename oldfile to oldfile.old, rename newfile.tmp to oldfile, delete oldfile.old".

So it could be done, but AFAICT, with small benefit and large cost. More security may be more secure, but it isn't necessarily better.

Crispin

Crispin Cowan, Ph.D.               http://crispincowan.com/~crispin/
Director of Software Engineering   http://novell.com
		   Security: It's not linear

Hello.

Yes, TOMOYO does so too.

Although TOMOYO is not using LSM, TOMOYO checks it by manually inserted hooks that are called after resolving the symbolic links.

Yes, TOMOYO does so too.

Conventional UNIX's access control can't restrict which path_to_file can link with which another_path_to_file because UNIX's access control is a label-based access control. Therefore "ln /etc/shadow /tmp/harmless" becomes a problem when introducing pathname-based access control.

TOMOYO checks the combination of both link source and destination,
using "allow_link path_to_link_source path_to_link_destination" syntax. Therefore "ln /etc/shadow /tmp/harmless" is impossible as long as "allow_link /etc/shadow /tmp/harmless" (or something like "allow_link /etc/\* /tmp/\*") is not given. TOMOYO checks rename operation as well using
"allow_rename path_to_oldname path_to_newname" syntax.

Yes, TOMOYO does so too.

But TOMOYO checks not only the capability to call mount() system call
but also the combination of "path_to_device" "path_to_mountpoint" and "filesystem_type" using "allow_mount path_to_device path_to_mountpoint filesystem_type" ("allow_mount --bind path_to_bind_source path_to_bind_destionation" for bind mounts) syntax.

And, this is why this very long long thread began.
One of the reasons that TOMOYO doesn't use LSM is that
it is impossible to obtain "struct vfsmount" parameter inside some of LSM hook functions.
From the label-based access control's point of view,
bind mount interferes nothing with label based access control
and passing "struct vfsmount" parameter looks redundant.
But, from the pathname-based access control's point of view,
bind mount interferes severely with pathname-based access control
because it is impossible to determine which pathname was requested. Although both pathnames point to the same object,
TOMOYO focuses on the PROCEDURE FOR REACHING AN OBJECT
and being able to know the procedure is very important.
(In contrast, SELinux focuses on the ATTRIBUTE OF THE REQUESTED OBJECT, doesn't it?)

I don't think so.
I think the pathname-based access control and the label-based access control are complementarity relation. TOMOYO can coexist with SELinux and AppArmor.
What is sad, it becomes impossible to coexist with SELinux and AppArmor if TOMOYO uses LSM.

Thanks.

UNIX access control is attribute based, not label based. The distinction may be hair splitting in the current context, but could be significant later if the thread continues.

Casey Schaufler
[email protected]

What's important is that traditional DAC stores the security attributes of the object with the object. Call them what you want, it matters not.

- James

James Morris
<[email protected]>

Wrong. It is very well possible to determine the path of a particular dentry (+ vfsmount) with bind mounts.

This doesn't make sense, either. With the following sequence of syscalls of processes A and B (both of them in the namespace root),

	A:				B:
	mkdir("/tmp/a")
	chdir("/tmp/a")
					rename("/tmp/a", "/tmp/b")
	creat("f")

the path being checked for the creat call must be "/tmp/b/f", even though process A never explicitly used "b". If that's not what TOMOYO is doing, then that's badly broken.

Andreas

Hello.

AppArmor can't determine which pathname (/tmp/public/file or /tmp/secret/file) was requested by touch command if bound mount is used in the following way

# mkdir /tmp/public /tmp/secret
# mount -t tmpfs none /tmp/public
# mount --bind /tmp/public /tmp/secret
# touch /tmp/public/file

because security_inode_create() doesn't receive vfsmount, can it? It is possible to determine that the requested pathname is either /tmp/public/file or /tmp/secret/file by comparing address of vfsmount available from current->namespace, but it is impossible to determine which one.

Yes, of course, TOMOYO checks "/tmp/b/f". What I meant PROCEDURE FOR REACHING is "which directory does the process need to go through to reach the requested file if the process's current directory is the root of the process's namespace". And in this case, it is "/tmp/b/f".

Thanks.

I don't know what you are talking about -- the very first patch in the AppArmor series adds the vfsmount parameter to security_inode_create().

Andreas

Hello.

I'm talking about "the current version" of AppArmor (I mean AppArmor available for OpenSuSE 10.1/10.2, but I should call "the previous version" of AppArmor
if you call AppArmor in this series of patch "the current version") can't handle bind mounts. I know "the next version" of AppArmor (I mean AppArmor in this series of patch) will be able to handle bind mounts better if these vfsmount patches are adopted.

Thanks.

Hi!

...

Unfortunately, aparmor is by design limited to subset of distro (network daemons). Unfortunately, some other programs (passwd, vi) routinely make hardlinks. So AA mediating hardlink is not enough, as vi will happily hardlink /etc/shadow into /etc/.vi-shadow-1234.

Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

but with the AA design of default deny this isn't a big problem unless you specificly allow some network daemon to access /etc/.vi-shadow-1234

in this context passwd and vi are considered trusted processes, they are used _after_ you authenticate onto the box.

no, this won't help you much against local users, but there are a _lot_ of boxes out there with few, if any, local users who don't also have the root password. AA helps the admin be safer when configuring netwrok daemons.

David Lang

Hi!

...or unless vi decides to hardlink into /tmp or something.

Hmm, I guess I'd love "it is useless on multiuser boxes" to become standard part of AA advertising.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

That's not quite what david@ said. As I understand it, AppArmor is not focused on preventing attacks by local users against other local users; that's not the main problem it is trying to solve. Rather, it's primary purpose is to deal with attacks by remote bad guys against your network servers. That is a laudable goal. Anything that helps reduce the impact of remote exploits is bound to be useful, even if doesn't do a darn thing to stop local users from attacking each other.

This means that AppArmor could still be useful on multiuser boxes, even if that utility is limited to defending (some) network daemons against remote attack (or, more precisely, reducing the damage done by a successful remote attack against a network daemon).

Hi!

(Please do not drop me from cc list when replying).

It is also not going to prevent local users attacking network servers. Local users can ln arbitrary files, and then play tricks with network servers.

Yes, if there's significantly more remote bad guys than local bad guys, and if remote bad guys can't just get some local user first, AA still has some value.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Experience over on the Windows side of the fence indicates that "remote bad guys get some local user first" is a *MAJOR* part of the current real-world threat model - the vast majority of successful attacks on end-user boxes these days start off with either "Get user to (click on link|open attachment)" or "Subvert the path to a website (either by hacking the real site or hijacking the DNS) and deliver a drive-by fruiting when the user visits the page".

and if your local non-root user can create a hard link to /etc/shadow and access it they own your box anyway (they can just set the root password to anything they want). since I don't hear about this happening there are other restrictions that prevent this anyway.

everyone recognises that AA has limits, but the way people are emphisising these acknowledged limits is beginning to sound a bit shrill.

David Lang

I think you need to look how unix security works:

pavel@amd:/tmp$ ln /etc/shadow .
pavel@amd:/tmp$ cat shadow
cat: shadow: Permission denied
pavel@amd:/tmp$

Yes, regular users are permitted to hardlink shadow, no, it is not a security hole, yes, it is a problem for AA.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

AppArmor isn't trying to defend everyday users from getting phished or social engineered; it is trying to protect servers from getting rooted because of security holes in their network daemons. I find that a laudable goal. Sure, it doesn't solve every security problem in the world, but so what? A tool that could solve that one security problem would still be a useful thing, even if it did nothing else.

I don't find the Windows stuff too relevant here. As I understand it, AppArmor isn't aimed at defending Windows desktop users; it is aimed at defending Linux servers. A pretty different environment, I'd say.

Ultimately, there are some things AppArmor may be good at, and there are also sure to be some things it is bloody useless for. My hammer isn't very good for screwing in screws, but I still find it useful. I confess I don't understand the kvetching about AppArmor's goals. What are you expecting, some kind of silver bullet?

A question I'd find more interesting is whether AppArmor is able to meet its stated goals, under a reasonable threat model, and with what degree of assurance, and at what cost. But I don't know whether that's relevant for the linux-kernel mailing list.

I'm surprised. The only Windows-specific thing in the whole paragraph is that the attack described is currently wildly successful. And there *have* been known exploitable bugs in the Linux version of Firefox. In other words, all the pieces are in place for exactly the same thing to work on Linux.

The type of hardening that AppArmor can provide network-facing daemons is only protecting the system against attacks that aren't even a large part of the
threat model. Exploiting a broken PHP script? Happens all the time, and
AppArmor can't do much for it. SQL injection? Happens all the time, and it can't help much there either. Systems getting pwned because the sysadmin's laptop got hacked? Pretty common, and another thing that AppArmor won't slow down. But yes, I *will* grant that the next time there's a buffer overflow in Apache, AppArmor will be able to help *that*....

The only reason you're not seeing the same exact threat model against Linux servers is because it's still a minority. It's *always* been true that one of the most productive attacks on a server has been to find a desktop that you can attack, and then abuse a trust relationship from the desktop to the server (and has been, ever since the server was a IBM mainframe and the desktop was an RJE station. Amazing how trusting OS/360 was of a card deck tossed into a remote card reader... :)

The question is whether it's a hammer, a screwdriver, or that coping saw that you never seem to find a use for...

Exactly the opposite - I'm worried that it will be treated as a silver bullet.

And historically, we've had an amazing amount of pushback againt things that are intrusive and only provide a partial solution.

actually, this is _exactly_ where AppArmor is the most useful. if the PHP script is restricted by AppArmor it won't be able to go out and touch things that it's not supposed to.

if you are targeting one specific company or one specific server then you are correct, however most attacks are not that targeted, they do things like useing google to find random servers that are running vunerable software and attack that (or just try the attack against random IP addresses in case it happens to be running the vunerable software)

David Lang

OK. I'll bite. AppArmor basically only mediates filename objects.

What filename do you specify to stop it when the exploited PHP script is used bu a spammer to send mail to millions, when it was intended to send mail only to a specific set of people? Wait, that's a tcp connection to localhost:25.

What filename do you specifu to stop blog comment spam and other abuses of a content management system (remember that the PHP code *does* need write access to the files in question)?

It might be able to stop J Random SkriptKiddy from scribbling "Y0uz Ben Pwned" all over your home page, but it doesn't do much to control lots of other abuses of web apps. To be fair, SELinux can't help a lot more, because the problem often ends up being abuse of an access privilege that the program *should* have - for example, if it's supposed to query the database, it's hard to stop it from making an inappropriate query at the level that AppArmor and SELinux work at.

I'm not convinced that it's solving enough *actual* problems, given that we've rejected a lot of other "helps a little in some cases" code for kernel inclusion.

There's a lot of that going around. And they're the attacks that you need to worry about, because you're likely to end up as a headline.

There's a big difference between "most attacks" and "most attacks you should worry about".

Rmember that at a minimum, that also means that you're Goggleable as vulnerable to attacks that AppArmor can't stop. And yes, Googling for vulnerable software *is* one of the primary ways that blog spammers find the vulerable blogs.

If your site is run in such a way that you you have to worry about random attackers who use google, your site has *bigger* security issues, and thinking that AppArmor is going to improve things is exactly the sort of smoke screen magic bullet that we don't want putting in the kernel.

for now, the AppArmor folks have said that the next step is to move on to other objects (like network connections) after the initial step is accepted.

it stops the PHP script from spawning a shell that would be considered a 'local trusted user'

if you use SELinux and give the PHP script permission to write to your content management system then the PHP script can corrupt that system if it's exploited. AppArmor is the same way

this is Security 101 (or even more basic), if you grant a program access to do something you can't prevent that program from doing that something.

it's not hard, it's impossible

how can you know if the command 'foo' sent to another program on your system will cause that system to erase everything it has access to or return 'bar'? you don't and no system tool can (and no system tool should)

if you want to control what commands one program can send to another you need a security proxy between them. that isn't what AppArmor or SELinux are trying to do so don't blame them for not doing it.

you seem to want a 'only let the system do what the programmer intended' command, and anything less isn't solving the actual problem. by that logic we shouldn't bother with passwords either, since they don't completely solve the problem

you first want to knock off all the things in that 'most attacks' catagory so that you can pay close attention to the threats that are targeted directly at you.

when the next exploit is found is network accessable server X, AppArmor can prevent it from doing anything to the system that server X wasn't already allowed to touch.

For example, this means that it probably won't be allowed to run bash and provide a shell to the outside attacker.

It also means that locally exploitable bugs are less likely to be able to be combined with remotely exploitable bugs into complete machine takeovers becouse the remotely exploitable server would have to have permission to access the locally exploitable software.

if you can't see any advantage to this then you would be happy doing a chmod -R 777 / on your system (becouse normal user permissions are a much more restricted way of limiting the same type of access)

David Lang

And Security 102 is "most of the *real* trouble starts when authorized programs access authorized things in unintended ways".

I'm not blaming them. I'm blaming the people who are trying to sell AppArmor as doing much more than a small bandaid on the totality of the security issue.

There's *lots* of attacks. AppArmor only addresses a very small segment of them, and it's quite arguably not even addressing the ones you should worry about.

No, I'm saying that we've had a *lot* of partial solutions, from Grsecurity to signed-modules, that didn't go in to the tree because they were too intrusive for the partial security they provided.

Quite frankly, if you've configured your system properly, and kept things patched, and all the other things you should be doing *anyhow*, the vast majority of "most attacks" really shouldn't be much more than "The daily report showed another 2,395 probes for that year-old exploit".

Remember - if you're getting hit by a "background noise" random attempt, it is almost certainly *not* a 0-day, because any attacker who's clued enough to *have* a 0-day has enough sense to save it for when he has a targeted attack planned. So there's 2 possibilities:

1) It's a random attack, and it's a *known* attack and other things (patches, IPS, and so on) should already be mitigating the attack.

2) You're a specific target, and the fact that AppArmor stopped the attack just means that a different attack will be made.

No, but I can mmap an area, make it executable, and download a block of executable code and branch to it, giving me a shell anyhow.

Right. Instead, the locally exploitable bug is combined with an exploitable browser bug instead. :)

Oddly enough, I don't see an advantage to only closing off half the avenues of attack. If you go digging through the SELinux and LSM archives, you'll probably find me arguing against the SELinux 'targeted' policy, for many of the same reasons. And the 'targeted' policy does a better containment of things than the AppArmor model.

Bad analogy. You meant "chmod -R 777 /var /etc because the file permissions on the /lib and /usr trees are enough to stop most random attacks".

Yes, normal access permissions, being DAC, *are* a very limited way of applying MAC. And if you go back and read the LSM archives, the decision was made to apply permissions/ACLS DAC first, and then apply LSM MAC, mostly for performance and least-surprise reasons. There's absolutely no big theoretical reason why you couldn't chmod 777 everything, if you loaded an LSM that implemented the necessary MAC.

Hi!

(Please preserve cc lists when replying on l-k).

AA solves less problems than SELinux does. Some people like AA more, but I guess they should just learn SELinux.

And yes, I'm afraid this discussion is relevant on l-k, because we should have very good reasons before merging duplicate functionality.

Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

And vi solves less problems than OpenOffice.
vi is good for a different set of purposes than OpenOffice. AA and SELinux both aspire to being Security Solutions, but that does not make either a subset of the other.

Knowing the people involved I would suggest that the AA people did learn SELinux, and came to their own conclusions regarding it's applicability to their needs, and that those conclusions are not the same as yours.

'cmon, you know better than to claim that this is duplicate functionality. No one is arguing that. The arguments have been that the conceptual basis of named based access control are flawwed. As that argument has failed to move the AA adherants, the old sawhorse that SELinux does everything, or could be made to if you sweated the policy hard enough, got pulled out. No evidence to that effect, mind you, but the old "waves paw" nonetheless.

SELinux is the finest implementation of Type Enforcement on the planet. TE does not match everyone's definition of security. AA is an alternative that clearly has as tough a roe to hoe as SELinux did in 2001, when it was up against MLS system vendors who compared it to Froot Loops. Alternatives, even those that you don't personally care for, are good for you.

Casey Schaufler
[email protected]

That is not true. AppArmor is designed to confine any application you do not want to trust completely. This includes network daemons (Apache, Sendmail, BIND, etc.) network clients (Firefox, Thunderbird, GAIM/Pidgin, etc.) and any other application that mediates trust: where data on one side of the application is higher privilege than potential attackers on the other side of the application.

I have no idea what you are talking about. I routinely profile vim on stage in AppArmor presentations, and it presents no such problems.

Caveat: the circumstances under which you would actually want to profile vi are rather limited, most users would not want to do that.

That is usually around slide 7 of the standard AppArmor presentation, right next to the remarks about how mulituser boxes are nearly extinct dinosaurs. AppArmor gets some of its simplicity and ease of use by not considering that vanishing use case. Even so, AppArmor does have uses on multiuser boxes, just not the uses Pavel wishes for. Fine, use a different tool for a different task, AppArmor has plenty of use cases.

The limitation Pavel is referring to is that AppArmor does not secure processes that are not profiled by AppArmor. We know, this is intentional, and contributes to AppArmor's ease of use, and does not generate a hole if you consider every process exposed to (say) network attack and confine it.

Crispin

Crispin Cowan, Ph.D.               http://crispincowan.com/~crispin/
Director of Software Engineering   http://novell.com
		   Security: It's not linear

Hi,

2007/5/24, James Morris <[email protected]>:

If I understand correctly, the original issue was whether to allow passing vfsmount to the inode_create LSM hook or not. Which is independent from AA or "pathname based MAC", I think.

It is proven that Linux can be used without that change,　however it is also clear that current LSM cause the ambiguities as AA people has explained. Clearing ambiguities is a obvious gain to Linux and will make benefits for auditing besides "pathname based MAC".

So here's my opinion. If anybody can't explain clear reason (or needs) to keep these ambiguities unsolved,　we should consider to merge the proposal.

Thanks.

Toshiharu Harada
[email protected]

notify_change() already calls security_inode_setattr() before calling iop->setattr.

---
 fs/proc/base.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

@@ -344,11 +344,8 @@ static int proc_setattr(struct dentry *d
 		return -EPERM;
 
 	error = inode_change_ok(inode, attr);
-	if (!error) {
-		error = security_inode_setattr(dentry, attr);
-		if (!error)
-			error = inode_setattr(inode, attr);
-	}
+	if (!error)
+		error = inode_setattr(inode, attr);
 	return error;
 }
 

notify_change() already calls security_inode_setattr() before calling iop->setattr.

---
 fs/proc/proc_sysctl.c |    7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

@@ -429,11 +429,8 @@ static int proc_sys_setattr(struct dentr
 		return -EPERM;
 
 	error = inode_change_ok(inode, attr);
-	if (!error) {
-		error = security_inode_setattr(dentry, attr);
-		if (!error)
-			error = inode_setattr(inode, attr);
-	}
+	if (!error)
+		error = inode_setattr(inode, attr);
 
 	return error;
 }

This is a behaviour change on all of these and limits some behaviour of existing established security modules

When inode_change_ok is called it has side effects. This includes clearing the SGID bit on attribute changes caused by chmod. If you make this change the results of some rulesets may be different before or after the change is made.

I'm not saying the change is wrong but it does change behaviour so that needs looking at closely (ditto all other attribute twiddles)

Alan

Pass struct path to remove_suid and should_remove_suid instead of only the dentry. Required by a later patch that adds a struct vfsmount parameter to notify_change().

---
 fs/ntfs/file.c             |    2 +-
 fs/ocfs2/file.c            |   10 +++++-----
 fs/reiserfs/file.c         |    2 +-
 fs/splice.c                |    6 +++---
 fs/xfs/linux-2.6/xfs_lrw.c |    2 +-
 include/linux/fs.h         |    6 +++---
 mm/filemap.c               |   16 ++++++++--------
 mm/filemap_xip.c           |    2 +-
 mm/shmem.c                 |    2 +-
 9 files changed, 24 insertions(+), 24 deletions(-)

@@ -2162,7 +2162,7 @@ static ssize_t ntfs_file_aio_write_noloc
 		goto out;
 	if (!count)
 		goto out;
-	err = remove_suid(file->f_path.dentry);
+	err = remove_suid(&file->f_path);
 	if (err)
 		goto out;
 	file_update_time(file);

@@ -1035,13 +1035,13 @@ out:
 	return ret;
 }
 
-static int ocfs2_prepare_inode_for_write(struct dentry *dentry,
+static int ocfs2_prepare_inode_for_write(struct path *path,
 					 loff_t *ppos,
 					 size_t count,
 					 int appending)
 {
 	int ret = 0, meta_level = appending;
-	struct inode *inode = dentry->d_inode;
+	struct inode *inode = path->dentry->d_inode;
 	u32 clusters;
 	loff_t newsize, saved_pos;
 

@@ -1067,7 +1067,7 @@ static int ocfs2_prepare_inode_for_write
 		 * inode. There's also the dinode i_size state which
 		 * can be lost via setattr during extending writes (we
 		 * set inode->i_size at the end of a write. */
-		if (should_remove_suid(dentry)) {
+		if (should_remove_suid(path)) {
 			if (meta_level == 0) {
 				ocfs2_meta_unlock(inode, meta_level);
 				meta_level = 1;

@@ -1176,7 +1176,7 @@ static ssize_t ocfs2_file_aio_write(stru
 		goto out;
 	}
 
-	ret = ocfs2_prepare_inode_for_write(filp->f_path.dentry, &iocb->ki_pos,
+	ret = ocfs2_prepare_inode_for_write(&filp->f_path, &iocb->ki_pos,
 					    iocb->ki_left, appending);
 	if (ret < 0) {
 		mlog_errno(ret);

@@ -1239,7 +1239,7 @@ static ssize_t ocfs2_file_splice_write(s
 		goto out;
 	}
 
-	ret = ocfs2_prepare_inode_for_write(out->f_path.dentry, ppos, len, 0);
+	ret = ocfs2_prepare_inode_for_write(&out->f_path, ppos, len, 0);
 	if (ret < 0) {
 		mlog_errno(ret);
 		goto out_unlock;

@@ -1353,7 +1353,7 @@ static ssize_t reiserfs_file_write(struc
 	if (count == 0)
 		goto out;
 
-	res = remove_suid(file->f_path.dentry);
+	res = remove_suid(&file->f_path);
 	if (res)
 		goto out;
 

@@ -797,7 +797,7 @@ generic_file_splice_write_nolock(struct 
 	ssize_t ret;
 	int err;
 
-	err = remove_suid(out->f_path.dentry);
+	err = remove_suid(&out->f_path);
 	if (unlikely(err))
 		return err;
 

@@ -843,10 +843,10 @@ generic_file_splice_write(struct pipe_in
 	ssize_t ret;
 	int err;
 
-	err = should_remove_suid(out->f_path.dentry);
+	err = should_remove_suid(&out->f_path);
 	if (unlikely(err)) {
 		mutex_lock(&inode->i_mutex);
-		err = __remove_suid(out->f_path.dentry, err);
+		err = __remove_suid(&out->f_path, err);
 		mutex_unlock(&inode->i_mutex);
 		if (err)
 			return err;

@@ -799,7 +799,7 @@ start:
 	     !capable(CAP_FSETID)) {
 		error = xfs_write_clear_setuid(xip);
 		if (likely(!error))
-			error = -remove_suid(file->f_path.dentry);
+			error = -remove_suid(&file->f_path);
 		if (unlikely(error)) {
 			xfs_iunlock(xip, iolock);
 			goto out_unlock_mutex;

@@ -1685,9 +1685,9 @@ extern void __iget(struct inode * inode)
 extern void clear_inode(struct inode *);
 extern void destroy_inode(struct inode *);
 extern struct inode *new_inode(struct super_block *);
-extern int __remove_suid(struct dentry *, int);
-extern int should_remove_suid(struct dentry *);
-extern int remove_suid(struct dentry *);
+extern int __remove_suid(struct path *, int);
+extern int should_remove_suid(struct path *);
+extern int remove_suid(struct path *);
 
 extern void __insert_inode_hash(struct inode *, unsigned long hashval);
 extern void remove_inode_hash(struct inode *);

@@ -1849,9 +1849,9 @@ repeat:
  *	if suid or (sgid and xgrp)
  *		remove privs
  */
-int should_remove_suid(struct dentry *dentry)
+int should_remove_suid(struct path *path)
 {
-	mode_t mode = dentry->d_inode->i_mode;
+	mode_t mode = path->dentry->d_inode->i_mode;
 	int kill = 0;
 
 	/* suid always must be killed */

@@ -1872,20 +1872,20 @@ int should_remove_suid(struct dentry *de
 }
 EXPORT_SYMBOL(should_remove_suid);
 
-int __remove_suid(struct dentry *dentry, int kill)
+int __remove_suid(struct path *path, int kill)
 {
 	struct iattr newattrs;
 
 	newattrs.ia_valid = ATTR_FORCE | kill;
-	return notify_change(dentry, &newattrs);
+	return notify_change(path->dentry, &newattrs);
 }
 
-int remove_suid(struct dentry *dentry)
+int remove_suid(struct path *path)
 {
-	int kill = should_remove_suid(dentry);
+	int kill = should_remove_suid(path);
 
 	if (unlikely(kill))
-		return __remove_suid(dentry, kill);
+		return __remove_suid(path, kill);
 
 	return 0;
 }

@@ -2252,7 +2252,7 @@ __generic_file_aio_write_nolock(struct k
 	if (count == 0)
 		goto out;
 
-	err = remove_suid(file->f_path.dentry);
+	err = remove_suid(&file->f_path);
 	if (err)
 		goto out;
 

@@ -405,7 +405,7 @@ xip_file_write(struct file *filp, const 
 	if (count == 0)
 		goto out_backing;
 
-	ret = remove_suid(filp->f_path.dentry);
+	ret = remove_suid(&filp->f_path);
 	if (ret)
 		goto out_backing;
 

@@ -1516,7 +1516,7 @@ shmem_file_write(struct file *file, cons
 	if (err || !count)
 		goto out;
 
-	err = remove_suid(file->f_path.dentry);
+	err = remove_suid(&file->f_path);
 	if (err)
 		goto out;
 

The vfsmount parameter must be set appropriately for files visibile outside the kernel. Files that are only used in a filesystem (e.g., reiserfs xattr files) will have a NULL vfsmount.

---
 fs/attr.c           |    3 ++-
 fs/ecryptfs/inode.c |    4 +++-
 fs/exec.c           |    3 ++-
 fs/fat/file.c       |    2 +-
 fs/hpfs/namei.c     |    2 +-
 fs/namei.c          |    3 ++-
 fs/nfsd/vfs.c       |    8 ++++----
 fs/open.c           |   28 +++++++++++++++-------------
 fs/reiserfs/xattr.c |    6 +++---
 fs/sysfs/file.c     |    2 +-
 fs/utimes.c         |    4 ++--
 include/linux/fs.h  |    6 +++---
 mm/filemap.c        |    2 +-
 mm/tiny-shmem.c     |    2 +-
 14 files changed, 41 insertions(+), 34 deletions(-)

@@ -101,7 +101,8 @@ int inode_setattr(struct inode * inode, 
 }
 EXPORT_SYMBOL(inode_setattr);
 
-int notify_change(struct dentry * dentry, struct iattr * attr)
+int notify_change(struct dentry *dentry, struct vfsmount *mnt,
+		  struct iattr *attr)
 {
 	struct inode *inode = dentry->d_inode;
 	mode_t mode;

@@ -870,12 +870,14 @@ static int ecryptfs_setattr(struct dentr
 {
 	int rc = 0;
 	struct dentry *lower_dentry;
+	struct vfsmount *lower_mnt;
 	struct inode *inode;
 	struct inode *lower_inode;
 	struct ecryptfs_crypt_stat *crypt_stat;
 
 	crypt_stat = &ecryptfs_inode_to_private(dentry->d_inode)->crypt_stat;
 	lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
 	inode = dentry->d_inode;
 	lower_inode = ecryptfs_inode_to_lower(inode);
 	if (ia->ia_valid & ATTR_SIZE) {

@@ -890,7 +892,7 @@ static int ecryptfs_setattr(struct dentr
 		if (rc < 0)
 			goto out;
 	}
-	rc = notify_change(lower_dentry, ia);
+	rc = notify_change(lower_dentry, lower_mnt, ia);
 out:
 	fsstack_copy_attr_all(inode, lower_inode, NULL);
 	return rc;

@@ -1532,7 +1532,8 @@ int do_coredump(long signr, int exit_cod
 		goto close_fail;
 	if (!file->f_op->write)
 		goto close_fail;
-	if (!ispipe && do_truncate(file->f_path.dentry, 0, 0, file) != 0)
+	if (!ispipe &&
+	    do_truncate(file->f_path.dentry, file->f_path.mnt, 0, 0, file) != 0)
 		goto close_fail;
 
 	retval = binfmt->core_dump(signr, regs, file);

@@ -92,7 +92,7 @@ int fat_generic_ioctl(struct inode *inod
 		}
 
 		/* This MUST be done before doing anything irreversible... */
-		err = notify_change(filp->f_path.dentry, &ia);
+		err = notify_change(filp->f_path.dentry, filp->f_path.mnt, &ia);
 		if (err)
 			goto up;
 

@@ -426,7 +426,7 @@ again:
 			/*printk("HPFS: truncating file before delete.\n");*/
 			newattrs.ia_size = 0;
 			newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
-			err = notify_change(dentry, &newattrs);
+			err = notify_change(dentry, NULL, &newattrs);
 			put_write_access(inode);
 			if (!err)
 				goto again;

@@ -1580,7 +1580,8 @@ int may_open(struct nameidata *nd, int a
 		if (!error) {
 			DQUOT_INIT(inode);
 			
-			error = do_truncate(dentry, 0, ATTR_MTIME|ATTR_CTIME, NULL);
+			error = do_truncate(dentry, nd->mnt, 0,
+					    ATTR_MTIME|ATTR_CTIME, NULL);
 		}
 		put_write_access(inode);
 		if (error)

@@ -358,7 +358,7 @@ nfsd_setattr(struct svc_rqst *rqstp, str
 	err = nfserr_notsync;
 	if (!check_guard || guardtime == inode->i_ctime.tv_sec) {
 		fh_lock(fhp);
-		host_err = notify_change(dentry, iap);
+		host_err = notify_change(dentry, fhp->fh_export->ex_mnt, iap);
 		err = nfserrno(host_err);
 		fh_unlock(fhp);
 	}

@@ -893,13 +893,13 @@ out:
 	return err;
 }
 
-static void kill_suid(struct dentry *dentry)
+static void kill_suid(struct dentry *dentry, struct vfsmount *mnt)
 {
 	struct iattr	ia;
 	ia.ia_valid = ATTR_KILL_SUID | ATTR_KILL_SGID;
 
 	mutex_lock(&dentry->d_inode->i_mutex);
-	notify_change(dentry, &ia);
+	notify_change(dentry, mnt, &ia);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 }
 

@@ -958,7 +958,7 @@ nfsd_vfs_write(struct svc_rqst *rqstp, s
 
 	/* clear setuid/setgid flag after write */
 	if (host_err >= 0 && (inode->i_mode & (S_ISUID | S_ISGID)))
-		kill_suid(dentry);
+		kill_suid(dentry, exp->ex_mnt);
 
 	if (host_err >= 0 && stable) {
 		static ino_t	last_ino;

@@ -194,8 +194,8 @@ out:
 	return error;
 }
 
-int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
-	struct file *filp)
+int do_truncate(struct dentry *dentry, struct vfsmount *mnt, loff_t length,
+		unsigned int time_attrs, struct file *filp)
 {
 	int err;
 	struct iattr newattrs;

@@ -212,7 +212,7 @@ int do_truncate(struct dentry *dentry, l
 	}
 
 	mutex_lock(&dentry->d_inode->i_mutex);
-	err = notify_change(dentry, &newattrs);
+	err = notify_change(dentry, mnt, &newattrs);
 	mutex_unlock(&dentry->d_inode->i_mutex);
 	return err;
 }

@@ -267,7 +267,7 @@ static long do_sys_truncate(const char _
 	error = locks_verify_truncate(inode, NULL, length);
 	if (!error) {
 		DQUOT_INIT(inode);
-		error = do_truncate(nd.dentry, length, 0, NULL);
+		error = do_truncate(nd.dentry, nd.mnt, length, 0, NULL);
 	}
 	put_write_access(inode);
 

@@ -319,7 +319,8 @@ static long do_sys_ftruncate(unsigned in
 
 	error = locks_verify_truncate(inode, file, length);
 	if (!error)
-		error = do_truncate(dentry, length, ATTR_MTIME|ATTR_CTIME, file);
+		error = do_truncate(dentry, file->f_path.mnt, length,
+				    ATTR_MTIME|ATTR_CTIME, file);
 out_putf:
 	fput(file);
 out:

@@ -519,7 +520,7 @@ asmlinkage long sys_fchmod(unsigned int 
 		mode = inode->i_mode;
 	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-	err = notify_change(dentry, &newattrs);
+	err = notify_change(dentry, file->f_path.mnt, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 
 out_putf:

@@ -554,7 +555,7 @@ asmlinkage long sys_fchmodat(int dfd, co
 		mode = inode->i_mode;
 	newattrs.ia_mode = (mode & S_IALLUGO) | (inode->i_mode & ~S_IALLUGO);
 	newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-	error = notify_change(nd.dentry, &newattrs);
+	error = notify_change(nd.dentry, nd.mnt, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 
 dput_and_out:

@@ -568,7 +569,8 @@ asmlinkage long sys_chmod(const char __u
 	return sys_fchmodat(AT_FDCWD, filename, mode);
 }
 
-static int chown_common(struct dentry * dentry, uid_t user, gid_t group)
+static int chown_common(struct dentry * dentry, struct vfsmount *mnt,
+			uid_t user, gid_t group)
 {
 	struct inode * inode;
 	int error;

@@ -597,7 +599,7 @@ static int chown_common(struct dentry * 
 	if (!S_ISDIR(inode->i_mode))
 		newattrs.ia_valid |= ATTR_KILL_SUID|ATTR_KILL_SGID;
 	mutex_lock(&inode->i_mutex);
-	error = notify_change(dentry, &newattrs);
+	error = notify_change(dentry, mnt, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 out:
 	return error;

@@ -611,7 +613,7 @@ asmlinkage long sys_chown(const char __u
 	error = user_path_walk(filename, &nd);
 	if (error)
 		goto out;
-	error = chown_common(nd.dentry, user, group);
+	error = chown_common(nd.dentry, nd.mnt, user, group);
 	path_release(&nd);
 out:
 	return error;

@@ -631,7 +633,7 @@ asmlinkage long sys_fchownat(int dfd, co
 	error = __user_walk_fd(dfd, filename, follow, &nd);
 	if (error)
 		goto out;
-	error = chown_common(nd.dentry, user, group);
+	error = chown_common(nd.dentry, nd.mnt, user, group);
 	path_release(&nd);
 out:
 	return error;

@@ -645,7 +647,7 @@ asmlinkage long sys_lchown(const char __
 	error = user_path_walk_link(filename, &nd);
 	if (error)
 		goto out;
-	error = chown_common(nd.dentry, user, group);
+	error = chown_common(nd.dentry, nd.mnt, user, group);
 	path_release(&nd);
 out:
 	return error;

@@ -664,7 +666,7 @@ asmlinkage long sys_fchown(unsigned int 
 
 	dentry = file->f_path.dentry;
 	audit_inode(NULL, dentry->d_inode);
-	error = chown_common(dentry, user, group);
+	error = chown_common(dentry, file->f_path.mnt, user, group);
 	fput(file);
 out:
 	return error;

@@ -527,7 +527,7 @@ reiserfs_xattr_set(struct inode *inode, 
 	newattrs.ia_size = buffer_size;
 	newattrs.ia_valid = ATTR_SIZE | ATTR_CTIME;
 	mutex_lock(&xinode->i_mutex);
-	err = notify_change(fp->f_path.dentry, &newattrs);
+	err = notify_change(fp->f_path.dentry, NULL, &newattrs);
 	if (err)
 		goto out_filp;
 

@@ -867,7 +867,7 @@ reiserfs_chown_xattrs_filler(void *buf, 
 	}
 
 	if (!S_ISDIR(xafile->d_inode->i_mode))
-		err = notify_change(xafile, attrs);
+		err = notify_change(xafile, NULL, attrs);
 	dput(xafile);
 
 	return err;

@@ -919,7 +919,7 @@ int reiserfs_chown_xattrs(struct inode *
 		goto out_dir;
 	}
 
-	err = notify_change(dir, attrs);
+	err = notify_change(dir, NULL, attrs);
 	unlock_kernel();
 
       out_dir:

@@ -584,7 +584,7 @@ int sysfs_chmod_file(struct kobject *kob
 			newattrs.ia_mode = (mode & S_IALLUGO) |
 						(inode->i_mode & ~S_IALLUGO);
 			newattrs.ia_valid = ATTR_MODE | ATTR_CTIME;
-			res = notify_change(victim, &newattrs);
+			res = notify_change(victim, NULL, &newattrs);
 			mutex_unlock(&inode->i_mutex);
 		}
 		dput(victim);

@@ -62,7 +62,7 @@ asmlinkage long sys_utime(char __user * 
 			goto dput_and_out;
 	}
 	mutex_lock(&inode->i_mutex);
-	error = notify_change(nd.dentry, &newattrs);
+	error = notify_change(nd.dentry, nd.mnt, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 dput_and_out:
 	path_release(&nd);

@@ -115,7 +115,7 @@ long do_utimes(int dfd, char __user *fil
 			goto dput_and_out;
 	}
 	mutex_lock(&inode->i_mutex);
-	error = notify_change(nd.dentry, &newattrs);
+	error = notify_change(nd.dentry, nd.mnt, &newattrs);
 	mutex_unlock(&inode->i_mutex);
 dput_and_out:
 	path_release(&nd);

@@ -1458,8 +1458,8 @@ static inline int break_lease(struct ino
 
 /* fs/open.c */
 
-extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
-		       struct file *filp);
+extern int do_truncate(struct dentry *, struct vfsmount *, loff_t start,
+		       unsigned int time_attrs, struct file *filp);
 extern long do_sys_open(int fdf, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);

@@ -1612,7 +1612,7 @@ extern int do_remount_sb(struct super_bl
 #ifdef CONFIG_BLOCK
 extern sector_t bmap(struct inode *, sector_t);
 #endif
-extern int notify_change(struct dentry *, struct iattr *);
+extern int notify_change(struct dentry *, struct vfsmount *, struct iattr *);
 extern int permission(struct inode *, int, struct nameidata *);
 extern int generic_permission(struct inode *, int,
 		int (*check_acl)(struct inode *, int));

@@ -1877,7 +1877,7 @@ int __remove_suid(struct path *path, int
 	struct iattr newattrs;
 
 	newattrs.ia_valid = ATTR_FORCE | kill;
-	return notify_change(path->dentry, &newattrs);
+	return notify_change(path->dentry, path->mnt, &newattrs);
 }
 
 int remove_suid(struct path *path)

@@ -86,7 +86,7 @@ struct file *shmem_file_setup(char *name
 	file->f_mode = FMODE_WRITE | FMODE_READ;
 
 	/* notify everyone as to the change of file size */
-	error = do_truncate(dentry, size, 0, file);
+	error = do_truncate(dentry, file->f_path.mnt, size, 0, file);
 	if (error < 0)
 		goto close_file;
 

This is needed for computing pathnames in the AppArmor LSM.

---
 fs/attr.c                |    4 ++--
 include/linux/security.h |    8 ++++++--
 security/dummy.c         |    3 ++-
 security/selinux/hooks.c |    5 +++--
 4 files changed, 13 insertions(+), 7 deletions(-)

@@ -145,13 +145,13 @@ int notify_change(struct dentry *dentry,
 		down_write(&dentry->d_inode->i_alloc_sem);
 
 	if (inode->i_op && inode->i_op->setattr) {
-		error = security_inode_setattr(dentry, attr);
+		error = security_inode_setattr(dentry, mnt, attr);
 		if (!error)
 			error = inode->i_op->setattr(dentry, attr);
 	} else {
 		error = inode_change_ok(inode, attr);
 		if (!error)
-			error = security_inode_setattr(dentry, attr);
+			error = security_inode_setattr(dentry, mnt, attr);
 		if (!error) {
 			if ((ia_valid & ATTR_UID && attr->ia_uid != inode->i_uid) ||
 			    (ia_valid & ATTR_GID && attr->ia_gid != inode->i_gid))

@@ -358,6 +358,7 @@ struct request_sock;
  *	file attributes change (such as when a file is truncated, chown/chmod
  *	operations, transferring disk quotas, etc).
  *	@dentry contains the dentry structure for the file.
+ *	@mnt is the vfsmount corresponding to @dentry (may be NULL).
  *	@attr is the iattr structure containing the new file attributes.
  *	Return 0 if permission is granted.
  * @inode_getattr:

@@ -1221,7 +1222,8 @@ struct security_operations {
 	int (*inode_readlink) (struct dentry *dentry);
 	int (*inode_follow_link) (struct dentry *dentry, struct nameidata *nd);
 	int (*inode_permission) (struct inode *inode, int mask, struct nameidata *nd);
-	int (*inode_setattr)	(struct dentry *dentry, struct iattr *attr);
+	int (*inode_setattr) (struct dentry *dentry, struct vfsmount *mnt,
+			      struct iattr *attr);
 	int (*inode_getattr) (struct vfsmount *mnt, struct dentry *dentry);
         void (*inode_delete) (struct inode *inode);
 	int (*inode_setxattr) (struct dentry *dentry, char *name, void *value,

@@ -1708,11 +1710,12 @@ static inline int security_inode_permiss
 }
 
 static inline int security_inode_setattr (struct dentry *dentry,
+					  struct vfsmount *mnt,
 					  struct iattr *attr)
 {
 	if (unlikely (IS_PRIVATE (dentry->d_inode)))
 		return 0;
-	return security_ops->inode_setattr (dentry, attr);
+	return security_ops->inode_setattr (dentry, mnt, attr);
 }
 
 static inline int security_inode_getattr (struct vfsmount *mnt,

@@ -2412,6 +2415,7 @@ static inline int security_inode_permiss
 }
 
 static inline int security_inode_setattr (struct dentry *dentry,
+					  struct vfsmount *mnt,
 					  struct iattr *attr)
 {
 	return 0;

@@ -328,7 +328,8 @@ static int dummy_inode_permission (struc
 	return 0;
 }
 
-static int dummy_inode_setattr (struct dentry *dentry, struct iattr *iattr)
+static int dummy_inode_setattr (struct dentry *dentry, struct vfsmount *mnt,
+				struct iattr *iattr)
 {
 	return 0;
 }

@@ -2269,11 +2269,12 @@ static int selinux_inode_permission(stru
 			       file_mask_to_av(inode->i_mode, mask), NULL);
 }
 
-static int selinux_inode_setattr(struct dentry *dentry, struct iattr *iattr)
+static int selinux_inode_setattr(struct dentry *dentry, struct vfsmount *mnt,
+				 struct iattr *iattr)
 {
 	int rc;
 
-	rc = secondary_ops->inode_setattr(dentry, iattr);
+	rc = secondary_ops->inode_setattr(dentry, mnt, iattr);
 	if (rc)
 		return rc;
 

The vfsmount will be passed down to the LSM hook so that LSMs can compute pathnames.

---
 fs/ecryptfs/inode.c   |    5 ++++-
 fs/namei.c            |    5 +++--
 fs/nfsd/nfs4recover.c |    3 ++-
 fs/nfsd/vfs.c         |    3 ++-
 include/linux/fs.h    |    2 +-
 5 files changed, 12 insertions(+), 6 deletions(-)

@@ -509,11 +509,14 @@ static int ecryptfs_mkdir(struct inode *
 {
 	int rc;
 	struct dentry *lower_dentry;
+	struct vfsmount *lower_mnt;
 	struct dentry *lower_dir_dentry;
 
 	lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
 	lower_dir_dentry = lock_parent(lower_dentry);
-	rc = vfs_mkdir(lower_dir_dentry->d_inode, lower_dentry, mode);
+	rc = vfs_mkdir(lower_dir_dentry->d_inode, lower_dentry, lower_mnt,
+		       mode);
 	if (rc || !lower_dentry->d_inode)
 		goto out;
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb, 0);

@@ -1916,7 +1916,8 @@ asmlinkage long sys_mknod(const char __u
 	return sys_mknodat(AT_FDCWD, filename, mode, dev);
 }
 
-int vfs_mkdir(struct inode *dir, struct dentry *dentry, int mode)
+int vfs_mkdir(struct inode *dir, struct dentry *dentry, struct vfsmount *mnt,
+	      int mode)
 {
 	int error = may_create(dir, dentry, NULL);
 

@@ -1960,7 +1961,7 @@ asmlinkage long sys_mkdirat(int dfd, con
 
 	if (!IS_POSIXACL(nd.dentry->d_inode))
 		mode &= ~current->fs->umask;
-	error = vfs_mkdir(nd.dentry->d_inode, dentry, mode);
+	error = vfs_mkdir(nd.dentry->d_inode, dentry, nd.mnt, mode);
 	dput(dentry);
 out_unlock:
 	mutex_unlock(&nd.dentry->d_inode->i_mutex);

@@ -156,7 +156,8 @@ nfsd4_create_clid_dir(struct nfs4_client
 		dprintk("NFSD: nfsd4_create_clid_dir: DIRECTORY EXISTS\n");
 		goto out_put;
 	}
-	status = vfs_mkdir(rec_dir.dentry->d_inode, dentry, S_IRWXU);
+	status = vfs_mkdir(rec_dir.dentry->d_inode, dentry, rec_dir.mnt,
+			   S_IRWXU);
 out_put:
 	dput(dentry);
 out_unlock:

@@ -1186,7 +1186,8 @@ nfsd_create(struct svc_rqst *rqstp, stru
 		host_err = vfs_create(dirp, dchild, iap->ia_mode, NULL);
 		break;
 	case S_IFDIR:
-		host_err = vfs_mkdir(dirp, dchild, iap->ia_mode);
+		host_err = vfs_mkdir(dirp, dchild, fhp->fh_export->ex_mnt,
+				     iap->ia_mode);
 		break;
 	case S_IFCHR:
 	case S_IFBLK:

@@ -980,7 +980,7 @@ extern void unlock_super(struct super_bl
  */
 extern int vfs_permission(struct nameidata *, int);
 extern int vfs_create(struct inode *, struct dentry *, int, struct nameidata *);
-extern int vfs_mkdir(struct inode *, struct dentry *, int);
+extern int vfs_mkdir(struct inode *, struct dentry *, struct vfsmount *, int);
 extern int vfs_mknod(struct inode *, struct dentry *, int, dev_t);
 extern int vfs_symlink(struct inode *, struct dentry *, const char *, int);
 extern int vfs_link(struct dentry *, struct inode *, struct dentry *);

This is needed for computing pathnames in the AppArmor LSM.

---
 fs/namei.c               |    2 +-
 include/linux/security.h |    8 ++++++--
 security/dummy.c         |    2 +-
 security/selinux/hooks.c |    3 ++-
 4 files changed, 10 insertions(+), 5 deletions(-)

@@ -1928,7 +1928,7 @@ int vfs_mkdir(struct inode *dir, struct 
 		return -EPERM;
 
 	mode &= (S_IRWXUGO|S_ISVTX);
-	error = security_inode_mkdir(dir, dentry, mode);
+	error = security_inode_mkdir(dir, dentry, mnt, mode);
 	if (error)
 		return error;
 

@@ -308,6 +308,7 @@ struct request_sock;
  *	associated with inode strcture @dir. 
  *	@dir containst the inode structure of parent of the directory to be created.
  *	@dentry contains the dentry structure of new directory.
+ *	@mnt is the vfsmount corresponding to @dentry (may be NULL).
  *	@mode contains the mode of new directory.
  *	Return 0 if permission is granted.
  * @inode_rmdir:

@@ -1213,7 +1214,8 @@ struct security_operations {
 	int (*inode_unlink) (struct inode *dir, struct dentry *dentry);
 	int (*inode_symlink) (struct inode *dir,
 	                      struct dentry *dentry, const char *old_name);
-	int (*inode_mkdir) (struct inode *dir, struct dentry *dentry, int mode);
+	int (*inode_mkdir) (struct inode *dir, struct dentry *dentry,
+			    struct vfsmount *mnt, int mode);
 	int (*inode_rmdir) (struct inode *dir, struct dentry *dentry);
 	int (*inode_mknod) (struct inode *dir, struct dentry *dentry,
 	                    int mode, dev_t dev);

@@ -1650,11 +1652,12 @@ static inline int security_inode_symlink
 
 static inline int security_inode_mkdir (struct inode *dir,
 					struct dentry *dentry,
+					struct vfsmount *mnt,
 					int mode)
 {
 	if (unlikely (IS_PRIVATE (dir)))
 		return 0;
-	return security_ops->inode_mkdir (dir, dentry, mode);
+	return security_ops->inode_mkdir (dir, dentry, mnt, mode);
 }
 
 static inline int security_inode_rmdir (struct inode *dir,

@@ -2371,6 +2374,7 @@ static inline int security_inode_symlink
 
 static inline int security_inode_mkdir (struct inode *dir,
 					struct dentry *dentry,
+					struct vfsmount *mnt,
 					int mode)
 {
 	return 0;

@@ -288,7 +288,7 @@ static int dummy_inode_symlink (struct i
 }
 
 static int dummy_inode_mkdir (struct inode *inode, struct dentry *dentry,
-			      int mask)
+			      struct vfsmount *mnt, int mask)
 {
 	return 0;
 }

@@ -2209,7 +2209,8 @@ static int selinux_inode_symlink(struct 
 	return may_create(dir, dentry, SECCLASS_LNK_FILE);
 }
 
-static int selinux_inode_mkdir(struct inode *dir, struct dentry *dentry, int mask)
+static int selinux_inode_mkdir(struct inode *dir, struct dentry *dentry,
+			       struct vfsmount *mnt, int mask)
 {
 	return may_create(dir, dentry, SECCLASS_DIR);
 }

The vfsmount will be passed down to the LSM hook so that LSMs can compute pathnames.

---
 fs/ecryptfs/inode.c |    5 ++++-
 fs/namei.c          |   10 ++++++----
 fs/nfsd/vfs.c       |    3 ++-
 include/linux/fs.h  |    2 +-
 net/unix/af_unix.c  |    2 +-
 5 files changed, 14 insertions(+), 8 deletions(-)

@@ -560,11 +560,14 @@ ecryptfs_mknod(struct inode *dir, struct
 {
 	int rc;
 	struct dentry *lower_dentry;
+	struct vfsmount *lower_mnt;
 	struct dentry *lower_dir_dentry;
 
 	lower_dentry = ecryptfs_dentry_to_lower(dentry);
+	lower_mnt = ecryptfs_dentry_to_lower_mnt(dentry);
 	lower_dir_dentry = lock_parent(lower_dentry);
-	rc = vfs_mknod(lower_dir_dentry->d_inode, lower_dentry, mode, dev);
+	rc = vfs_mknod(lower_dir_dentry->d_inode, lower_dentry, lower_mnt, mode,
+		       dev);
 	if (rc || !lower_dentry->d_inode)
 		goto out;
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb, 0);