Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751348AbdIRVA4 (ORCPT ); Mon, 18 Sep 2017 17:00:56 -0400 Received: from mail-io0-f179.google.com ([209.85.223.179]:49739 "EHLO mail-io0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750813AbdIRVAx (ORCPT ); Mon, 18 Sep 2017 17:00:53 -0400 X-Google-Smtp-Source: AOwi7QCbZhEMEY0afwkrS7jTP7rQYp1/umR65Yx9QDxw4aR7d6Mu/qOzD9KrrVrhaKvZQI9BNCxRrRq7+qszd+8dW5g= MIME-Version: 1.0 In-Reply-To: <1505465028-15256-1-git-send-email-s.mesoraca16@gmail.com> References: <1505465028-15256-1-git-send-email-s.mesoraca16@gmail.com> From: Kees Cook Date: Mon, 18 Sep 2017 14:00:50 -0700 X-Google-Sender-Auth: 738k1XsMCfx01D4drCTNJ-jYN1s Message-ID: Subject: Re: [RFC] Restrict writes into untrusted FIFOs and regular files To: Salvatore Mesoraca Cc: Kernel Hardening , Solar Designer , Alexander Viro , "Eric W. Biederman" , LKML , "linux-fsdevel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7420 Lines: 202 On Fri, Sep 15, 2017 at 1:43 AM, Salvatore Mesoraca 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 . The email address obfuscation can be dropped here, it's already listed in the "Suggested-by". :) > > Suggested-by: Solar Designer > Suggested-by: Kees Cook > Signed-off-by: Salvatore Mesoraca > --- > 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