From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: AG47ELs+omtss1c6BTF6n2az22QHArAWmiG4i4iZZtb58QM0pWqwgkH86AVuJ5qfQitXKCYuKqoO ARC-Seal: i=1; a=rsa-sha256; t=1519809772; cv=none; d=google.com; s=arc-20160816; b=h1R9OZhv/u/z0bT9DVEql8wqPDwkX5yZPWOQu+jUIffOqSgw6csbawBF3aN2ctr1Wk DJqRswF0+280Y1Zk9WuRJZbghOZelwO/PJy6T55Ue9iSep8wL4ak7Xmc0ySjL3nLxtEE XcUYsvEeCJ4/4h1Yda2aOQd4ObwKbAk1gEKN6AuE4YOkGZiadAQapR3F8A/9P8wExuqa rjw1r9naRlBtbl3F7hSmAd9GovHs0sPjZnjLP5px1pRw0tYVx6TpfXNJ5oAMG81m3h2/ mS+rdJSfMltALmmhnuG8Y7POi+iIB4xgXZDusKihVWT5ILifQX3b+j5wlLN3LJC7yXSg Wg7A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=cc:to:subject:message-id:date:from:references:in-reply-to :mime-version:dkim-signature:delivered-to:list-id:list-subscribe :list-unsubscribe:list-help:list-post:precedence:mailing-list :arc-authentication-results; bh=Xcvb9zJY8r6bBpA6DWD2YX1cSAodGsQm7jafOJDgjBA=; b=waPpJ3g7mbYpVJSs0UgWr2yfXF9QwW546QrKkVOG8WBAnLWdLyMgRCqQUSPx0BI66M wY8PtwAoaMLnckMneHVbNKaIP7AQRpsq/cUtiF2rbhZK2n84abP2xD0rOhCeAYUrqxkz gQC3SxFiwB3G4KFIkSZLD8zxkm6UZCcNe3cHV/OekLnPN//31/ra2ZMiKIoMJ6H7ZxP4 GMWIyrhkBrw2FnpF0qfMBOC6bgTY3FyPHMSAaNjTzJlzwLipQiyGXbktRWBLnCU4ENrG FugLhlNOWSAS4O06CpggUPst5mFks4r5gc5LrAVfUP/x21/WZEz2z9HDup370ekbehd1 S3Ug== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=oZHBcXLH; spf=pass (google.com: domain of kernel-hardening-return-12031-gregkh=linuxfoundation.org@lists.openwall.com designates 195.42.179.200 as permitted sender) smtp.mailfrom=kernel-hardening-return-12031-gregkh=linuxfoundation.org@lists.openwall.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=oZHBcXLH; spf=pass (google.com: domain of kernel-hardening-return-12031-gregkh=linuxfoundation.org@lists.openwall.com designates 195.42.179.200 as permitted sender) smtp.mailfrom=kernel-hardening-return-12031-gregkh=linuxfoundation.org@lists.openwall.com; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm List-Post: List-Help: List-Unsubscribe: List-Subscribe: MIME-Version: 1.0 In-Reply-To: References: <1519729200-16056-1-git-send-email-s.mesoraca16@gmail.com> From: Salvatore Mesoraca Date: Wed, 28 Feb 2018 10:22:10 +0100 Message-ID: Subject: Re: [PATCH v4] Protected FIFOs and regular files To: Kees Cook Cc: LKML , Kernel Hardening , "linux-fsdevel@vger.kernel.org" , Alan Cox , Alexander Viro , David Laight , Ian Campbell , Jann Horn , Matthew Wilcox , Pavel Vasilyev , Solar Designer , "Eric W. Biederman" , "Tobin C. Harding" Content-Type: text/plain; charset="UTF-8" X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1593551596949260408?= X-GMAIL-MSGID: =?utf-8?q?1593636051536343824?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: 2018-02-27 21:22 GMT+01:00 Kees Cook : > On Tue, Feb 27, 2018 at 11:47 AM, Kees Cook wrote: >> On Tue, Feb 27, 2018 at 3:00 AM, Salvatore Mesoraca >> wrote: >>> 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 hardlinks/symlinks restrictions, fifos/regular >>> files weren't the favorite vehicle to exploit them. >>> >>> Suggested-by: Solar Designer >>> Suggested-by: Kees Cook >>> Signed-off-by: Salvatore Mesoraca >>> --- >>> >>> Notes: >>> Changes in v3: >>> - Fixed format string for uid_t that is unsigned >>> (suggested by Jann Horn). >>> Changes in v4: >>> - Some English fixes (suggested by Tobin C. Harding). >>> - The original patchset has been split to help this part >>> land upstream (suggested by Solar Designer). >>> >>> 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 6c00c1e..819caf8 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", writing to FIFOs 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 applies 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", writing to regular files 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 applies 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 921ae32..eaab668 100644 >>> --- a/fs/namei.c >>> +++ b/fs/namei.c >>> @@ -883,6 +883,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 >>> @@ -996,6 +998,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, on files that already >>> + * exist. >>> + * @dir: the sticky 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 %u.%u in a sticky directory by UID %u, EUID %u, 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); >> >> Instead of this pr_notice_ratelimited(), I think >> audit_log_link_denied() should be refactored and used instead. Drop >> this line from this patch, and I think this is great as-is. The >> logging can be separate (as it may get heavily bike-shed, as I >> experienced with hard/symlink restrictions). >> >> Otherwise, I think this looks great. >> >> Acked-by: Kees Cook > > I've also tested this now; so: > > Tested-by: Kees Cook > > # grep . /proc/sys/fs/protected_* > /proc/sys/fs/protected_fifos:0 > /proc/sys/fs/protected_hardlinks:1 > /proc/sys/fs/protected_regular:0 > /proc/sys/fs/protected_symlinks:1 > # cd /tmp > # mkfifo fifo > # touch regular > # chown nobody fifo regular > # chmod a+w fifo regular > # chmod a+w regular > # cat fifo > output & > # su - keescook > $ cd /tmp > $ python > ... >>>> import os >>>> fd = os.open("fifo", os.O_RDWR | os.O_CREAT) >>>> os.write(fd, "OHAI\n") > 5 >>>> fd = os.open("regular", os.O_RDWR | os.O_CREAT) >>>> os.write(fd, "OHAI\n") > 5 >>>> exit() > $ exit > # echo 1 > /proc/sys/fs/protected_fifos > # echo 1 > /proc/sys/fs/protected_regular > # su - keescook > $ cd /tmp > $ python > ... >>>> import os >>>> fd = os.open("fifo", os.O_RDWR | os.O_CREAT) > Traceback (most recent call last): > File "", line 1, in > OSError: [Errno 13] Permission denied: 'fifo' >>>> fd = os.open("regular", os.O_RDWR | os.O_CREAT) > Traceback (most recent call last): > File "", line 1, in > OSError: [Errno 13] Permission denied: 'regular' > >> I'll create a branch for this on git.kernel.org and see if anything >> surprising pops out. :) > > Here it is with my suggested refactoring of the audit message: > > https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=kspp/userspace/protected-creat > > Which produces: > > [ 146.854080] audit: type=1703 audit(1519762816.978:95): op=fifo > ppid=3091 pid=3092 auid=0 uid=1000 gid=1000 euid=1000 suid=1000 > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts8 ses=1 comm="python" > exe="/usr/bin/python2.7" res=0 > [ 146.858691] audit: type=1302 audit(1519762816.978:95): item=0 > name="/tmp/fifo" inode=531 dev=fd:01 mode=010666 ouid=65534 ogid=0 > rdev=00:00 nametype=NORMAL cap_fp=0000000000000000 > cap_fi=0000000000000000 cap_fe=0 cap_fver=0 > ... > [ 152.993518] audit: type=1703 audit(1519762823.117:96): op=regular > ppid=3091 pid=3092 auid=0 uid=1000 gid=1000 euid=1000 suid=1000 > fsuid=1000 egid=1000 sgid=1000 fsgid=1000 tty=pts8 ses=1 comm="python" > exe="/usr/bin/python2.7" res=0 > [ 152.997963] audit: type=1302 audit(1519762823.117:96): item=0 > name="/tmp/regular" inode=700 dev=fd:01 mode=0100666 ouid=65534 ogid=0 > rdev=00:00 nametype=NORMAL cap_fp=0000000000000000 > cap_fi=0000000000000000 cap_fe=0 cap_fver=0 > > and other things (uid, etc) Awesome! Thank you very much for your help! Salvatore