fs: symlink restrictions on sticky directories

kernel/sysctl.c static struct ctl_table fs_table[] = { · line 1506

Quoted 12 lines · Kees Cook

kernel/sysctl.c +8 -0

@@ -1495,6 +1496,15 @@ static struct ctl_table fs_table[] = {
  #endif
  #endif
       {
+             .procname       = "protected_sticky_symlinks",
+             .data           = &protected_sticky_symlinks,
+             .maxlen         = sizeof(int),
+             .mode           = 0644,
+             .proc_handler   = proc_dointvec_minmax,
+             .extra1         = &zero,
+             .extra2         = &one,
+     },

Small detail:

Might make sense to change the .mode to 0600, to make it harder for unprivileged attack code to guess whether this protection (and the resulting audit warning to the administrator) is enabled on a system or not.

Quoted 17 lines · Ingo Molnar

kernel/sysctl.c static struct ctl_table fs_table[] = { · line 1506

Quoted 12 lines · Kees Cook

kernel/sysctl.c +8 -0

@@ -1495,6 +1496,15 @@ static struct ctl_table fs_table[] = {
  #endif
  #endif
       {
+             .procname       = "protected_sticky_symlinks",
+             .data           = &protected_sticky_symlinks,
+             .maxlen         = sizeof(int),
+             .mode           = 0644,
+             .proc_handler   = proc_dointvec_minmax,
+             .extra1         = &zero,
+             .extra2         = &one,
+     },

Small detail:

Quoted 4 lines

Might make sense to change the .mode to 0600, to make it harder for unprivileged attack code to guess whether this protection (and the resulting audit warning to the administrator) is enabled on a system or not.

Sure, I have no problem with that. In addition to this change, what's the best next step for this patch?

Quoted 23 lines · Kees Cook

Quoted 21 lines · Ingo Molnar

kernel/sysctl.c static struct ctl_table fs_table[] = { · line 1506

Quoted 12 lines · Kees Cook

kernel/sysctl.c +8 -0

@@ -1495,6 +1496,15 @@ static struct ctl_table fs_table[] = {
 #endif
 #endif
      {
+             .procname       = "protected_sticky_symlinks",
+             .data           = &protected_sticky_symlinks,
+             .maxlen         = sizeof(int),
+             .mode           = 0644,
+             .proc_handler   = proc_dointvec_minmax,
+             .extra1         = &zero,
+             .extra2         = &one,
+     },

Small detail:

Might make sense to change the .mode to 0600, to make it harder for unprivileged attack code to guess whether this protection (and the resulting audit warning to the administrator) is enabled on a system or not.

Quoted 2 lines

Sure, I have no problem with that. In addition to this change, what's the best next step for this patch?

Al and Linus's call I guess. Maybe ask Andrew whether he'd put it into -mm?

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.

...

fs/Kconfig +5 -0

+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink following in sticky world-writable directories"
+	default y
+	help
+	  A long-standing class of security issues is the symlink-based

fs/namei.c +8 -0

+/**
+ * may_follow_link - Check symlink following for unsafe situations
+ * @dentry: The inode/dentry of the symlink
+ * @nameidata: The path data of the symlink
+ *
+ * In the case of the protected_sticky_symlinks sysctl being enabled,
+ * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
+ * in a sticky world-writable directory. This is to protect privileged

fs/namei.c +22 -0

+{
+	int error = 0;
+	const struct inode *parent;
+	const struct inode *inode;
+	const struct cred *cred;
+
+	if (!protected_sticky_symlinks)
+		return 0;
+
+	/* Allowed if owner and follower match. */
+	cred = current_cred();
+	inode = dentry->d_inode;
+	if (cred->fsuid == inode->i_uid)
+		return 0;
+
+	/* Check parent directory mode and owner. */
+	spin_lock(&dentry->d_lock);
+	parent = dentry->d_parent->d_inode;
+	if ((parent->i_mode & (S_ISVTX|S_IWOTH)) == (S_ISVTX|S_IWOTH) &&
+	    parent->i_uid != inode->i_uid) {
+		error = -EACCES;
+	}

fs/namei.c +13 -3

+				 inode->i_ino);
+		audit_log_end(ab);
+	}
+#endif
+	return error;
+}
+
 static  always_inline int
-follow_link(struct path *link, struct nameidata *nd, void **p)
+follow_link(struct path *link, struct nameidata *nd, void **p, bool sensitive)
 {
-	int error;
+	int error = 0;
 	struct dentry *dentry = link->dentry;
 
 	BUG_ON(nd->flags & LOOKUP_RCU);

@@ -646,7 +716,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)
 	touch_atime(link->mnt, dentry);
 	nd_set_link(nd, NULL);
 
-	error = security_inode_follow_link(link->dentry, nd);
+	if (sensitive)
+		error = may_follow_link(link->dentry, nd);
+	if (!error)
+		error = security_inode_follow_link(link->dentry, nd);
 	if (error) {
 		*p = ERR_PTR(error); /* no ->put_link(), please */
 		path_put(&nd->path);

fs/namei.c +2 -1

 		if (!res)
 			res = walk_component(nd, path, &nd->last,
 					     nd->last_type, LOOKUP_FOLLOW);

@@ -1612,7 +1685,8 @@ static int path_lookupat(int dfd, const char *name,
 			void *cookie;
 			struct path link = path;
 			nd->flags |= LOOKUP_PARENT;
-			err = follow_link(&link, nd, &cookie);
+
+			err = follow_link(&link, nd, &cookie, 1);
 			if (!err)
 				err = lookup_last(nd, &path);
 			put_link(nd, &link, cookie);
 
...

Quoted 5 lines

fs/Kconfig +5 -0

+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink following in sticky world-writable directories"
+	default y
+	help
+	  A long-standing class of security issues is the symlink-based

fs/Kconfig source "fs/nls/Kconfig" · line 296

Quoted 10 lines

fs/Kconfig +10 -0

+	  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 malicious symlink belonging to another user).
+
+	  Enabling this solves the problem by permitting symlinks to be
+	  followed only when outside a sticky world-writable directory,
+	  or when the uid of the symlink and follower match, or when
+	  the directory and symlink owners match.

This is all quite misleading. One would expect that CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on or off permanently. ie, it controls whether the code is generated into vmlinux in the usual fashion. But it's not that at all - the user gets the feature whether or not he wants it, and this variable only sets the initial value.

Why are we forcing the user to have the feature if he doesn't want it, btw?

And we appear to be enabling the feature if CONFIG_PROC_FS=n, which might not be terribly useful?

Quoted 23 lines · Andrew Morton

Quoted 5 lines

fs/Kconfig +5 -0

+config PROTECTED_STICKY_SYMLINKS
+	bool "Protect symlink following in sticky world-writable directories"
+	default y
+	help
+	  A long-standing class of security issues is the symlink-based

fs/Kconfig source "fs/nls/Kconfig" · line 296

Quoted 10 lines

fs/Kconfig +10 -0

+	  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 malicious symlink belonging to another user).
+
+	  Enabling this solves the problem by permitting symlinks to be
+	  followed only when outside a sticky world-writable directory,
+	  or when the uid of the symlink and follower match, or when
+	  the directory and symlink owners match.

This is all quite misleading. One would expect that CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on or off permanently. ie, it controls whether the code is generated into vmlinux in the usual fashion. But it's not that at all - the user gets the feature whether or not he wants it, and this variable only sets the initial value.

Quoted 2 lines

Why are we forcing the user to have the feature if he doesn't want it, btw?

Basing on the (not yet fully confirmed) assertion that no apps are broken by this change but exploits, I'd argue that this is actually the sane and correct semantics for symlinks - i.e. we want this to be the default Linux behavior - not just a 'feature'.

That way the configuration knobs are compat settings in essence
- in case some app cares.

If people disagree and want it default off and as a separate feature then it has to be modularized out some more. There's notable silence from VFS folks on all this so Kees made an educated guess. It might be wrong.

Quoted 2 lines

And we appear to be enabling the feature if CONFIG_PROC_FS=n, which might not be terribly useful?

It can still be useful if it's default on - just cannot be turned off via /proc/sys/, right?

The combination that is not so useful is when it's off and there's !PROC_FS. If it's a compat feature then i wouldnt bother about it. If it's a separated out modular feature in a separate .c file then it can all be properly shaped via Kconfig dependencies.

Maybe true for a general purpose computer, but someone who is making a single-purpose device such as a digital TV or a wifi router won't want it.

Quoted 3 lines · Andrew Morton

Maybe true for a general purpose computer, but someone who is making a single-purpose device such as a digital TV or a wifi router won't want it.

That's the case for 99% of the features and semantics we have:
by definition a single-purpose device uses only a small sub-set of an infinite purpose OS, right?

Still we only modularize semantics out if they easily fit into some existing plug-in/module concept, if the feature is arguably oddball that a sizable portion of people want to disable, or if it makes notable sense for size reasons. To me it looked distinctly silly to complicate things for such a small piece of code.

I doubt Kees would mind modularizing it, but it would be nice to get VFS maintainer feedback in the:

   { 'you are crazy, over my dead body' ... 'cool, merge it' }

continuous spectrum of possible answers.

Quoted 10 lines · On Fri, 6 Jan 2012 11:05:20 +0100 Ingo Molnar <[email protected]> wrote:

Quoted 3 lines · Andrew Morton

Maybe true for a general purpose computer, but someone who is making a single-purpose device such as a digital TV or a wifi router won't want it.

That's the case for 99% of the features and semantics we
have: by definition a single-purpose device uses only a small sub-set of an infinite purpose OS, right?

Quoted 6 lines

Still we only modularize semantics out if they easily fit into some existing plug-in/module concept, if the feature is arguably oddball that a sizable portion of people want to disable, or if it makes notable sense for size reasons. To me it looked distinctly silly to complicate things for such a small piece of code.

We're talking tens or hundreds of millions of machines for which the patch is a straightforward speed and space regression. Fixing this needs just a little Kconfig twiddling and a #else clause. We may as well do it.

Quoted 5 lines

I doubt Kees would mind modularizing it, but it would be nice to get VFS maintainer feedback in the:

   { 'you are crazy, over my dead body' ... 'cool, merge it' }

Quoted 1 line

continuous spectrum of possible answers.

Well yes. We'll get there.

Alas, I've become rather slack in my maintainer patchbombing in the past year or two. It's just boring and depressing to spray patches at maintainers and have 90% or more of them simply ignored. [...]

[...] I'm sitting on 45 such patches at present so I suppose I should get off my tail and do a respray.

Quoted 33 lines · On Fri, 6 Jan 2012 10:43:40 +0100 Ingo Molnar <[email protected]> wrote:

Quoted 23 lines · Andrew Morton

Quoted 5 lines

(quoted diff) +5 -0

+config PROTECTED_STICKY_SYMLINKS
+ bool "Protect symlink following in sticky world-writable directories"
+ default y
+ help
+   A long-standing class of security issues is the symlink-based

Review

Quoted 10 lines

(quoted diff) +10 -0

+   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 malicious symlink belonging to another user).
+
+   Enabling this solves the problem by permitting symlinks to be
+   followed only when outside a sticky world-writable directory,
+   or when the uid of the symlink and follower match, or when
+   the directory and symlink owners match.

This is all quite misleading.  One would expect that CONFIG_PROTECTED_STICKY_SYMLINKS turns the entire feature on or off permanently.  ie, it controls whether the code is generated into vmlinux in the usual fashion.  But it's not that at all - the user gets the feature whether or not he wants it, and this variable only sets the initial value.

Quoted 2 lines

Why are we forcing the user to have the feature if he doesn't want it, btw?

Basing on the (not yet fully confirmed) assertion that no apps are broken by this change but exploits, I'd argue that this is actually the sane and correct semantics for symlinks - i.e. we want this to be the default Linux behavior - not just a 'feature'.

Quoted 7 lines

That way the configuration knobs are compat settings in essence
- in case some app cares.

If people disagree and want it default off and as a separate feature then it has to be modularized out some more. There's notable silence from VFS folks on all this so Kees made an educated guess. It might be wrong.

Maybe true for a general purpose computer, but someone who is making a single-purpose device such as a digital TV or a wifi router won't want it.
[...]
I'd have thought the way to configure this feature would be to have CONFIG_PROTECTED_STICKY_SYMLINKS to control the code generation then a 0 or 1 CONFIG_PROTECTED_STICKY_SYMLINKS_ENABLED to control the initial setting.

Quoted 24 lines · On Fri, 6 Jan 2012 08:36:35 +0100 Ingo Molnar <[email protected]> wrote:

Quoted 23 lines · Kees Cook

Quoted 21 lines · Ingo Molnar

kernel/sysctl.c static struct ctl_table fs_table[] = { · line 1506

Quoted 12 lines · Kees Cook

kernel/sysctl.c +8 -0

@@ -1495,6 +1496,15 @@ static struct ctl_table fs_table[] = {
  #endif
  #endif
       {
+             .procname       = "protected_sticky_symlinks",
+             .data           = &protected_sticky_symlinks,
+             .maxlen         = sizeof(int),
+             .mode           = 0644,
+             .proc_handler   = proc_dointvec_minmax,
+             .extra1         = &zero,
+             .extra2         = &one,
+     },

Small detail:

Might make sense to change the .mode to 0600, to make it harder for unprivileged attack code to guess whether this protection (and the resulting audit warning to the administrator) is enabled on a system or not.

Quoted 5 lines

Quoted 2 lines

Sure, I have no problem with that. In addition to this change, what's the best next step for this patch?

Al and Linus's call I guess. Maybe ask Andrew whether he'd put it into -mm?

Sure, I can babysit it for Viro and get it a bit of testing meanwhile.

However, you've now made me go and look at it...

Quoted 8 lines · On Wed, 4 Jan 2012 12:18:00 -0800 Kees Cook <[email protected]> wrote:

fs/namei.c +0 -0

index 5008f01..b826d2e 100644
@@ -624,10 +624,80 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
       path_put(link);
  }
 

fs/namei.c static inline void put_link(struct nameidata *nd, struct path *link, void *cooki · line 627

Quoted 1 line

fs/namei.c +1 -0

+int protected_sticky_symlinks  read_mostly =

A nice convention is to prefix these globals with sysctl_.

       grep ".data.*&sysctl_" kernel/sysctl.c

has examples.

Quoted 14 lines

fs/namei.c +14 -0

+/**
+ * may_follow_link - Check symlink following for unsafe situations
+ * @dentry: The inode/dentry of the symlink
+ * @nameidata: The path data of the symlink
+ *
+ * In the case of the protected_sticky_symlinks sysctl being enabled,
+ * CAP_DAC_OVERRIDE needs to be specifically ignored if the symlink is
+ * in a sticky world-writable directory. This is to protect privileged
+ * processes from failing races against path names that may change out
+ * from under them by way of other users creating malicious symlinks.
+ * It will permit symlinks to be followed only 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.
+ *

fs/namei.c static inline void put_link(struct nameidata *nd, struct path *link, void *cooki · line 651

Quoted 4 lines

fs/namei.c +4 -0

+ * Returns 0 if following the symlink is allowed, -ve on error.
+ */
+static inline int
+may_follow_link(struct dentry *dentry, struct nameidata *nameidata)

Setting aside the issue that gcc will cheerily ignore the inline directive, is it prudent to inline this?  Doing so will pork up the follow_link() cache footprint.  Perhaps it should actually be noinline?

fs/namei.c static inline void put_link(struct nameidata *nd, struct path *link, void *cooki · line 683

Quoted 4 lines

fs/namei.c +4 -0

+             ab = audit_log_start(current->audit_context,
+                                  GFP_KERNEL, AUDIT_AVC);
+             audit_log_format(ab, "op=follow_link action=denied");
+             audit_log_format(ab, " pid=%d comm=", current->pid);

The same pid can exist in multiple namespaces.  Now whatcha gonna do?

fs/namei.c static inline void put_link(struct nameidata *nd, struct path *link, void *cooki · line 689

Quoted 6 lines

fs/namei.c +6 -0

+             audit_log_untrustedstring(ab, current->comm);
+             audit_log_d_path(ab, "path=", &nameidata->path);
+             audit_log_format(ab, " name=");
+             audit_log_untrustedstring(ab, dentry->d_name.name);
+             audit_log_format(ab, " dev=%s ino=%lu",
+                              inode->i_sb->s_id,

Can s_id contain characters which audit_log_untrustedstring() would have escaped?  I suspect so.

Quoted 32 lines

fs/namei.c +7 -0

+                              inode->i_ino);
+             audit_log_end(ab);
+     }
+#endif
+     return error;
+}
+

 static  always_inline int

fs/namei.c +1 -1

-follow_link(struct path *link, struct nameidata *nd, void **p)
+follow_link(struct path *link, struct nameidata *nd, void **p, bool sensitive)

 {
-     int error;
+     int error = 0;
      struct dentry *dentry = link->dentry;

      BUG_ON(nd->flags & LOOKUP_RCU);

fs/namei.c +4 -1

@@ -646,7 +716,10 @@ follow_link(struct path *link, struct nameidata *nd, void **p)
       touch_atime(link->mnt, dentry);
       nd_set_link(nd, NULL);
 
-     error = security_inode_follow_link(link->dentry, nd);
+     if (sensitive)
+             error = may_follow_link(link->dentry, nd);
+     if (!error)
+             error = security_inode_follow_link(link->dentry, nd);
       if (error) {
               *p = ERR_PTR(error); /* no ->put_link(), please */
               path_put(&nd->path);

@@ -1339,7 +1412,7 @@ static inline int nested_symlink(struct path *path, struct nameidata *nd)
               struct path link = *path;
               void *cookie;
 

fs/namei.c static inline int nested_symlink(struct path *path, struct nameidata *nd) · line 1415

Quoted 2 lines

fs/namei.c +1 -1

-             res = follow_link(&link, nd, &cookie);
+             res = follow_link(&link, nd, &cookie, 0);

follow_link() takes a bool, so this 0 should anally be "false". Multiple instances of this.

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.

Quoted 12 lines · Kees Cook

fs/Kconfig +0 -0

index 5f4c45d..26ede24 100644
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"

 source "fs/dlm/Kconfig"

 endmenu

fs/Kconfig +4 -0

+
+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Why do we need a config option for this?  What's wrong with just using the sysctl?

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Quoted 15 lines · Nick Bowler

Quoted 12 lines · Kees Cook

fs/Kconfig +0 -0

index 5f4c45d..26ede24 100644
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"

 source "fs/dlm/Kconfig"

 endmenu

fs/Kconfig +4 -0

+
+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Quoted 2 lines

Why do we need a config option for this?  What's wrong with just using the sysctl?

This way the sysctl can configured directly without needing to have a distro add a new item to sysctl.conf.

Quoted 2 lines

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

Quoted 16 lines · Kees Cook

Quoted 15 lines · Nick Bowler

Quoted 12 lines · Kees Cook

fs/Kconfig +0 -0

index 5f4c45d..26ede24 100644
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"

 source "fs/dlm/Kconfig"

 endmenu

fs/Kconfig +4 -0

+
+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Quoted 5 lines

Quoted 2 lines

Why do we need a config option for this?  What's wrong with just using the sysctl?

This way the sysctl can configured directly without needing to have a distro add a new item to sysctl.conf.

This seems totally pointless to me.  There are tons of sysctls that don't have Kconfig options: what makes this one special?

Quoted 4 lines

Quoted 2 lines

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

But this is a brand new feature that changes longstanding behaviour of various syscalls.  Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.

Quoted 23 lines · Nick Bowler

Quoted 21 lines · Kees Cook

Quoted 15 lines · Nick Bowler

Quoted 12 lines · Kees Cook

fs/Kconfig +0 -0

index 5f4c45d..26ede24 100644
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"

 source "fs/dlm/Kconfig"

 endmenu

fs/Kconfig +4 -0

+
+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Quoted 2 lines

Why do we need a config option for this?  What's wrong with just using the sysctl?

This way the sysctl can configured directly without needing to have a distro add a new item to sysctl.conf.

Quoted 2 lines

This seems totally pointless to me.  There are tons of sysctls that don't have Kconfig options: what makes this one special?

Most are system tuning; this is directly related to vulnerability mitigation. Besides, I like having CONFIGs for sysctls because then I can build my kernel the way I want it without having to worry about tweaking my userspace sysctl.conf file, or run newer kernels on older userspaces, etc etc.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

Quoted 4 lines

But this is a brand new feature that changes longstanding behaviour of various syscalls.  Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.

I couldn't disagree more. There has been zero evidence of this change causing anything but regressions in _attacks_.

If anything, I think there should be no CONFIG and no sysctl, and it should be entirely non-optional. But since this patch needs consensus, I have provided knobs to control it. This is the way of security features. For example, years back I added a knob for /proc/$pid/maps protection being optional (and defaulted it to insecure because of people's fear of regression), and eventually it changed to secure-by-default, and then the knob went away completely because it didn't actually cause problems.

Quoted 24 lines · Kees Cook

Quoted 23 lines · Nick Bowler

Quoted 21 lines · Kees Cook

Quoted 15 lines · Nick Bowler

Quoted 12 lines · Kees Cook

fs/Kconfig +0 -0

index 5f4c45d..26ede24 100644
@@ -278,3 +278,19 @@ source "fs/nls/Kconfig"

 source "fs/dlm/Kconfig"

 endmenu

fs/Kconfig +4 -0

+
+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Quoted 2 lines

Why do we need a config option for this?  What's wrong with just using the sysctl?

This way the sysctl can configured directly without needing to have a distro add a new item to sysctl.conf.

Quoted 8 lines

Quoted 2 lines

This seems totally pointless to me.  There are tons of sysctls that don't have Kconfig options: what makes this one special?

Most are system tuning; this is directly related to vulnerability mitigation. Besides, I like having CONFIGs for sysctls because then I can build my kernel the way I want it without having to worry about tweaking my userspace sysctl.conf file, or run newer kernels on older userspaces, etc etc.

I agree that having kconfig knobs for sysctls may be convenient for some users.  But every kconfig option we add requires the user to make a decision before building their kernel.  In this case, this decision is a waste of time because the option doesn't really affect the kernel in a meaningful way: either choice can be easily changed from userspace after booting.  A similar argument could be applied to almost any sysctl, and we could add hundreds of new Kconfig options to control their default values.  The result would be untenable.

Perhaps what we need instead is a way to set arbitrary sysctls from the kernel command line.  This could easily be done by an initramfs, and not require any changes to the kernel at all.

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

But this is a brand new feature that changes longstanding behaviour of various syscalls.  Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.

Quoted 2 lines

I couldn't disagree more. There has been zero evidence of this change causing anything but regressions in _attacks_.

We have absolutely no idea what applications people are running that will be affected by this change.  Of course there's no evidence of breakage, because affected users (if any) have not had a single chance to try this new feature out: it's not in the kernel yet.

Quoted 8 lines

If anything, I think there should be no CONFIG and no sysctl, and it should be entirely non-optional. But since this patch needs consensus, I have provided knobs to control it. This is the way of security features. For example, years back I added a knob for /proc/$pid/maps protection being optional (and defaulted it to insecure because of people's fear of regression), and eventually it changed to secure-by-default, and then the knob went away completely because it didn't actually cause problems.

The process you describe above for /proc/$pid/maps is the right way to change kernel behaviour while mitigating the risk of regressions.  With this patch, you've skipped all those important steps!

Quoted 4 lines · Nick Bowler

But this is a brand new feature that changes longstanding behaviour of various syscalls. Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.

I couldn't disagree more. There has been zero evidence of this change causing anything but regressions in _attacks_. :P If anything, I think there should be no CONFIG and no sysctl, and it should be entirely non-optional. But since this patch needs consensus, I have provided knobs to control it.

Quoted 7 lines · Kees Cook

Quoted 4 lines · Nick Bowler

But this is a brand new feature that changes longstanding behaviour of various syscalls. Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.

Quoted 6 lines

I couldn't disagree more. There has been zero evidence of this change causing anything but regressions in _attacks_. :P If anything, I think there should be no CONFIG and no sysctl, and it should be entirely non-optional. But since this patch needs consensus, I have provided knobs to control it.

I agree with you, Kees.

The behaviour introduced by this patch should produce so few issues, that the new behaviour should probably be on by default.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

+config PROTECTED_STICKY_SYMLINKS
+     bool "Protect symlink following in sticky world-writable directories"
+     default y

[...]

Quoted 5 lines

Quoted 2 lines

Why do we need a config option for this?  What's wrong with just using the sysctl?

This way the sysctl can configured directly without needing to have a distro add a new item to sysctl.conf.

This seems totally pointless to me. [...]

[...] There are tons of sysctls that don't have Kconfig
options: what makes this one special?

Quoted 4 lines

Quoted 2 lines

Why have you made this option "default y", when enabling it clearly makes user-visible changes to kernel behaviour?

Ingo specifically asked me to make it "default y".

But this is a brand new feature that changes longstanding behaviour of various syscalls. Making it default to enabled is rather mean to users (since it will tend to get enabled by "oldconfig") and seems almost guaranteed to cause regressions.