From patchwork Tue Jun 20 21:02:40 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kees Cook X-Patchwork-Id: 9800267 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2DA596038C for ; Tue, 20 Jun 2017 21:03:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 14B692846B for ; Tue, 20 Jun 2017 21:03:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 097FC28455; Tue, 20 Jun 2017 21:03:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from mother.openwall.net (mother.openwall.net [195.42.179.200]) by mail.wl.linuxfoundation.org (Postfix) with SMTP id B43B7223A6 for ; Tue, 20 Jun 2017 21:02:58 +0000 (UTC) Received: (qmail 20449 invoked by uid 550); 20 Jun 2017 21:02:56 -0000 Mailing-List: contact kernel-hardening-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Delivered-To: mailing list kernel-hardening@lists.openwall.com Received: (qmail 20376 invoked from network); 20 Jun 2017 21:02:53 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=7NnipDOkGN0ge8DDl4mecS4xPsZLMCnGbQ2k40+HVfE=; b=Bru2nAV7HKWx7jeipB8/BeNzYvf9Wuf63825imBmE1Hm7EfiCg+APfNhOV4cNgaKEv KaVW//U2KGIO0P4Vi53+P/aKmO/K88ceQiPGtoy0ujHpeoGOgrea0trwtBYUjPgCkGe/ xAEj9p96J1H0WBuijGXZJhDYIjgO21EQsRAYBLjRjuiTlcjo07CxHibs4niKu8QfurgT GyuNhslAeSghQNH2Cb0aCO9ig9ByZXkpv+Du3JwLRLM4HRjAcoNw387sUhrXjuIglYb5 YTNWNX0mByYGYblvDfUi3+ELSSjK38MlBUUgSLQQ8vmyZhMd+Ycs9h91DfQ+8FF8lHD5 3B7A== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=7NnipDOkGN0ge8DDl4mecS4xPsZLMCnGbQ2k40+HVfE=; b=Er5S5hM7TaiynuKQMoHpCYLnh/59UEM6fkX3QGw0TnBiUyRcDf5fHS8Pw+x0Ys9jgV 4mN31EuK8TJFyKNMb05ml+zLYN1jTSFb9Wz5v7qDE/PFX70LDExGQ3ZW/6HJV+f/z1Ny ZnXi8fQPSXJpRfxX+fqPgaduf3VgPw+3eQukM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=7NnipDOkGN0ge8DDl4mecS4xPsZLMCnGbQ2k40+HVfE=; b=ppM4XX9QG+lAC+dcQLBXaB7zkm9bs+AtRLV4TFzftpNKygzmCCHqbRDj+nopcKn5HJ R3RwTbIVaZ9I8oAfAQyzl6a5i9YuAypSzexdXWvSOqnqWmN8Ip7E6qpY+FpWUemZ7k9P sFv6tdrlisSWMKjQooddrRxNPs56zjNh7TxD+BedSJAf71P012bop4UKw1jxIQPTgYic F5wHp3m0CCgC+bZSRQbcmuFXnRWjz26tfd7X+IjZiLpNiwg3O5MEt52pS6ahWO7JZ1An TDCTAIvFnhyijYIStxMr8uQgKMRpdUOlq5veTyG1m7czl+ERjaITh4Ojsd3aIexl09b/ PmvA== X-Gm-Message-State: AKS2vOwciIR9rWgTsS2WQHlNPNHEzM3YFz1xqAwSA2Bj+labyEhpt8wL 4SqStWC8oVjiuIKjv2ZxEbKOTri+TCcD X-Received: by 10.107.35.83 with SMTP id j80mr26578277ioj.72.1497992561280; Tue, 20 Jun 2017 14:02:41 -0700 (PDT) MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: <20170608211817.GA31010@openwall.com> References: <20170608211817.GA31010@openwall.com> From: Kees Cook Date: Tue, 20 Jun 2017 14:02:40 -0700 X-Google-Sender-Auth: m96want_MWJ4rIeMC_OgEIuQtwA Message-ID: To: Solar Designer , "Serge E. Hallyn" , Andy Lutomirski Cc: "kernel-hardening@lists.openwall.com" Subject: [kernel-hardening] Re: hard link restrictions X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 8, 2017 at 2:18 PM, Solar Designer wrote: > Kees, all - > > My renewed interest in hard link restrictions was in context of crontab > vs. crond privsep: > > http://www.openwall.com/lists/oss-security/2017/06/08/3 > > Under that threat model (mostly overlooked/neglected so far?), any > hard link to another user's (or root's) file is risky. Even a file the > linking user could readily read and write. For crond specifically, this > is not the case because it will refuse to process files with extra write > permissions. But for other services not yet hardened like this, e.g. > mode 666 files hard-linked into their queue, etc. directories could be > usable for attacks. > > Another subtle scenario where a hard link to another user's writable > file could help the attacker is preserving one's ability to bypass disk > quota via that file, even after the original owner would have deleted > their original link to the file. Similarly, it'd allow for keeping the > other user's disk quota consumption even until after that user would > have deleted their original link and wanted the quota usage reclaimed. > > Because those hard link restrictions were so non-standard back > when they were new, we applied them only to files the user could not > readily read and write, plus to SUIDs/SGIDs for the "pinning" concern. > We tried to minimize breakage of programs relying on being able to hard > link to arbitrary files. > > Maybe now is the time to introduce a stricter mode, perhaps enabled with > "fs.protected_hardlinks = 2", where any hard links to other users' files > would be disallowed, except when the invoking process has CAP_FOWNER? I wouldn't be opposed to this idea. I always found hardlink behavior to be surprising. > In code, this would be skipping the "|| safe_hardlink_source(inode)" in: > > /* Source inode owner (or CAP_FOWNER) can hardlink all they like, > * otherwise, it must be a safe source. > */ > if (inode_owner_or_capable(inode) || safe_hardlink_source(inode)) > return 0; Yup, agreed. Pardon the gmail-induced whitespace damage: > While we're at it, doesn't the above code unnecessarily set PF_SUPERPRIV > (which is then reported via BSD process accounting) when the CAP_FOWNER > check inside inode_owner_or_capable() is reached and passed, but > safe_hardlink_source() later returns false? Erm, yeah, good point. > In fact, inode_owner_or_capable() itself might also be problematic in > this respect in that it'd set PF_SUPERPRIV even if kuid_has_mapping() > later fails: > > if (ns_capable(ns, CAP_FOWNER) && kuid_has_mapping(ns, inode->i_uid)) > return true; I thought there was an alternative capable() check that didn't set PF_SUPERPRIV... Ah, "has_*" prefix doesn't set them, but we should fix these others since they may actually be using a capability. > Or has the kernel gave up on being careful not to set PF_SUPERPRIV > unnecessarily? Sometimes it's a conflicting goal to minimizing the > attack surface and improving performance in case of request flood DoS > attacks, where it's best to stop processing the request sooner ("you > would not be capable anyway") than later (after expensive other checks). Right, I think it's not well audited. I'd expect anything with an expensive check to use has_* first and then DTRT with PF_SUPERPRIV, but that doesn't look to be the case in may_linkat() nor inode_owner_or_capable(). -Kees diff --git a/Documentation/sysctl/fs.txt b/Documentation/sysctl/fs.txt index 35e17f748ca7..aea564ee5f00 100644 --- a/Documentation/sysctl/fs.txt +++ b/Documentation/sysctl/fs.txt @@ -198,6 +198,9 @@ When set to "0", hardlink creation behavior is unrestricted. When set to "1" hardlinks cannot be created by users if they do not already own the source file, or do not have read/write access to it. +When set to "2" hardlinks cannot be created by users if they do not +already own the source file. + This protection is based on the restrictions in Openwall and grsecurity. ============================================================== diff --git a/fs/namei.c b/fs/namei.c index 6571a5f5112e..0c52c0f8eebd 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -1005,10 +1005,12 @@ static int may_linkat(struct path *link) inode = link->dentry->d_inode; - /* Source inode owner (or CAP_FOWNER) can hardlink all they like, - * otherwise, it must be a safe source. - */ - if (inode_owner_or_capable(inode) || safe_hardlink_source(inode)) + /* Source inode owner (or CAP_FOWNER) can hardlink all they like. */ + if (inode_owner_or_capable(inode)) + return 0; + + /* Otherwise, mode 1 allows a reasonable source. */ + if (sysctl_protected_hardlinks < 2 && safe_hardlink_source(inode)) return 0; audit_log_link_denied("linkat", link); diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 4dfba1a76cc3..827ec97a0898 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1778,7 +1778,7 @@ static struct ctl_table fs_table[] = { .mode = 0600, .proc_handler = proc_dointvec_minmax, .extra1 = &zero, - .extra2 = &one, + .extra2 = &two, }, { .procname = "suid_dumpable",