On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca
<s.mesoraca16@xxxxxxxxx> wrote:
> Disallows writing into 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.
Thanks for working on this!
> The purpose is to make data spoofing attacks harder.
Do you have any examples of attacks (CVEs, blog posts, etc) that you
could link to in this commit?
> 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 <solar at openwall.com>.
The email address obfuscation can be dropped here, it's already listed
in the "Suggested-by". :)
>
> Suggested-by: Solar Designer <solar@xxxxxxxxxxxx>
> Suggested-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Signed-off-by: Salvatore Mesoraca <s.mesoraca16@xxxxxxxxx>
> ---
> Documentation/sysctl/fs.txt | 34 ++++++++++++++++++++++++++++++++++
> fs/namei.c | 23 +++++++++++++++++++++++
> include/linux/fs.h | 2 ++
> kernel/sysctl.c | 18 ++++++++++++++++++
> 4 files changed, 77 insertions(+)
>
> diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt
> index 35e17f7..18e16b7 100644
> --- a/Documentation/sysctl/fs.txt
> +++ b/Documentation/sysctl/fs.txt
> @@ -36,6 +36,8 @@ Currently, these files are in /proc/sys/fs:
> - pipe-user-pages-soft
> - protected_hardlinks
> - protected_symlinks
> +- protected_fifos
> +- protected_regular_files
bike shed: I think "_files" is redundant, since we're already in proc/sys/fs/
Also, here and later, keep this list alphabetized (fifos, hardlinks,
regular_files, symlinks).
> - suid_dumpable
> - super-max
> - super-nr
> @@ -222,6 +224,38 @@ This protection is based on the restrictions in Openwall and grsecurity.
>
> ==============================================================
>
> +protected_fifos:
> +
> +The intent of this protection is to avoid unintentional writes to
> +an attacker-controlled FIFO, where program expected to create a regular
> +file.
Typo: "a program" or "programs", instead of "program".
> +
> +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.
> +
> +This protection is based on the restrictions in Openwall.
> +
> +==============================================================
> +
> +protected_regular_files:
> +
> +This protection is similar to protected_fifos, but it
> +avoids writes to an attacker-controlled regular file, where 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.
> +
> +This protection is based on the restrictions in Openwall.
> +
> +==============================================================
> +
> suid_dumpable:
>
> This value can be used to query and set the core dump mode for setuid
> diff --git a/fs/namei.c b/fs/namei.c
> index c75ea03..5459dbc 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3225,6 +3225,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
> return error;
> }
>
> +int sysctl_protected_fifos __read_mostly;
> +int sysctl_protected_regular_files __read_mostly;
> +
> /*
> * Handle the last step of open()
> */
> @@ -3354,6 +3357,26 @@ static int do_last(struct nameidata *nd,
>
> seq = 0; /* out of RCU mode, so the value doesn't matter */
> inode = d_backing_inode(path.dentry);
> + /*
> + * When this protection is turned on via sysctl:
> + * don't allow "O_CREAT" open on FIFOs or regular files that we
> + * don't own in world writable sticky directories, unless they
> + * are owned by the owner of the directory.
> + */
> + if (((sysctl_protected_fifos && S_ISFIFO(inode->i_mode)) ||
> + (sysctl_protected_regular_files && S_ISREG(inode->i_mode))) &&
> + (dir->d_inode->i_mode & (S_ISVTX|S_IWOTH) == (S_ISVTX|S_IWOTH)) &&
> + !uid_eq(inode->i_uid, dir->d_inode->i_uid) &&
> + !uid_eq(current_fsuid(), inode->i_uid)) {
> + pr_notice_ratelimited("denied writing in file of %d.%d in a sticky directory by UID %d, EUID %d, process %s:%d.",
> + 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);
> + path_to_nameidata(&path, nd);
> + return -EACCES;
> + }
I think instead of putting this inline in do_last(), you can make a
helper function (e.g. may_creat()), as done for the hardlink and
symlink restrictions. Also, I think you'll need to do this check later
and merge with with the existing O_CREAT flag sanity check:
if (open_flag & O_CREAT) {
error = -EISDIR;
if (d_is_dir(nd->path.dentry))
goto out;
error = may_creat(dir, inode);
if (unlikely(error))
goto out;
}
> finish_lookup:
> error = step_into(nd, &path, 0, inode, seq);
> if (unlikely(error))
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 339e737..591ae87 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_files;
>
> typedef __kernel_rwf_t rwf_t;
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 6648fbb..b18d500 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 = &one,
> + },
> + {
> + .procname = "protected_regular_files",
> + .data = &sysctl_protected_regular_files,
> + .maxlen = sizeof(int),
> + .mode = 0600,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> + {
> .procname = "suid_dumpable",
> .data = &suid_dumpable,
> .maxlen = sizeof(int),
> --
> 1.9.1
>
Otherwise, yeah, looks good! :)
-Kees
--
Kees Cook
Pixel Security
