Skip to contentSkip to conversation

fs: block cross-uid sticky symlinks

PATCH Superseded Kees Cook 8 participants 23 messages May 31, 2010 – Jun 1, 2010

Subsystems (6)

v2 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH v2 1/1 fs: block cross-uid sticky symlinks

A long-standing class of security issues is the symlink-based time-of-check-time-of-use race, most commonly seen in world-writable directories like /tmp. The common method of exploitation of this flaw is to cross privilege boundaries when following a given symlink (i.e. a root process follows a symlink belonging to another user). For a likely incomplete list of hundreds of examples across the years, please see:
http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp

The solution is to permit symlinks to only be followed when outside a sticky world-writable directory, or when the uid of the symlink and follower match, or when the directory owner matches the symlink's owner.

Some pointers to the history of earlier discussion that I could find:

1996 Aug, Zygo Blaxell
http://marc.info/?l=bugtraq&m=87602167419830&w=2
1996 Oct, Andrew Tridgell
http://lkml.indiana.edu/hypermail/linux/kernel/9610.2/0086.html
1997 Dec, Albert D Cahalan
http://lkml.org/lkml/1997/12/16/4
2005 Feb, Lorenzo Hernández García-Hierro
http://lkml.indiana.edu/hypermail/linux/kernel/0502.0/1896.html

Past objections and rebuttals could be summarized as:

- Violates POSIX.
- POSIX didn't consider this situation and it's not useful to follow a broken specification at the cost of security.
- Might break unknown applications that use this feature.
- Applications that break because of the change are easy to spot and fix. Applications that are vulnerable to symlink ToCToU by not having the change aren't.
- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written all the time that makes these mistakes; blocking this flaw at the kernel is a single solution to the entire class of vulnerability.

This patch is based on the patch in Openwall and grsecurity. I have added a sysctl to toggle the behavior back to the old logic via /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited warning.

v2:
- dropped redundant S_ISLNK check.
- moved sysctl extern into security.h.
- asked to include CC to linux-fsdevel.

---
 Documentation/sysctl/kernel.txt |   16 ++++++++++++++++
 include/linux/security.h        |    3 +++
 kernel/sysctl.c                 |    9 +++++++++
 security/capability.c           |    6 ------
 security/commoncap.c            |   24 ++++++++++++++++++++++++
 5 files changed, 52 insertions(+), 6 deletions(-)
Documentation/sysctl/kernel.txt +16 -0
index 3894eaa..6b059f6 100644
@@ -66,6 +66,7 @@ show up in /proc/sys/kernel:
 - threads-max
 - unknown_nmi_panic
 - version
+- weak-sticky-symlinks
 
 ==============================================================
 
@@ -526,3 +527,18 @@ A small number of systems do generate NMI's for bizarre random reasons such as
 power management so the default is off. That sysctl works like the existing
 panic controls already in that directory.
 
+==============================================================
+
+weak-sticky-symlinks:
+
+Following symlinks in world-writable sticky directories (like /tmp) can
+be dangerous due to time-of-check-time-of-use races that frequently result
+in security vulnerabilities.  By default, symlinks can only be followed in
+sticky world-writable directories if the symlink and the follower's uid
+match (or if the symlink is owned by the owner of the world-writable directory
+itself).
+
+The default value is "0".  To disable this protection, setting a value of "1"
+will allow symlinks in sticky world-writable directories to be followed by
+anyone.
+
include/linux/security.h +3 -0
index 0c88191..ee31647 100644
@@ -43,6 +43,8 @@
 #define SECURITY_CAP_NOAUDIT 0
 #define SECURITY_CAP_AUDIT 1
 
+extern int weak_sticky_symlinks;
+
 struct ctl_table;
 struct audit_krule;
 
@@ -67,6 +69,7 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
 extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
 extern int cap_inode_need_killpriv(struct dentry *dentry);
 extern int cap_inode_killpriv(struct dentry *dentry);
+extern int cap_inode_follow_link(struct dentry *dentry, struct nameidata *nd);
 extern int cap_file_mmap(struct file *file, unsigned long reqprot,
 			 unsigned long prot, unsigned long flags,
 			 unsigned long addr, unsigned long addr_only);
kernel/sysctl.c +9 -0
index 997080f..cd7fee4 100644
@@ -1463,6 +1463,15 @@ static struct ctl_table fs_table[] = {
 		.extra1		= &zero,
 		.extra2		= &two,
 	},
+	{
+		.procname	= "weak-sticky-symlinks",
+		.data		= &weak_sticky_symlinks,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &one,
+	},
 #if defined(CONFIG_BINFMT_MISC) || defined(CONFIG_BINFMT_MISC_MODULE)
 	{
 		.procname	= "binfmt_misc",
security/capability.c +0 -6
index 8168e3e..ff34291 100644
@@ -169,12 +169,6 @@ static int cap_inode_readlink(struct dentry *dentry)
 	return 0;
 }
 
-static int cap_inode_follow_link(struct dentry *dentry,
-				 struct nameidata *nameidata)
-{
-	return 0;
-}
-
 static int cap_inode_permission(struct inode *inode, int mask)
 {
 	return 0;
security/commoncap.c +24 -0
index 4e01599..0ff07a6 100644
@@ -29,6 +29,9 @@
 #include <linux/securebits.h>
 #include <linux/syslog.h>
 
+/* sysctl for symlink permissions checking */
+int weak_sticky_symlinks;
+
 /*
  * If a non-root user executes a setuid-root binary in
  * !secure(SECURE_NOROOT) mode, then we raise capabilities.
@@ -281,6 +284,27 @@ int cap_inode_killpriv(struct dentry *dentry)
 	return inode->i_op->removexattr(dentry, XATTR_NAME_CAPS);
 }
 
+int cap_inode_follow_link(struct dentry *dentry,
+			  struct nameidata *nameidata)
+{
+	const struct inode *parent = dentry->d_parent->d_inode;
+	const struct inode *inode = dentry->d_inode;
+	const struct cred *cred = current_cred();
+
+	if (weak_sticky_symlinks)
+		return 0;
+
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid &&
+	    cred->fsuid != inode->i_uid) {
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);
+		return -EACCES;
+	}
+	return 0;
+}
+
 /*
  * Calculate the new process capability sets from the capability sets attached
  * to a file.
Signature · 6 lines
1.7.0.4


-- 
Kees Cook
Ubuntu Security Team
#1 lore raw
Quoted 37 lines · Kees Cook

A long-standing class of security issues is the symlink-based time-of-check-time-of-use race, most commonly seen in world-writable directories like /tmp. The common method of exploitation of this flaw is to cross privilege boundaries when following a given symlink (i.e. a root process follows a symlink belonging to another user). For a likely incomplete list of hundreds of examples across the years, please see:
http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp

The solution is to permit symlinks to only be followed when outside a sticky world-writable directory, or when the uid of the symlink and follower match, or when the directory owner matches the symlink's owner.

Some pointers to the history of earlier discussion that I could find:

1996 Aug, Zygo Blaxell
http://marc.info/?l=bugtraq&m=87602167419830&w=2
1996 Oct, Andrew Tridgell
http://lkml.indiana.edu/hypermail/linux/kernel/9610.2/0086.html
1997 Dec, Albert D Cahalan
http://lkml.org/lkml/1997/12/16/4
2005 Feb, Lorenzo Hernández García-Hierro
http://lkml.indiana.edu/hypermail/linux/kernel/0502.0/1896.html

Past objections and rebuttals could be summarized as:

- Violates POSIX.
- POSIX didn't consider this situation and it's not useful to follow a broken specification at the cost of security.
- Might break unknown applications that use this feature.
- Applications that break because of the change are easy to spot and fix. Applications that are vulnerable to symlink ToCToU by not having the change aren't.
- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written all the time that makes these mistakes; blocking this flaw at the kernel is a single solution to the entire class of vulnerability.

Quoted 4 lines

This patch is based on the patch in Openwall and grsecurity. I have added a sysctl to toggle the behavior back to the old logic via /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited warning.

The name of the sysctl is horrible it is a double negative, which makes thinking about it hard.

Why not simply put each user in a different mount namespace and have separate /tmp directories per user? That works today, with no kernel changes.

Placing this in cap_inode_follow_link is horrible naming. There is nothing capabilities about this. Either this needs to go into one or several of the security modules or this needs to go into the core vfs.

I can't argue with taking action to close the too frequently security issues in /tmp, but this changes appears to be unnecessary, difficult to maintain, and difficult to understand.

Eric

author
#2 lore raw

Hi Eric,

Quoted 2 lines · Eric W. Biederman

The name of the sysctl is horrible it is a double negative, which makes thinking about it hard.

Hmm, I see your point, the "safe" value is "weak: not". I was trying to be descriptive without needing a "true" value, but I guess that's silly; we already have things like randomize_va_space set to "2" by default, etc. What would you suggest instead? "protected_sticky_symlinks" (and reverse the default and test logic)?

Quoted 2 lines

Why not simply put each user in a different mount namespace and have separate /tmp directories per user? That works today, with no kernel changes.

The key here is "no kernel changes" -- trying to isolate every user and service from each other using different mount namespaces will not work quickly in current distributions. Even doing bind-mount tricks to keep /tmp away from different users is overkill, especially when you have situations like "screen" using a common /tmp directory tree (in the setuid version), etc. Things (correctly) expect to share /tmp in some cases. However, this one kernel change will allow everything to continue without userspace overhead and without breaking anything terribly. Using containers will probably be the future, but I want to solve this in the general case today.

Quoted 3 lines

Placing this in cap_inode_follow_link is horrible naming. There is nothing capabilities about this. Either this needs to go into one or several of the security modules or this needs to go into the core vfs.

My thinking was that most of the LSMs call down to commoncaps first, so it's a single place to put this. When I was looking at this code originally, I thought that if it doesn't go in security_inode_follow_link, then a new function would be added to the VFS and both callers of security_inode_follow_link would need to call it just before security_inode_follow_link. It seemed like putting it in there reduced duplication of logic.

However, on closer examination, it seems that this code could live in __do_follow_link instead.

fs/namei.c:
...

                error = security_inode_follow_link(path.dentry, &nd);
                if (error)
                        goto exit_dput;
                error = __do_follow_link(&path, &nd, &cookie);
...
        err = security_inode_follow_link(path->dentry, nd);
        if (err)
                goto loop;
        current->link_count++;
        current->total_link_count++;
        nd->depth++;
        err = __do_follow_link(path, nd, &cookie);

...

What would you suggest for the best approach here?

Quoted 3 lines

I can't argue with taking action to close the too frequently security issues in /tmp, but this changes appears to be unnecessary, difficult to maintain, and difficult to understand.

Well, we disagree about "unnecessary". :) Finding an easy to maintain solution is my goal here, and if it's difficult to understand, then I need to fix that too. What could use better clarification?

Thanks!

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#3 lore raw
In reply to Kees Cook
Quoted 42 lines · Kees Cook

A long-standing class of security issues is the symlink-based time-of-check-time-of-use race, most commonly seen in world-writable directories like /tmp. The common method of exploitation of this flaw is to cross privilege boundaries when following a given symlink (i.e. a root process follows a symlink belonging to another user). For a likely incomplete list of hundreds of examples across the years, please see:
http://cve.mitre.org/cgi-bin/cvekey.cgi?keyword=/tmp

The solution is to permit symlinks to only be followed when outside a sticky world-writable directory, or when the uid of the symlink and follower match, or when the directory owner matches the symlink's owner.

Some pointers to the history of earlier discussion that I could find:

1996 Aug, Zygo Blaxell
http://marc.info/?l=bugtraq&m=87602167419830&w=2
1996 Oct, Andrew Tridgell
http://lkml.indiana.edu/hypermail/linux/kernel/9610.2/0086.html
1997 Dec, Albert D Cahalan
http://lkml.org/lkml/1997/12/16/4
2005 Feb, Lorenzo Hernández García-Hierro
http://lkml.indiana.edu/hypermail/linux/kernel/0502.0/1896.html

Past objections and rebuttals could be summarized as:

- Violates POSIX.
- POSIX didn't consider this situation and it's not useful to follow a broken specification at the cost of security.
- Might break unknown applications that use this feature.
- Applications that break because of the change are easy to spot and fix. Applications that are vulnerable to symlink ToCToU by not having the change aren't.
- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written all the time that makes these mistakes; blocking this flaw at the kernel is a single solution to the entire class of vulnerability.

This patch is based on the patch in Openwall and grsecurity. I have added a sysctl to toggle the behavior back to the old logic via /proc/sys/fs/weak-sticky-symlinks, documentation, and a ratelimited warning.

Quoted 4 lines

v2:
- dropped redundant S_ISLNK check.
- moved sysctl extern into security.h.
- asked to include CC to linux-fsdevel.

We need to call this function in the SELinux case. So you'll need a patch like the one attached (not even compiled but I think it is right)

security/selinux/hooks.c +5 -0
index 5c9f25b..d6ebee2 100644
@@ -2668,8 +2668,13 @@ static int selinux_inode_readlink(struct dentry *dentry)
 
 static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
 {
+       int rc;
        const struct cred *cred = current_cred();
 
+       rc = cap_inode_follow_link(dentry, nameidata);
+       if (rc)
+               return rc;
+
        return dentry_has_perm(cred, NULL, dentry, FILE__READ);
 }
 
Quoted 10 lines
(quoted diff) +10 -0
+int cap_inode_follow_link(struct dentry *dentry,
+			  struct nameidata *nameidata)
+{
+	const struct inode *parent = dentry->d_parent->d_inode;
+	const struct inode *inode = dentry->d_inode;
+	const struct cred *cred = current_cred();
+
+	if (weak_sticky_symlinks)
+		return 0;
+

Review

Quoted 10 lines
(quoted diff) +10 -0
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid &&
+	    cred->fsuid != inode->i_uid) {
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);
+		return -EACCES;
+	}
+	return 0;
+}

What stops us from racing between the assignment of parent and it's first use with a rename on our object and rmdir on the old parent? I'm wondering if we need to be doing this test holding dentry->d_lock (which is what protects dentry->d_parent if I recall correctly)

Certainly doesn't fix all of the raciness, but I think it would close the opps part. Maybe someone who knows the VFS better can tell me if I am misguided.

-Eric

author
#4 lore raw

Hi Eric,

Quoted 6 lines · Eric Paris

We need to call this function in the SELinux case. So you'll need a
patch like the one attached (not even compiled but I think it is right) [..]
static int selinux_inode_follow_link(struct dentry *dentry, struct nameidata *nameidata)
[..]
+ rc = cap_inode_follow_link(dentry, nameidata);

Yeah, when I quickly checked SELinux and AppArmor, it seemed that they were always calling down to all commoncaps functions, but it looks like not in all cases. I think that Eric Biederman's observations here makes the most sense: this check needs to happen without involving the LSMs at all.

Quoted 26 lines
Quoted 10 lines
(quoted diff) +10 -0
+int cap_inode_follow_link(struct dentry *dentry,
+			  struct nameidata *nameidata)
+{
+	const struct inode *parent = dentry->d_parent->d_inode;
+	const struct inode *inode = dentry->d_inode;
+	const struct cred *cred = current_cred();
+
+	if (weak_sticky_symlinks)
+		return 0;
+

Review

Quoted 10 lines
(quoted diff) +10 -0
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid &&
+	    cred->fsuid != inode->i_uid) {
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);
+		return -EACCES;
+	}
+	return 0;
+}

What stops us from racing between the assignment of parent and it's first use with a rename on our object and rmdir on the old parent? I'm wondering if we need to be doing this test holding dentry->d_lock (which is what protects dentry->d_parent if I recall correctly)

Quoted 3 lines

Certainly doesn't fix all of the raciness, but I think it would close the opps part. Maybe someone who knows the VFS better can tell me if I am misguided.

The only other use of d_parent I can see there is in may_delete(). With vfs_unlink() calling that, it would seem to be racey too if we needed to hold a lock for that. But it's not clear to me in vfs_follow_link is doing locking somehow.

Thanks,

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#5 lore raw
In reply to Kees Cook
Quoted 1 line

Past objections and rebuttals could be summarized as:

You missed a couple at least

1.We have things like SELinux so you can write rules to bound apps
which should be able to do this, if not then we should fix the security policy to generally solve it

2. If it worries you that much then you can *fix* /tmp/ for good using the work Al Viro did years ago on mountpoints. Give people their own /tmp. End of problem.

Quoted 3 lines

- Violates POSIX.
- POSIX didn't consider this situation and it's not useful to follow a broken specification at the cost of security.

(If you fill your computer with concrete it will also violate POSIX but it will be more secure than your proposal. Both will of course has some impact on applications).

Quoted 3 lines

- Might break unknown applications that use this feature.
- Applications that break because of the change are easy to spot and fix. Applications that are vulnerable to symlink ToCToU by not having

What if they are things like binary only games - now you can neither turn on your feature nor fix the app if you want to use it. If it was using SELinux or similar rules you could create a single exception rule. If it was using per user /tmp nobody would have to do anything

Quoted 4 lines

- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written all the time that makes these mistakes; blocking this flaw at the kernel is a single solution to the entire class of vulnerability.

So new software is also being written all the time which has other holes. Actually its also according to your argument being written all the time so it'll break if you turn the feature on ?

Documentation/sysctl/kernel.txt A small number of systems do generate NMI's for bizarre random reasons such as · line 543

Quoted 3 lines
Documentation/sysctl/kernel.txt +3 -0
+The default value is "0".  To disable this protection, setting a value of "1"
+will allow symlinks in sticky world-writable directories to be followed by
+anyone.

Wrong way around. You don't enable non-compliance and misbehaviour by default. For it to be any use you need a userspace adapted to run on your odd environment, so your distro can flip the flag from Linux mode to odd.

security/commoncap.c int cap_inode_killpriv(struct dentry *dentry) · line 302

Quoted 6 lines
security/commoncap.c +6 -0
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid &&
+	    cred->fsuid != inode->i_uid) {
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);

This is of minimal use - current->comm is arbitary and user controlled. Also are we guaranteed to hold enough locks here to stop the parent uid changing ?

You've also stopped root doing this. Given DAC override that makes no sense and is asking to confuse stuff.

Give your users their own /tmp. No kernel mods, no misbehaviours, no weirdomatic path walking hackery. No kernel patch needed that I can see.

Alan

author
#6 lore raw

Hi Alan,

Quoted 3 lines · Alan Cox

1.We have things like SELinux so you can write rules to bound apps
which should be able to do this, if not then we should fix the security policy to generally solve it

This requires that policy be written for all applications ever, which isn't feasible.

Quoted 3 lines

2. If it worries you that much then you can *fix* /tmp/ for good using the work Al Viro did years ago on mountpoints. Give people their own /tmp. End of problem.

This has the same flaws in interoperability that you mention below for hypothetical binary games.

Quoted 3 lines

(If you fill your computer with concrete it will also violate POSIX but it will be more secure than your proposal. Both will of course has some impact on applications).

Well, my point was that in the past, a stand-alone objection was that it violates POSIX. I don't feel that this is a sufficient objection -- POSIX did not consider this corner-case. Filling my computer with concrete has a nasty side-effect of making all my applications fail. This patch isn't that intrusive. :)

Quoted 4 lines

What if they are things like binary only games - now you can neither turn on your feature nor fix the app if you want to use it. If it was using SELinux or similar rules you could create a single exception rule. If it was using per user /tmp nobody would have to do anything

I do not believe there are any applications that depend on following symlinks in sticky world-writable directories. Besides, just because something lacks source doesn't mean I can't change its behavior via binary editing or LD_PRELOAD. I do not feel that non-existent hypothetical applications needing this feature is a sufficient reason to reject the patch.

Quoted 8 lines
Quoted 4 lines

- Applications should just use mkstemp() or O_CREATE|O_EXCL.
- True, but applications are not perfect, and new software is written all the time that makes these mistakes; blocking this flaw at the kernel is a single solution to the entire class of vulnerability.

So new software is also being written all the time which has other holes. Actually its also according to your argument being written all the time so it'll break if you turn the feature on ?

Yes, everything is buggy -- I am seeking to solve a specific class of security issue so that those bugs don't turn into an more serious problem. One that has been fully solved with no known bad side-effects in a hand-full of distros for possibly more than 10 years now.

Quoted 7 lines

Documentation/sysctl/kernel.txt A small number of systems do generate NMI's for bizarre random reasons such as · line 543

Quoted 3 lines
Documentation/sysctl/kernel.txt +3 -0
+The default value is "0".  To disable this protection, setting a value of "1"
+will allow symlinks in sticky world-writable directories to be followed by
+anyone.

Wrong way around. You don't enable non-compliance and misbehaviour by default. For it to be any use you need a userspace adapted to run on your odd environment, so your distro can flip the flag from Linux mode to odd.

I'm happy to make that adjustment.

Quoted 8 lines

security/commoncap.c int cap_inode_killpriv(struct dentry *dentry) · line 302

Quoted 6 lines
security/commoncap.c +6 -0
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid &&
+	    cred->fsuid != inode->i_uid) {
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);

This is of minimal use - current->comm is arbitary and user controlled.

It's used strictly for reporting. I'm open to other suggestions -- I just want to give a system admin some information in the case where an application is being attacked with /tmp symlink races.

Quoted 2 lines

Also are we guaranteed to hold enough locks here to stop the parent uid changing ?

I don't know the answer to that -- I'm happy to add any needed locking.

Quoted 2 lines

You've also stopped root doing this. Given DAC override that makes no sense and is asking to confuse stuff.

This should specifically ignore DAC override. In most cases, it is root we are trying to protect from the symlink race.

Quoted 2 lines

Give your users their own /tmp. No kernel mods, no misbehaviours, no weirdomatic path walking hackery. No kernel patch needed that I can see.

Some real applications expect to share /tmp (e.g. "screen"). Splitting up /tmp for every user isn't quickly feasible and requires every distro change. I would note that separate /tmp also breaks the hypothetical binary game if it really is required to follow cross-uid symlinks.

I know that for the VFS folks, this is not a popular patch. I have no illusions there, but I want to find a kernel solution that is acceptable. Fixing this in the kernel is trivial. Fixing this in userspace is not trivial -- it requires that every distro change the very semantics of how /tmp works. I can't say that's a bad idea, but I can say that it will not be globally successful. This kernel changes breaks no applications, and fixes a whole class of problem forever, is small, and has well-defined rules.

I would prefer it be solved at the VFS layer. Doing it in commoncaps is possible (and is even what I proposed), but I want to avoid having everyone punt this patch around to different subsystem maintainers.

It's a real problem. I have proposed a real solution. If it's not okay, I'd like to see an alternative solution that you're comfortable with that fixes it to the same global scope. (i.e. not userspace or tied to a specific LSM.)

Thanks,

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#7 lore raw
Quoted 9 lines · On Mon, 31 May 2010 10:50:08 -0700 Kees Cook <[email protected]> wrote:

Hi Alan,

Quoted 3 lines · Alan Cox

1.We have things like SELinux so you can write rules to bound apps
which should be able to do this, if not then we should fix the security policy to generally solve it

This requires that policy be written for all applications ever, which isn't feasible.

Wrong, you can write a generic policy then write exceptions.

Quoted 6 lines
Quoted 3 lines

2. If it worries you that much then you can *fix* /tmp/ for good using the work Al Viro did years ago on mountpoints. Give people their own /tmp. End of problem.

This has the same flaws in interoperability that you mention below for hypothetical binary games.

Not that I can see - please explain.

Quoted 2 lines

nasty side-effect of making all my applications fail. This patch isn't that intrusive. :)

I would disagree there, and it seems Christoph and others likjewise.

Quoted 6 lines

I do not believe there are any applications that depend on following symlinks in sticky world-writable directories. Besides, just because something lacks source doesn't mean I can't change its behavior via binary editing or LD_PRELOAD. I do not feel that non-existent hypothetical applications needing this feature is a sufficient reason to reject the patch.

Maybe you are right - but you can do this without a kernel patch or as a security module and leave the rest of the kernel alone.

Quoted 4 lines

Yes, everything is buggy -- I am seeking to solve a specific class of security issue so that those bugs don't turn into an more serious problem. One that has been fully solved with no known bad side-effects in a hand-full of distros for possibly more than 10 years now.

Plan 9 solved this years ago. Linux added the necessary per user /tmp (or per session /tmp if you prefer) support years ago.

Quoted 9 lines
Quoted 5 lines

security/commoncap.c int cap_inode_killpriv(struct dentry *dentry) · line 302

Quoted 3 lines
security/commoncap.c +3 -0
+		printk_ratelimited(KERN_NOTICE "non-matching-uid symlink "
+			"following attempted in sticky-directory by "
+			"%s (fsuid %d)\n", current->comm, cred->fsuid);

This is of minimal use - current->comm is arbitary and user controlled.

It's used strictly for reporting. I'm open to other suggestions -- I just want to give a system admin some information in the case where an application is being attacked with /tmp symlink races.

current->comm can be a random mess of control codes, it could include a newline to fool the printk paths. You don't printk user provided untrusted bits...

Quoted 2 lines

This should specifically ignore DAC override. In most cases, it is root we are trying to protect from the symlink race.

So how does root override the DAC override ignore when she needs to ?

Quoted 8 lines
Quoted 3 lines

Give your users their own /tmp. No kernel mods, no misbehaviours, no weirdomatic path walking hackery. No kernel patch needed that I can see.

Some real applications expect to share /tmp (e.g. "screen"). Splitting up /tmp for every user isn't quickly feasible and requires every distro change. I would note that separate /tmp also breaks the hypothetical binary game if it really is required to follow cross-uid symlinks.

I've yet to meet a game that tries to share a /tmp file across users.

Quoted 6 lines

Fixing this in the kernel is trivial. Fixing this in userspace is not trivial -- it requires that every distro change the very semantics of how /tmp works. I can't say that's a bad idea, but I can say that it will not be globally successful. This kernel changes breaks no applications, and fixes a whole class of problem forever, is small, and has well-defined rules.

At best you fix one of a zillion /tmp problems and corner cases from rootfs cross linking into /var/mail to quota avoidance. Putting /tmp into a per user context fixes the lot (and doesn't break screen for users).

Quoted 3 lines

I would prefer it be solved at the VFS layer. Doing it in commoncaps is possible (and is even what I proposed), but I want to avoid having everyone punt this patch around to different subsystem maintainers.

You can do it in an existing LSM, you can write your own new LSM if you want. This is why we have LSM hooks.

Quoted 4 lines

It's a real problem. I have proposed a real solution. If it's not okay, I'd like to see an alternative solution that you're comfortable with that fixes it to the same global scope. (i.e. not userspace or tied to a specific LSM.)

The fact you can do it already rather clearly says your patch is not needed. If you must do it then write your own Ubuntu LSM, that way nobody has to care. If you want to actually *fix* /tmp then you want a per login user or per session /tmp. A per session /tmp does confuse a few things, a per login user tmp works nicely. About the only thing you do need to tweak slightly is the gdm/kdm handing off of the session - both to do the mount and to migrate the X socket into it.

Alan

author
#8 lore raw
Quoted 11 lines · Alan Cox
Quoted 7 lines · On Mon, 31 May 2010 10:50:08 -0700 Kees Cook <[email protected]> wrote:
Quoted 3 lines · Alan Cox

1.We have things like SELinux so you can write rules to bound apps
which should be able to do this, if not then we should fix the security policy to generally solve it

This requires that policy be written for all applications ever, which isn't feasible.

Wrong, you can write a generic policy then write exceptions.

This depends on your LSM. And some people run with no LSM at all.

Quoted 4 lines
Quoted 2 lines

nasty side-effect of making all my applications fail. This patch isn't that intrusive. :)

I would disagree there, and it seems Christoph and others likjewise.

I think there is a difference between "offensive" and "intrusive". The patch itself is about 4 lines of logic. That in itself isn't intrusive. But a lot of people (now and in the past) find the change in VFS semantics offensive. What I'd like to do is get people to really think about what this change means -- it's blocking a specific pathological situation that has no positive outcome. If we can all agree "this needs to be fixed", then we can work on a common solution.

Quoted 9 lines
Quoted 6 lines

I do not believe there are any applications that depend on following symlinks in sticky world-writable directories. Besides, just because something lacks source doesn't mean I can't change its behavior via binary editing or LD_PRELOAD. I do not feel that non-existent hypothetical applications needing this feature is a sufficient reason to reject the patch.

Maybe you are right - but you can do this without a kernel patch or as a security module and leave the rest of the kernel alone.

I see this as a flaw in DAC. As such, I'd like to see it fixed globally.

Quoted 2 lines

Plan 9 solved this years ago. Linux added the necessary per user /tmp (or per session /tmp if you prefer) support years ago.

Right, I don't disagree on this point -- having per-user /tmp is great, but it hasn't happened because it's not trivial. I feel that fixing symlink following and per-user /tmp are tangential issues, both worthy of being fixed. And I realize we disagree on this point. :)

Quoted 9 lines
Quoted 5 lines
Quoted 1 line

This is of minimal use - current->comm is arbitary and user controlled.

It's used strictly for reporting. I'm open to other suggestions -- I just want to give a system admin some information in the case where an application is being attacked with /tmp symlink races.

current->comm can be a random mess of control codes, it could include a newline to fool the printk paths. You don't printk user provided untrusted bits...

Right, of course. I was, however, modelling this off of existing warnings, though perhaps I didn't do it right. Here is kernel/capabilities.c, warn_legacy_capability_use():

printk(KERN_INFO "warning: `%s' uses 32-bit capabilities"
" (legacy support in use)\n", get_task_comm(name, current));

If this is unsafe, then all users of get_task_comm() should be audited. I had the impression that current->comm was sanitized very early... hm, it's not. It's sanitized for newlines in fs/proc/array.c task_name(), but not by other callers. Seems like the sanitizing should happen in get_task_comm(). Thoughts?

Quoted 1 line

So how does root override the DAC override ignore when she needs to ?

By turning off the entire sysctl. It's designed to protect privileged users, so it doesn't make sense to have it be bypassed.

Quoted 1 line

I've yet to meet a game that tries to share a /tmp file across users.

Well, right, that was my point -- no such application needs to follow symlinks in /tmp across users. Such symlinks would be impossible to create with per-user /tmp mounts. So, if per-user /tmp is a viable solution to symlink races, then so is denying cross-user symlink following.

Quoted 3 lines

At best you fix one of a zillion /tmp problems and corner cases from rootfs cross linking into /var/mail to quota avoidance. Putting /tmp into a per user context fixes the lot (and doesn't break screen for users).

IIRC, screen, when setuid, allows users to share screen sessions (following some system-defined ACLs) but it does it via the /tmp directory trees it creates. Per-user /tmp would break this (but yes, it's solvable using some kind of /var/lib/screen which maybe even already exists).

Quoted 2 lines

You can do it in an existing LSM, you can write your own new LSM if you want. This is why we have LSM hooks.

Right, I'm well aware of that. I was hoping to solve this problem for all Linux users, since it has no operational downside.

Quoted 3 lines

The fact you can do it already rather clearly says your patch is not needed. If you must do it then write your own Ubuntu LSM, that way nobody has to care.

I'm slightly sensitive here, as people (hi Greg KH :P) like to take pot-shots at Ubuntu sometimes, so apologies if I've misunderstood you. For some reason you've specifically brought up Ubuntu, which makes me wonder if this patch would have been received differently if I had sent it from my personal email account. Regardless, you seem to be saying a few things, and I want to make sure I get it:

1) "I don't want this change to symlink logic in the mainline kernel"
2) "Go add it to Ubuntu only"
3) "Nobody will care if Ubuntu carries this patch"

I'm hoping to change your mind about #1 -- it has no downsides and only has benefits.

#2 is already done -- but my goal is to protect everyone else. While a large share of Linux users run an Ubuntu kernel, there are still a lot of other people that will continue to suffer from this vulnerability, and I have what I feel is a moral obligation to try to get this fixed globally.

For #3, if by "nobody" you mean "no Linux kernel developer", then I think your opinion lack credence since I consider myself a Linux kernel developer (not a huge one, but I do contribute). And I do care about this, obviously. If by "nobody" you mean "nobody using Linux", then I again think your opinion lacks credence since I've already had a few people glad to see this fixed in Ubuntu. That's more than zero people, so not "nobody". If instead, by "nobody" you mean "no Linux VFS developer" than I'm just puzzled since that would imply that all Linux VFS developers don't care about fixing this problem (which I can't believe). Or at least they don't care if the semantics change, as long as it's not in the VFS core, which also makes no sense to me. Either the logic changes or it doesn't. And since it needs to be changed, it seems that everyone should be working together to fix it.

If instead, you mean "if it's changed in an LSM, then VFS developers don't have to think about it", then I remain puzzled, since it's so clearly in need of being fixed. What does it matter where it goes?

Quoted 5 lines

If you want to actually *fix* /tmp then you want a per
login user or per session /tmp. A per session /tmp does confuse a few things, a per login user tmp works nicely. About the only thing you do need to tweak slightly is the gdm/kdm handing off of the session - both to do the mount and to migrate the X socket into it.

I totally agree. But I view it as tangential to solving symlink races (even though it does address it, that solution is a much different endeavour).

What is the path to take for having this change in the mainline kernel, and LSM-agnostic? I want to see it solved globally.

Thanks,

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
maintainer
#9 lore raw
Quoted 4 lines · Kees Cook

IIRC, screen, when setuid, allows users to share screen sessions (following some system-defined ACLs) but it does it via the /tmp directory trees it creates. Per-user /tmp would break this (but yes, it's solvable using some kind of /var/lib/screen which maybe even already exists).

screen(1) does *not* put directories in /tmp these days, TYVM.

al@duke:~/linux/trees/vfs-next$ ls -l /var/run/screen/
total 1
drwx------ 2 al al 1024 May 20 21:50 S-al

That's lenny/x86_64; I can't be arsed to install ubuntu, but in case you have a really ancient screen(1), pulling one from debian -stable would suffice. IIRC, -oldstable would work as well, actually, but I could be wrong on that.

In any case, the suggested "improvement" breaks realistic use cases, AFAICS. In particular,

cd /tmp
tar jxf foo-2.42.orig.tar.bz2
<...>
tar jxf foo-gtk-wank-wank-wank-2.69.orig.tar.bz2 <...>
ln -s foo-gtk-wank-wank-wank-2.69/docs/GNOME/design/ crap <...>
lpr crap/taste-is-optional.ps
lpr crap/why-options-are-wrong.ps

is going to break with that, isn't it?

author
#10 lore raw

Hi Al,

Quoted 11 lines · Al Viro
Quoted 4 lines · Kees Cook

IIRC, screen, when setuid, allows users to share screen sessions (following some system-defined ACLs) but it does it via the /tmp directory trees it creates. Per-user /tmp would break this (but yes, it's solvable using some kind of /var/lib/screen which maybe even already exists).

screen(1) does *not* put directories in /tmp these days, TYVM.

al@duke:~/linux/trees/vfs-next$ ls -l /var/run/screen/
total 1
drwx------ 2 al al 1024 May 20 21:50 S-al

Okay, good; that's a relief.

Quoted 13 lines

In any case, the suggested "improvement" breaks realistic use cases, AFAICS. In particular,

cd /tmp
tar jxf foo-2.42.orig.tar.bz2
<...>
tar jxf foo-gtk-wank-wank-wank-2.69.orig.tar.bz2 <...>
ln -s foo-gtk-wank-wank-wank-2.69/docs/GNOME/design/ crap <...>
lpr crap/taste-is-optional.ps
lpr crap/why-options-are-wrong.ps

Quoted 1 line

is going to break with that, isn't it?

Nope. To be fair, it depends on the implementation of of LPR. In the case of CUPS, this is fine since lpr will run as the local user, follow the symlink and read the file contents before POSTing the contents to the CUPS server. The privilege boundary is crossed at the network, not the filesystem in this case.

I would note however that without the symlink following patch a hypothetical attacker would be able to race you for the "foo-gtk-wank-wank-wank-2.69" entry, or the "crap" entry, since either could be directed out from under "tar" and "ln" to symlinks controlled by the attacker. Unpacking archives in a sticky world-writable directory is dangerous without this symlink following patch.

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
maintainer
#11 lore raw
In reply to Kees Cook
Quoted 4 lines · Kees Cook
Quoted 2 lines

Give your users their own /tmp. No kernel mods, no misbehaviours, no weirdomatic path walking hackery. No kernel patch needed that I can see.

Some real applications expect to share /tmp (e.g. "screen").

/tmp/screen-exchange, you mean? Such a brilliant idea on multiuser system...

#12 lore raw
In reply to Kees Cook

NAK for changing this in core code. These are long-living and documented semantics that we can't simply break, and as Alan mentioned having there's good enough workaround that don't break applications.

Feel free to shovel it into the crackpot LSM of your choice.

author
#13 lore raw

Hi Christoph,

Quoted 3 lines · Christoph Hellwig

NAK for changing this in core code. These are long-living and documented semantics that we can't simply break, and as Alan mentioned having there's good enough workaround that don't break applications.

I totally disagree -- there are no applications that depend on this behavior in sticky world-writable directories. Adding this logic (where ever it lands) fixes /tmp symlink races forever and breaks nothing.

Quoted 1 line

Feel free to shovel it into the crackpot LSM of your choice.

I have no strong opinion about where it should live. The strong opinion I have is that all Linux users, regardless of LSM choice, should benefit from the fix.

I'd like to separate objections about implementation from objections about the change in semantics itself. If every user of Linux gains this symlink protection, does it matter if it's in core VFS or in commoncaps?

Expecting the push-back from VFS, I wrote this patch against commoncaps.

James, would you take it there (along with the patch to SELinux to call out to commoncaps) if there is no way to proceed with the VFS core towards solving this?

Thanks,

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
maintainer
#14 lore raw
Quoted 5 lines · Kees Cook

Expecting the push-back from VFS, I wrote this patch against commoncaps.

James, would you take it there (along with the patch to SELinux to call out to commoncaps) if there is no way to proceed with the VFS core towards solving this?

Isn't this just bypassing the VFS objections?

- James

Signature · 2 lines
James Morris
<[email protected]>
author
#15 lore raw
Quoted 9 lines · James Morris
Quoted 5 lines · Kees Cook

Expecting the push-back from VFS, I wrote this patch against commoncaps.

James, would you take it there (along with the patch to SELinux to call out to commoncaps) if there is no way to proceed with the VFS core towards solving this?

Isn't this just bypassing the VFS objections?

Well, that's what I'm trying to understand. It sounds like there is some general agreement that the issue needs to be solved, but some folks do not want it in the core VFS. As in, the objections aren't with how symlink behavior is changed, just that the changes would be in the fs/ directory.

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

Ironically, doing this in commoncaps is the patch I sent. :P

To me, the VFS objections really seem like a obfuscated version of "this should not be in the kernel in any way", which I think does not admit to the problem existing in the first place. Either there is a problem or there isn't. If there is a problem, it should be fixed. I think there is a problem with the kernel semantics of symlinks. Therefore, it should be fixed in the kernel.

I personally do not care if it's in fs/ or security/, I'm just seeking the most efficient and complete solution.

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#16 lore raw
Quoted 4 lines · Kees Cook

Well, that's what I'm trying to understand. It sounds like there is some general agreement that the issue needs to be solved, but some folks do not want it in the core VFS. As in, the objections aren't with how symlink behavior is changed, just that the changes would be in the fs/ directory.

No, it's not. It's not a change we can make for the default that everyone uses. If you're keen to mess up installations you control (aka ubuntu valuedadd viper) push it into a special LSM or rather a non-standard rule for it. It really doesn't matter if it's in fs/ or security/ but it's simplify not going to happen by default.

Quoted 3 lines

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking some really bad stuff.

#17 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Kees Cook

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking some really bad stuff.

Sounds to me like we should probably follow the same path as mmap_min_addr. We should add these hooks right in the VFS where they belong (much like mmap_min_addr hooks into the vm) and control them 2 ways.

1) a Kconfig so distros can choose to turn it on or off by default
2) a /proc interface so root can turn it off

Nothing about that precludes additional similar checks inside an LSM (like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled. So maybe we want to follow up with the core VFS check with new checks in SELinux (and maybe apparmour). This allows the user to disable the general check and still be provided with some modicum of protection. You might ask why not ONLY do the check in SELinux and drop the generic check, but we have seen with mmap_min_addr that the SELinux unconfined user can do damn well anything it wants to, so having a non-LSM version of appropriate security checks is highly regarded.

(see eparis.livejournal.com for a long discussion on the LSM creating a weaker machine in some cases when there are no non-LSM checks)

-Eric

author
#18 lore raw
Quoted 18 lines · Eric Paris
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Kees Cook

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking some really bad stuff.

Sounds to me like we should probably follow the same path as mmap_min_addr. We should add these hooks right in the VFS where they belong (much like mmap_min_addr hooks into the vm) and control them 2 ways.

1) a Kconfig so distros can choose to turn it on or off by default
2) a /proc interface so root can turn it off

Quoted 9 lines

Nothing about that precludes additional similar checks inside an LSM (like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled. So maybe we want to follow up with the core VFS check with new checks in SELinux (and maybe apparmour). This allows the user to disable the general check and still be provided with some modicum of protection. You might ask why not ONLY do the check in SELinux and drop the generic check, but we have seen with mmap_min_addr that the SELinux unconfined user can do damn well anything it wants to, so having a non-LSM version of appropriate security checks is highly regarded.

Would a CONFIG for this be overkill? mmap_min_addr is a little different in that there was desire to control a bottom limit on it, etc. Given this is either "on" or "off", I think just a sysctl is needed?

I will send a v3 patch that fixes the sysctl name and default, so that it is up to the distro and end user how to configure their symlink semantics.

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#19 lore raw
Quoted 29 lines · Kees Cook
Quoted 18 lines · Eric Paris
Quoted 8 lines · Christoph Hellwig
Quoted 3 lines · Kees Cook

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking some really bad stuff.

Sounds to me like we should probably follow the same path as mmap_min_addr. We should add these hooks right in the VFS where they belong (much like mmap_min_addr hooks into the vm) and control them 2 ways.

1) a Kconfig so distros can choose to turn it on or off by default
2) a /proc interface so root can turn it off

Quoted 9 lines

Nothing about that precludes additional similar checks inside an LSM (like CONFIG_LSM_MMAP_MIN_ADDR) which can be more finely controlled. So maybe we want to follow up with the core VFS check with new checks in SELinux (and maybe apparmour). This allows the user to disable the general check and still be provided with some modicum of protection. You might ask why not ONLY do the check in SELinux and drop the generic check, but we have seen with mmap_min_addr that the SELinux unconfined user can do damn well anything it wants to, so having a non-LSM version of appropriate security checks is highly regarded.

Quoted 3 lines

Would a CONFIG for this be overkill? mmap_min_addr is a little different in that there was desire to control a bottom limit on it, etc. Given this is either "on" or "off", I think just a sysctl is needed?

Seems like one of Alan's main arguments is that you should not turn it on 'by default.' I assume most distros will want it on by default. Alan made the same argument against mmap_min_addr (known to break dos emu) but I think most major distros have it on by default these days even if it does break those weird obscure use cases. I guess distros can do it through sysctl but Fedora, at least, likes to keep those default if possible, which is why I suggested the CONFIG. In any case, putting this right square in the VFS where it happens makes the most sense to me.

I'd also like to point out that I don't buy the argument that per user /tmp/ is a 'better' solution for the general case. Any application that would be broken by this change will also be broken by per user /tmp. Now, if we used filesystem namespaces regularly for years and users, administrators, and developers dealt with them often I agree that would probably be the preferred solution. It would solve this issue, but in introduces a whole host of other problems that are even more obvious and even likely to bite people.

I probably would move the security hook down into __do_follow_link and put this check down there as well, but I think you still have a problem with d_parent. I don't see what keeps d_parent from being freed while you are using it....

-Eric

#20 lore raw
Quoted 10 lines · Eric Paris

Seems like one of Alan's main arguments is that you should not turn it on 'by default.' I assume most distros will want it on by default. Alan made the same argument against mmap_min_addr (known to break dos emu) but I think most major distros have it on by default these days even if it does break those weird obscure use cases. I guess distros can do it through sysctl but Fedora, at least, likes to keep those default if possible, which is why I suggested the CONFIG. In any case, putting this right square in the VFS where it happens makes the most sense to me.

If the use cases where people would really want to support following a third-party symlink located in a world-writable sticky-bitted directory are real (I'm not really convinced they really are real), then it might make more sense to do it as a sysctl-controlled variable with distro's chooinsg to enable it by default in their shipped /etc/sysctl.conf file. That way their users/customers will be able to more easily disable the security feature if it really matters.

That being said, I'm not at all convinced there are any legtimiate use cases that would be impacted by this change, and as I've pointed out earlier, I don't think the "POSIX doesn't allow this change" argument holds water, since POSIX originally didn't even allow for the semantics for the sticky-bit on directories. The real argument is going to come from traditionalists, who will argue, "that isn't the traditional behaviour!"

How strongly you buy this as an argument is going to be religious argument, since it's doubtful that it people can come up with real use cases that will actually break with that sort of change --- especially given that the vast majority of Linux systems are single-user systems, and the main concern will be a privilege escalation attacks.

Quoted 8 lines

I'd also like to point out that I don't buy the argument that per user /tmp/ is a 'better' solution for the general case. Any application that would be broken by this change will also be broken by per user /tmp. Now, if we used filesystem namespaces regularly for years and users, administrators, and developers dealt with them often I agree that would probably be the preferred solution. It would solve this issue, but in introduces a whole host of other problems that are even more obvious and even likely to bite people.

Per-user /tmp solves other problems as well, though --- especially for people who care a whole lot about mandatory access control scenarios, for example.

I do have a slight preference against per-user /tmp mostly because it gets confusing for administrators, and because it could be used by rootkits to hide themselves in ways that would be hard for most system administrators to find.

- Ted

author
#21 lore raw
In reply to Christoph Hellwig
Quoted 11 lines · Christoph Hellwig
Quoted 4 lines · Kees Cook

Well, that's what I'm trying to understand. It sounds like there is some general agreement that the issue needs to be solved, but some folks do not want it in the core VFS. As in, the objections aren't with how symlink behavior is changed, just that the changes would be in the fs/ directory.

No, it's not. It's not a change we can make for the default that everyone uses. If you're keen to mess up installations you control (aka ubuntu valuedadd viper) push it into a special LSM or rather a non-standard rule for it. It really doesn't matter if it's in fs/ or security/ but it's simplify not going to happen by default.

Okay, thanks; that clarifies some of my confusion. It sounds like there are some people that genuinely believe that the symlink-following logic should not change. I would pose, then, a question of "what are legitimate and safe situations that require following cross-user symlinks in a sticky world-writable directory?" And if the answers to that aren't very convincing, then I think it's reasonable to include at least an option to change the behavior.

Quoted 6 lines
Quoted 3 lines

My rationale is that if it's in commoncaps, it's effective for everyone, so it might as well be in core VFS. If the VFS objections really do boil down to "not in fs/" then I'm curious if doing this in commoncaps is acceptable.

If you think the objection is about having things in fs/ you're smoking some really bad stuff.

Right, that was my point exactly. It didn't make sense to object to it being in fs/. The objection was to having it in the kernel at all. So now I can focus my efforts on convincing people about the value of making this a setting in the kernel, like turning on or off TCP syn-flood protection. Some people may demand it, some people may hate it, but the choice it up to the end user.

-Kees

Signature · 2 lines
Kees Cook
Ubuntu Security Team
#22 lore raw
In reply to Kees Cook
Quoted 3 lines · Kees Cook

- Violates POSIX.
- POSIX didn't consider this situation and it's not useful to follow a broken specification at the cost of security.

POSIX allows implementations to fail requested operations with a permission denied for reasons beyond that which is specified by the POSIX implementation. Indeed, POSIX doesn't specify the sticky bit at all, so implementations that implemented the sticky bit were able to pass POSIX precisely because it's OK to provide stricter semantics than that which is specified by POSIX. (The sticky bit is specified in the XSI exstension to the Single Unix Specification, but the same principle applies; it's OK for us have additional situations where we return EACCESS beyond that which was contemplated by POSIX/SUS; this alone wouldn't cause us to 'violate POSIX'.)

So for people who to argue against this (which I believe to be a useful restriction, and not one that necessarily has to be done in a LSM), it's not sufficient to say that it is a POSIX violation, because it isn't. The sticky bit itself wasn't originally considered by POSIX, and many systems which implemented the sticky bit had no problems becoming ceritifed as POSIX compliant.

- Ted