Skip to contentSkip to conversation

VFS: add config options to enable link restrictions

PATCH In review Kees Cook 4 participants 9 messages Oct 26, 2012 VFS

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 VFS: add config options to enable link restrictions

There are situations where devices running without initrds may need very early protection from link vulnerabilities, so make these sysctls configurable at build time, since 561ec64ae67e ("VFS: don't do protected {sym,hard}links by default") has disabled the protections by default.

---
 fs/Kconfig |   39 +++++++++++++++++++++++++++++++++++++++
 fs/namei.c |    6 ++++--
 2 files changed, 43 insertions(+), 2 deletions(-)
fs/Kconfig +39 -0
index f95ae3a..46ae2dc 100644
@@ -276,4 +276,43 @@ endif # NETWORK_FILESYSTEMS
 source "fs/nls/Kconfig"
 source "fs/dlm/Kconfig"
 
+config PROTECTED_SYMLINKS
+	bool "Protect symlink following in sticky world-writable dirs"
+	default n
+	help
+	  Solve the class of ToCToU symlink race vulnerabilities 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.
+
+	  When PROC_SYSCTL is enabled, this setting can also be controlled
+	  via /proc/sys/kernel/protected_symlinks.
+
+	  See Documentation/sysctl/fs.txt for details.
+
+config PROTECTED_SYMLINKS_SYSCTL
+	int
+	default "1" if PROTECTED_SYMLINKS
+	default "0"
+
+config PROTECTED_HARDLINKS
+	bool "Protect hardlink creation to non-accessible files"
+	default n
+	help
+	  Solve the class of ToCToU hardlink race vulnerabilities by
+	  permitting hardlinks to be created only when to a regular file
+	  that is owned by the user, or is readable and writable by the
+	  user. Also blocks users from "pinning" vulnerable setuid/setgid
+	  programs from being upgraded by the administrator.
+
+	  When PROC_SYSCTL is enabled, this setting can also be controlled
+	  via /proc/sys/kernel/protected_hardlinks.
+
+	  See Documentation/sysctl/fs.txt for details.
+
+config PROTECTED_HARDLINKS_SYSCTL
+	int
+	default "1" if PROTECTED_HARDLINKS
+	default "0"
+
 endmenu
fs/namei.c +4 -2
index 937f9d5..21854df 100644
@@ -705,8 +705,10 @@ static inline void put_link(struct nameidata *nd, struct path *link, void *cooki
 	path_put(link);
 }
 
-int sysctl_protected_symlinks __read_mostly = 0;
-int sysctl_protected_hardlinks __read_mostly = 0;
+int sysctl_protected_symlinks __read_mostly =
+	CONFIG_PROTECTED_SYMLINKS_SYSCTL;
+int sysctl_protected_hardlinks __read_mostly =
+	CONFIG_PROTECTED_HARDLINKS_SYSCTL;
 
 /**
  * may_follow_link - Check symlink following for unsafe situations
Signature · 6 lines
1.7.9.5


-- 
Kees Cook
Chrome OS Security
maintainer
#1 lore raw

fs/Kconfig endif # NETWORK_FILESYSTEMS · line 296

Quoted 4 lines · Kees Cook
fs/Kconfig +4 -0
+config PROTECTED_SYMLINKS_SYSCTL
+	int
+	default "1" if PROTECTED_SYMLINKS
+	default "0"

If anything, use IS_ENABLED instead...

Quoted 6 lines
fs/namei.c +4 -2
-int sysctl_protected_symlinks __read_mostly = 0;
-int sysctl_protected_hardlinks __read_mostly = 0;
+int sysctl_protected_symlinks __read_mostly =
+	CONFIG_PROTECTED_SYMLINKS_SYSCTL;
+int sysctl_protected_hardlinks __read_mostly =
+	CONFIG_PROTECTED_HARDLINKS_SYSCTL;
#2 lore raw
In reply to Kees Cook
Quoted 2 lines · Kees Cook

There are situations where devices running without initrds may need very early protection from link vulnerabilities

I really don't see what this argument is all about.

If you don't have initrd, you still have early bootup scripts etc.

If your early bootup has security problems, you have security problems. It has nothing to do with initrd, or with restricted links, or anything else.

I also refuse to add these kinds of micro-management config options and ask any kind of normal person these kinds of "do you want this random crazy feature". A config option would need to be way more sane, not this kind of micro-management.

Linus

author
#3 lore raw
Quoted 11 lines · Linus Torvalds
Quoted 2 lines · Kees Cook

There are situations where devices running without initrds may need very early protection from link vulnerabilities

I really don't see what this argument is all about.

If you don't have initrd, you still have early bootup scripts etc.

If your early bootup has security problems, you have security problems. It has nothing to do with initrd, or with restricted links, or anything else.

I think there's value in being able to enable these protections at build-time so there's no need for a distro to have to ship extra files/lines, spend time setting it, etc. This isn't like other tunables, IMO.

Quoted 4 lines

I also refuse to add these kinds of micro-management config options and ask any kind of normal person these kinds of "do you want this random crazy feature". A config option would need to be way more sane, not this kind of micro-management.

Would a single config item be acceptable? What would be an agreeable way to enable this at build-time?

-Kees

Signature · 2 lines
Kees Cook
Chrome OS Security
#4 lore raw
Quoted 4 lines · Kees Cook

I think there's value in being able to enable these protections at build-time so there's no need for a distro to have to ship extra files/lines, spend time setting it, etc.

The "value in not having to change the distro" is valid value. That said, two lines to rc.local does that too.

But dammit, every single discussion I see, you use some *other* argument for it, like "people don't have initrd" or whatever crazy crap. That's what I was objecting to.

Quoted 2 lines

Would a single config item be acceptable? What would be an agreeable way to enable this at build-time?

I dunno. Maybe a CONFIG_LOCKDOWN thing that sets a few of our other default values as well. Or at least hide the config options from normal users behind some extra security menu, so that people just don't have to care this deeply. Because most people *don't* care.

I also don't see why it would be a build-time config option at all. Why not just expose them as option using module_param_named(), and then you can do it in the kernel command flags (and set it in your grub.conf etc).

Linus

author
#5 lore raw
Quoted 8 lines · Linus Torvalds
Quoted 2 lines · Kees Cook

Would a single config item be acceptable? What would be an agreeable way to enable this at build-time?

I dunno. Maybe a CONFIG_LOCKDOWN thing that sets a few of our other default values as well. Or at least hide the config options from normal users behind some extra security menu, so that people just don't have to care this deeply. Because most people *don't* care.

My take is that while most people don't care, they also should have it enabled. It really is only corner-cases that run into problems. All the other situations are actual security vulnerabilities. So I'd really like to make this as easy as possible to be "on by default" for some extremely early definition of "by default".

Quoted 4 lines

I also don't see why it would be a build-time config option at all. Why not just expose them as option using module_param_named(), and then you can do it in the kernel command flags (and set it in your grub.conf etc).

I'd like it to be the exception to turn it _off_, rather than the exception to turn it on.

I'm happy to start collecting things under CONFIG_LOCKDOWN, but I worry it'd just turn into the opposite of CONFIG_EXPERIMENTAL where if there was some value in it that someone didn't want on, they'd leave it off, etc. Most distros have infrastructure for maintaining their desired configs, so I didn't think the granularity was very bad as I sent it.

Every distro will ship with this enabled (except perhaps Damn Vulnerable Linux), so why make it harder?

-Kees

Signature · 2 lines
Kees Cook
Chrome OS Security
#6 lore raw
Quoted 3 lines · Kees Cook

I'd like it to be the exception to turn it _off_, rather than the exception to turn it on.

Kees, you don't seem to understand.

Breaking applications is unacceptable. End of story. It's broken them. Get over it.

Distributions can turn it on with a security app. They already have it for selinux selection etc. That's where this makes sense. Then sysadmins can set it once (maybe it can default on there, at least for new installs), and be done with it.

Your "IT HAS TO BE DONE AT BOOT TIME, THE SKY IS FALLING, NOTHING ELSE IS ACCEPTABLE!" ranting is a disease. Stop it.

Linus

author
#7 lore raw
Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Kees Cook

I'd like it to be the exception to turn it _off_, rather than the exception to turn it on.

Kees, you don't seem to understand.

Breaking applications is unacceptable. End of story. It's broken them. Get over it.

No, I get that. I've been over it. I can handle it being off by default. I just want there to be a way to make it enabled at build time. I'll explore some other options; it's sensible to tie it to other settings/things that are security-sensitive.

-Kees

Signature · 2 lines
Kees Cook
Chrome OS Security
#8 lore raw
In reply to Kees Cook
Quoted 4 lines · Kees Cook

Every distro will ship with this enabled (except perhaps Damn Vulnerable Linux), so why make it harder?

So please remind me why can't it be on by default in code. And the normal sysctl to turn it off for these who want to experiment with "filesystem corruption".

So the basic premise is that you must not have any filesystem corruption at the parts used by boot up until the init portion that turns "filesystem corruption" on

Quoted 2 lines

-Kees

Cheers
Boaz