[kernel-hardening] [RFC v2 0/2] Restrict dangerous open in sticky directories

[kernel-hardening] [RFC v2 0/2] Restrict dangerous open in sticky directories

[Salvatore Mesoraca]

This patch-set introduces two separate features aimed at restricting
dangerous open in world or group writable sticky directories.
The purpose is to prevent exploitable bugs in user-space programs
that don't access sticky directories in the proper way.
The first patch prevents the O_CREAT open of FIFOs and regular files
in world or group writable sticky directories if they already exists
and are owned by someone else.
The second patch prevents O_CREAT open in world or group writable
sticky when the O_EXCL flag is not set, even if the file doesn't
exist yet.
More details can be found in respective commit messages.

Salvatore Mesoraca (2):
  Protected FIFOs and regular files
  Protected O_CREAT open in sticky directory

 Documentation/sysctl/fs.txt |  66 +++++++++++++++++++++++++
 fs/namei.c                  | 118 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/fs.h          |   3 ++
 kernel/sysctl.c             |  27 ++++++++++
 4 files changed, 211 insertions(+), 3 deletions(-)

-- 
1.9.1

[kernel-hardening] [RFC v2 1/2] Protected FIFOs and regular files

[Salvatore Mesoraca]

Disallows open of FIFOs or regular files not owned by the user in world
writable sticky directories, unless the owner is the same as that of
the directory or the file is opened without the O_CREAT flag.
The purpose is to make data spoofing attacks harder.
This protection can be turned on and off separately for FIFOs and regular
files via sysctl, just like the symlinks/hardlinks protection.
This patch is based on Openwall's "HARDEN_FIFO" feature by Solar
Designer.

This is a brief list of old vulnerabilities that could have been prevented
by this feature, some of them even allow for privilege escalation:
CVE-2000-1134
CVE-2007-3852
CVE-2008-0525
CVE-2009-0416
CVE-2011-4834
CVE-2015-1838
CVE-2015-7442
CVE-2016-7489

This list is not meant to be complete. It's difficult to track down
all vulnerabilities of this kind because they were often reported
without any mention of this particular attack vector.
In fact, before symlinks restrictions, fifos/regular files were not the
favorite vehicle to exploit them.

Suggested-by: Solar Designer <solar@openwall.com>
Suggested-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
 Documentation/sysctl/fs.txt | 36 ++++++++++++++++++++++++++
 fs/namei.c                  | 61 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/fs.h          |  2 ++
 kernel/sysctl.c             | 18 +++++++++++++
 4 files changed, 114 insertions(+), 3 deletions(-)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 35e17f7..655e261 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -34,7 +34,9 @@ Currently, these files are in /proc/sys/fs:
 - overflowgid
 - pipe-user-pages-hard
 - pipe-user-pages-soft
+- protected_fifos
 - protected_hardlinks
+- protected_regular
 - protected_symlinks
 - suid_dumpable
 - super-max
@@ -182,6 +184,24 @@ applied.
 
 ==============================================================
 
+protected_fifos:
+
+The intent of this protection is to avoid unintentional writes to
+an attacker-controlled FIFO, where a program expected to create a regular
+file.
+
+When set to "0", FIFOs writing is unrestricted.
+
+When set to "1" don't allow O_CREAT open on FIFOs that we don't own
+in world writable sticky directories, unless they are owned by the
+owner of the directory.
+
+When set to "2" it also apply to group writable sticky directories.
+
+This protection is based on the restrictions in Openwall.
+
+==============================================================
+
 protected_hardlinks:
 
 A long-standing class of security issues is the hardlink-based
@@ -202,6 +222,22 @@ This protection is based on the restrictions in Openwall and grsecurity.
 
 ==============================================================
 
+protected_regular:
+
+This protection is similar to protected_fifos, but it
+avoids writes to an attacker-controlled regular file, where a program
+expected to create one.
+
+When set to "0", regular files writing is unrestricted.
+
+When set to "1" don't allow O_CREAT open on regular files that we
+don't own in world writable sticky directories, unless they are
+owned by the owner of the directory.
+
+When set to "2" it also apply to group writable sticky directories.
+
+==============================================================
+
 protected_symlinks:
 
 A long-standing class of security issues is the symlink-based
diff --git a/fs/namei.c b/fs/namei.c
index c75ea03..d2b287d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -901,6 +901,8 @@ static inline void put_link(struct nameidata *nd)
 
 int sysctl_protected_symlinks __read_mostly = 0;
 int sysctl_protected_hardlinks __read_mostly = 0;
+int sysctl_protected_fifos __read_mostly;
+int sysctl_protected_regular __read_mostly;
 
 /**
  * may_follow_link - Check symlink following for unsafe situations
@@ -1014,6 +1016,54 @@ static int may_linkat(struct path *link)
 	return -EPERM;
 }
 
+/**
+ * may_create_in_sticky - Check whether an O_CREAT open in a sticky directory
+ *			  should be allowed or not, when the file already
+ *			  existed.
+ * @dir: the stick parent directory
+ * @name: the file name
+ * @inode: the inode of the file to open
+ *
+ * Block an O_CREAT open of a FIFO (or a regular file) when:
+ *   - sysctl_protected_fifos (or sysctl_protected_regular) is enabled
+ *   - the file already exists
+ *   - we are in a sticky directory
+ *   - we don't own the file
+ *   - the owner of the directory doesn't own the file
+ *   - the directory is world writable
+ * If the sysctl_protected_fifos (or sysctl_protected_regular) is set to 2
+ * the directory doesn't have to be world writable: being group writable will
+ * be enough.
+ *
+ * Returns 0 if the open is allowed, -ve on error.
+ */
+static int may_create_in_sticky(struct dentry * const dir,
+				const unsigned char * const name,
+				struct inode * const inode)
+{
+	if ((!sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) ||
+	    (!sysctl_protected_regular && S_ISREG(inode->i_mode)) ||
+	    likely(!(dir->d_inode->i_mode & S_ISVTX)) ||
+	    uid_eq(inode->i_uid, dir->d_inode->i_uid) ||
+	    uid_eq(current_fsuid(), inode->i_uid))
+		return 0;
+
+	if (likely(dir->d_inode->i_mode & 0002) ||
+	    (dir->d_inode->i_mode & 0020 &&
+	     ((sysctl_protected_fifos >= 2 && S_ISFIFO(inode->i_mode)) ||
+	      (sysctl_protected_regular >= 2 && S_ISREG(inode->i_mode))))) {
+		pr_notice_ratelimited("denied writing in '%s' of %d.%d in a sticky directory by UID %d, EUID %d, process %s:%d.\n",
+				      name,
+				      from_kuid(&init_user_ns, inode->i_uid),
+				      from_kgid(&init_user_ns, inode->i_gid),
+				      from_kuid(&init_user_ns, current_uid()),
+				      from_kuid(&init_user_ns, current_euid()),
+				      current->comm, current->pid);
+		return -EACCES;
+	}
+	return 0;
+}
+
 static __always_inline
 const char *get_link(struct nameidata *nd)
 {
@@ -3364,9 +3414,14 @@ static int do_last(struct nameidata *nd,
 	if (error)
 		return error;
 	audit_inode(nd->name, nd->path.dentry, 0);
-	error = -EISDIR;
-	if ((open_flag & O_CREAT) && d_is_dir(nd->path.dentry))
-		goto out;
+	if (open_flag & O_CREAT) {
+		error = -EISDIR;
+		if (d_is_dir(nd->path.dentry))
+			goto out;
+		error = may_create_in_sticky(dir, nd->last.name, inode);
+		if (unlikely(error))
+			goto out;
+	}
 	error = -ENOTDIR;
 	if ((nd->flags & LOOKUP_DIRECTORY) && !d_can_lookup(nd->path.dentry))
 		goto out;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 339e737..14bb497 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -71,6 +71,8 @@
 extern int leases_enable, lease_break_time;
 extern int sysctl_protected_symlinks;
 extern int sysctl_protected_hardlinks;
+extern int sysctl_protected_fifos;
+extern int sysctl_protected_regular;
 
 typedef __kernel_rwf_t rwf_t;
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6648fbb..6b127e2 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1807,6 +1807,24 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.extra2		= &one,
 	},
 	{
+		.procname	= "protected_fifos",
+		.data		= &sysctl_protected_fifos,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+	{
+		.procname	= "protected_regular",
+		.data		= &sysctl_protected_regular,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+	{
 		.procname	= "suid_dumpable",
 		.data		= &suid_dumpable,
 		.maxlen		= sizeof(int),
-- 
1.9.1

[kernel-hardening] [RFC v2 2/2] Protected O_CREAT open in sticky directory

[Salvatore Mesoraca]

Disallows O_CREAT open missing the O_EXCL flag, in world or
group writable directories, even if the file doesn't exist yet.
With few exceptions (e.g. shared lock files based on flock())
if a program tries to open a file with the O_CREAT flag and
without the O_EXCL, it probably has a bug.
This feature allows to detect and potentially block programs that
act this way and can be used to find vulnerabilities (like those
prevented by patch #1) and to do policy enforcement.

Suggested-by: Solar Designer <solar@openwall.com>
Signed-off-by: Salvatore Mesoraca <s.mesoraca16@gmail.com>
---
I wasn't able to come up with a better name than
"protected_sticky_child_create" so I'm open to suggestion
for more decent names.
---
 Documentation/sysctl/fs.txt | 30 ++++++++++++++++++++++++
 fs/namei.c                  | 57 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h          |  1 +
 kernel/sysctl.c             |  9 +++++++
 4 files changed, 97 insertions(+)

diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
index 655e261..647aee5 100644
--- a/Documentation/sysctl/fs.txt
+++ b/Documentation/sysctl/fs.txt
@@ -37,6 +37,7 @@ Currently, these files are in /proc/sys/fs:
 - protected_fifos
 - protected_hardlinks
 - protected_regular
+- protected_sticky_child_create
 - protected_symlinks
 - suid_dumpable
 - super-max
@@ -238,6 +239,35 @@ When set to "2" it also apply to group writable sticky directories.
 
 ==============================================================
 
+protected_sticky_child_create:
+
+An O_CREAT open missing the O_EXCL flag in a sticky directory is,
+often, a bug or a synthom of the fact that the program is not
+using appropriate procedures to access sticky directories.
+This protection allow to detect and possibly block these unsafe
+open invocations, even if the files doen't exist yet.
+Though should be noted that, sometimes, it's OK to open a file
+with O_CREAT and without O_EXCL (e.g. shared lock files based
+on flock()), for this reason values above 2 should be set
+with care.
+
+When set to "0" the protection is disabled.
+
+When set to "1", notify about O_CREAT open missing the O_EXCL flag
+in world writable sticky directories.
+
+When set to "2", notify about O_CREAT open missing the O_EXCL flag
+in group writable sticky directories.
+
+When set to "3", block O_CREAT open missing the O_EXCL flag
+in world writable sticky directories and notify (but don't block)
+in group writable sticky directories.
+
+When set to "4", block O_CREAT open missing the O_EXCL flag
+in world writable and group writable sticky directories.
+
+==============================================================
+
 protected_symlinks:
 
 A long-standing class of security issues is the symlink-based
diff --git a/fs/namei.c b/fs/namei.c
index d2b287d..5c6c0eb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -903,6 +903,7 @@ static inline void put_link(struct nameidata *nd)
 int sysctl_protected_hardlinks __read_mostly = 0;
 int sysctl_protected_fifos __read_mostly;
 int sysctl_protected_regular __read_mostly;
+int sysctl_protected_sticky_child_create __read_mostly;
 
 /**
  * may_follow_link - Check symlink following for unsafe situations
@@ -1064,6 +1065,54 @@ static int may_create_in_sticky(struct dentry * const dir,
 	return 0;
 }
 
+/**
+ * may_create_no_excl - Detect and possibly block unsafe O_CREAT open
+ *			without O_EXCL.
+ * @dir: the stick parent directory
+ * @name: the file name
+ * @inode: the inode of the file to open (can be NULL to skip uid checks)
+ *
+ * When sysctl_protected_sticky_child_create is set to "0" the
+ * protection is disabled.
+ * When it's set to "1", notify about O_CREAT open missing the O_EXCL flag
+ * in world writable sticky directories.
+ * When it's set to "2", notify about O_CREAT open missing the O_EXCL flag
+ * in group writable sticky directories.
+ * When it's set to "3", block O_CREAT open missing the O_EXCL flag
+ * in world writable sticky directories and notify (but don't block)
+ * in group writable sticky directories.
+ * When it's set to "4", block O_CREAT open missing the O_EXCL flag
+ * in world writable and group writable sticky directories.
+ *
+ * Returns 0 if the open is allowed, -ve on error.
+ */
+static int may_create_no_excl(struct dentry * const dir,
+			      const unsigned char * const name,
+			      struct inode * const inode)
+{
+	umode_t mode = dir->d_inode->i_mode;
+
+	if (likely(!(mode & S_ISVTX)))
+		return 0;
+	if (inode && (uid_eq(inode->i_uid, dir->d_inode->i_uid) ||
+		      uid_eq(current_fsuid(), inode->i_uid)))
+		return 0;
+
+	if ((sysctl_protected_sticky_child_create && likely(mode & 0002)) ||
+	    (sysctl_protected_sticky_child_create >= 2 && mode & 0020)) {
+		pr_notice_ratelimited("unsafe O_CREAT open (missing O_EXCL) of '%s' in a sticky directory by UID %d, EUID %d, process %s:%d.\n",
+				      name,
+				      from_kuid(&init_user_ns, current_uid()),
+				      from_kuid(&init_user_ns, current_euid()),
+				      current->comm, current->pid);
+		if (sysctl_protected_sticky_child_create >= 4 ||
+		    (sysctl_protected_sticky_child_create == 3 &&
+		     likely(mode & 0002)))
+			return -EACCES;
+	}
+	return 0;
+}
+
 static __always_inline
 const char *get_link(struct nameidata *nd)
 {
@@ -3255,6 +3304,11 @@ static int lookup_open(struct nameidata *nd, struct path *path,
 			error = -EACCES;
 			goto out_dput;
 		}
+		if (!(open_flag & O_EXCL)) {
+			error = may_create_no_excl(dir, nd->last.name, NULL);
+			if (unlikely(error))
+				goto out_dput;
+		}
 		error = dir_inode->i_op->create(dir_inode, dentry, mode,
 						open_flag & O_EXCL);
 		if (error)
@@ -3421,6 +3475,9 @@ static int do_last(struct nameidata *nd,
 		error = may_create_in_sticky(dir, nd->last.name, inode);
 		if (unlikely(error))
 			goto out;
+		error = may_create_no_excl(dir, nd->last.name, inode);
+		if (unlikely(error))
+			goto out;
 	}
 	error = -ENOTDIR;
 	if ((nd->flags & LOOKUP_DIRECTORY) && !d_can_lookup(nd->path.dentry))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 14bb497..16025e6 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -73,6 +73,7 @@
 extern int sysctl_protected_hardlinks;
 extern int sysctl_protected_fifos;
 extern int sysctl_protected_regular;
+extern int sysctl_protected_sticky_child_create;
 
 typedef __kernel_rwf_t rwf_t;
 
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6b127e2..5927823 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -1825,6 +1825,15 @@ static int sysrq_sysctl_handler(struct ctl_table *table, int write,
 		.extra2		= &two,
 	},
 	{
+		.procname	= "protected_sticky_child_create",
+		.data		= &sysctl_protected_sticky_child_create,
+		.maxlen		= sizeof(int),
+		.mode		= 0600,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &four,
+	},
+	{
 		.procname	= "suid_dumpable",
 		.data		= &suid_dumpable,
 		.maxlen		= sizeof(int),
-- 
1.9.1

Re: [kernel-hardening] [RFC v2 2/2] Protected O_CREAT open in sticky directory

[Jann Horn]

On Tue, Sep 26, 2017 at 4:14 PM, Salvatore Mesoraca
<s.mesoraca16@gmail.com> wrote:
> Disallows O_CREAT open missing the O_EXCL flag, in world or
> group writable directories, even if the file doesn't exist yet.
> With few exceptions (e.g. shared lock files based on flock())
> if a program tries to open a file with the O_CREAT flag and
> without the O_EXCL, it probably has a bug.
> This feature allows to detect and potentially block programs that
> act this way and can be used to find vulnerabilities (like those
> prevented by patch #1) and to do policy enforcement.
[...]
> +static int may_create_no_excl(struct dentry * const dir,
> +                             const unsigned char * const name,
> +                             struct inode * const inode)
> +{
> +       umode_t mode = dir->d_inode->i_mode;
> +
> +       if (likely(!(mode & S_ISVTX)))
> +               return 0;
> +       if (inode && (uid_eq(inode->i_uid, dir->d_inode->i_uid) ||
> +                     uid_eq(current_fsuid(), inode->i_uid)))
> +               return 0;

Why is there an exception for inode->i_uid==dir->d_inode->i_uid? Is this
kind of behavior more likely to be correct when the directory owner is
doing it?


> +       if ((sysctl_protected_sticky_child_create && likely(mode & 0002)) ||
> +           (sysctl_protected_sticky_child_create >= 2 && mode & 0020)) {
> +               pr_notice_ratelimited("unsafe O_CREAT open (missing O_EXCL) of '%s' in a sticky directory by UID %d, EUID %d, process %s:%d.\n",
> +                                     name,
> +                                     from_kuid(&init_user_ns, current_uid()),
> +                                     from_kuid(&init_user_ns, current_euid()),
> +                                     current->comm, current->pid);

As far as I can tell, uid_t is unsigned, so the first two "%d"
format string elements are technically wrong:
  extern uid_t from_kuid(struct user_namespace *to, kuid_t uid);
  typedef __kernel_uid32_t uid_t;
  typedef unsigned int __kernel_uid32_t;

> +               if (sysctl_protected_sticky_child_create >= 4 ||
> +                   (sysctl_protected_sticky_child_create == 3 &&
> +                    likely(mode & 0002)))
> +                       return -EACCES;
> +       }
> +       return 0;
> +}

Re: [kernel-hardening] [RFC v2 2/2] Protected O_CREAT open in sticky directory

[Salvatore Mesoraca]

2017-09-26 16:40 GMT+02:00 Jann Horn <jannh@google.com>:
> On Tue, Sep 26, 2017 at 4:14 PM, Salvatore Mesoraca
> <s.mesoraca16@gmail.com> wrote:
>> Disallows O_CREAT open missing the O_EXCL flag, in world or
>> group writable directories, even if the file doesn't exist yet.
>> With few exceptions (e.g. shared lock files based on flock())
>> if a program tries to open a file with the O_CREAT flag and
>> without the O_EXCL, it probably has a bug.
>> This feature allows to detect and potentially block programs that
>> act this way and can be used to find vulnerabilities (like those
>> prevented by patch #1) and to do policy enforcement.
> [...]
>> +static int may_create_no_excl(struct dentry * const dir,
>> +                             const unsigned char * const name,
>> +                             struct inode * const inode)
>> +{
>> +       umode_t mode = dir->d_inode->i_mode;
>> +
>> +       if (likely(!(mode & S_ISVTX)))
>> +               return 0;
>> +       if (inode && (uid_eq(inode->i_uid, dir->d_inode->i_uid) ||
>> +                     uid_eq(current_fsuid(), inode->i_uid)))
>> +               return 0;
>
> Why is there an exception for inode->i_uid==dir->d_inode->i_uid? Is this
> kind of behavior more likely to be correct when the directory owner is
> doing it?

That exception is there because we can't really prevent the directory
owner from playing nasty tricks with other users files, so there is no
point in trying to stop him.
Nevertheless, if we intend this feature just as a way to detect possible
vulnerabilities, it may be useful to remove that exception.
I'll do it in the next revision.

>> +       if ((sysctl_protected_sticky_child_create && likely(mode & 0002)) ||
>> +           (sysctl_protected_sticky_child_create >= 2 && mode & 0020)) {
>> +               pr_notice_ratelimited("unsafe O_CREAT open (missing O_EXCL) of '%s' in a sticky directory by UID %d, EUID %d, process %s:%d.\n",
>> +                                     name,
>> +                                     from_kuid(&init_user_ns, current_uid()),
>> +                                     from_kuid(&init_user_ns, current_euid()),
>> +                                     current->comm, current->pid);
>
> As far as I can tell, uid_t is unsigned, so the first two "%d"
> format string elements are technically wrong:
>   extern uid_t from_kuid(struct user_namespace *to, kuid_t uid);
>   typedef __kernel_uid32_t uid_t;
>   typedef unsigned int __kernel_uid32_t;

You are right. I'll fix it in the next version.

Thank you very much for taking the time to look at my email.

Salvatore