From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A648C43382 for ; Tue, 25 Sep 2018 16:38:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AE8062083A for ; Tue, 25 Sep 2018 16:37:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AE8062083A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=tycho.nsa.gov Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727171AbeIYWqP (ORCPT ); Tue, 25 Sep 2018 18:46:15 -0400 Received: from uhil19pa11.eemsg.mail.mil ([214.24.21.84]:19560 "EHLO uhil19pa11.eemsg.mail.mil" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726026AbeIYWqO (ORCPT ); Tue, 25 Sep 2018 18:46:14 -0400 X-EEMSG-check-008: 339852509|UHIL19PA11_EEMSG_MP9.csd.disa.mil Received: from emsm-gh1-uea11.ncsc.mil ([214.29.60.3]) by uhil19pa11.eemsg.mail.mil with ESMTP/TLS/DHE-RSA-AES256-SHA256; 25 Sep 2018 16:37:54 +0000 X-IronPort-AV: E=Sophos;i="5.54,302,1534809600"; d="scan'208";a="18644476" IronPort-PHdr: =?us-ascii?q?9a23=3A81y+9xVBYFAVylKTyp0dLX68s+TV8LGtZVwlr6?= =?us-ascii?q?E/grcLSJyIuqrYbR2Ot8tkgFKBZ4jH8fUM07OQ7/i/HzRYqb+681k6OKRWUB?= =?us-ascii?q?EEjchE1ycBO+WiTXPBEfjxciYhF95DXlI2t1uyMExSBdqsLwaK+i764jEdAA?= =?us-ascii?q?jwOhRoLerpBIHSk9631+ev8JHPfglEnjWwba9wIRmssQndqtQdjJd/JKo21h?= =?us-ascii?q?bHuGZDdf5MxWNvK1KTnhL86dm18ZV+7SleuO8v+tBZX6nicKs2UbJXDDI9M2?= =?us-ascii?q?Ao/8LrrgXMTRGO5nQHTGoblAdDDhXf4xH7WpfxtTb6tvZ41SKHM8D6Uaw4VD?= =?us-ascii?q?K/5KpwVhTmlDkIOCI48GHPi8x/kqRboA66pxdix4LYeZyZOOZicq/Ye94VS3?= =?us-ascii?q?BBXsJMXCJfBI2yYZYEA+4YMepGs4Xxol0Dpga8CwaxHuPi0iJGiGH43aM60O?= =?us-ascii?q?ovHw/J0wMiEN0Sv3rZt8n1OaUIXOyp0KXFwzfOYvVL0jn98ojIdRUhrOmRU7?= =?us-ascii?q?Jsb8XR0UkvGB3Djl6NtILlOima1uAJs2eF7+trSOWii3U6pAFquTWv2scthZ?= =?us-ascii?q?XJhoIS0FzE8z55z5wvKd23T057f8epHZ1NvC+ZL4t7Wt4uTm5ntSogyrAKpI?= =?us-ascii?q?S3cDYFxZg53RLTdvqKeJWS7B35TuaeOzJ4iWpgeLK4mhm971Ctyvb5VsmoyF?= =?us-ascii?q?ZKqTdFksXUunANyRPT7s+HR+Nh/ki7wzaP1h3T6vpeLUAolavUN54hwrkqmp?= =?us-ascii?q?oVrUvDBTP5lF/zjK+XckUo4umo6+L5bbX6vpKQKoB5hw7kPqkuh8CzG/o0Pw?= =?us-ascii?q?cQU2SB5OiwzLjj8lf4QLVOgP02iK7ZsJXCKMQAu6G5GBRY0poj6hmjDzem18?= =?us-ascii?q?4UnX8cLF1fYh6HgI/pO0/WLPDiEfi/m0iskCtsx/3eJr3uGIjNLnzYnbf5Z7?= =?us-ascii?q?l99kpcxBM2zdBY4JJUEK8OLOjvVU/2sdzSFgU5PBCsw+b7FNV90ZsTWX6VDa?= =?us-ascii?q?+aN6PSt0KH5vg1LOSXeIAVuS39JOQi5/L0kXA5nlodd7Gz3ZQLcHC4AuhmI0?= =?us-ascii?q?KBbHrog9cBF3oKvwUnQOzpllKCSzhTaGi2X68n+DE7B5ypDZ3ZSoCunrOBxi?= =?us-ascii?q?G7EYNSZmxcDVCMC3jofZ2eW/gQcCKSPtNhkjscWLmuVYAtzBWutA78y7p6Ie?= =?us-ascii?q?vY4zMXtJ3/1Ndr/e3Tkws99ThvAsuB0mGNVH17nmQSSzAq26B/pB819lDW6a?= =?us-ascii?q?Flh7R9EttJ6rsdSg4nMbbExvF+TtX1XRjMONyOTQDiCsmvAjY3Qsg469QPeE?= =?us-ascii?q?16Xd6li0PtxS2vVoQJmqSLCZp8yafV23z8No4p0Hrd/LUwhFkhBM1UPCupgb?= =?us-ascii?q?AppFubPJLAj0jMz/XiTq8bxiOYsT7ZlWc=3D?= X-IPAS-Result: =?us-ascii?q?A2CAAADdY6pb/wHyM5BbGgEBAQEBAgEBAQEHAgEBAQGDN?= =?us-ascii?q?SqBZCiDdJRCUAEBBoEILYhoj2A2AYRAAoNmITgUAQMBAQEBAQECAWwogjUkA?= =?us-ascii?q?YJeAQEBAQIBIxVBEAsYAgImAgJXBgEMBgIBAReCRz+BdQUIpA+BLoR3hSOBC?= =?us-ascii?q?4lvF3mBB4E5gW1+h3+CVwKIRDCFCUGOQQmQIwYXjyyWYiGBVSsIAhgIIQ+DJ?= =?us-ascii?q?4IgBReONCMwewEBjRYBAQ?= Received: from tarius.tycho.ncsc.mil ([144.51.242.1]) by emsm-gh1-uea11.NCSC.MIL with ESMTP; 25 Sep 2018 16:37:54 +0000 Received: from moss-pluto.infosec.tycho.ncsc.mil (moss-pluto.infosec.tycho.ncsc.mil [192.168.25.131]) by tarius.tycho.ncsc.mil (8.14.4/8.14.4) with ESMTP id w8PGbpFf015840; Tue, 25 Sep 2018 12:37:53 -0400 Subject: Re: [RFC PATCH] selinux: add a fallback to defcontext for native labeling To: Paul Moore , takondra@cisco.com Cc: linux-kernel@vger.kernel.org, selinux@tycho.nsa.gov, xe-linux-external@cisco.com References: <20180919165248.53090-1-takondra@cisco.com> <153741130781.7326.1773144725860636439@takondra-t460s> <9a944aea-f35e-5a49-841d-6bfca8f17b9d@tycho.nsa.gov> <153748436388.5590.1406237328277739821@takondra-t460s> <66ec16b0-a335-d9ef-deb3-ef391cdd66e0@tycho.nsa.gov> <153785433233.5039.17960184426345262866@takondra-t460s> From: Stephen Smalley Message-ID: Date: Tue, 25 Sep 2018 12:39:55 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/25/2018 12:03 PM, Paul Moore wrote: > On Tue, Sep 25, 2018 at 9:58 AM Stephen Smalley wrote: >> On 09/25/2018 01:45 AM, Taras Kondratiuk wrote: >>> Quoting Paul Moore (2018-09-24 20:46:57) >>>> On Fri, Sep 21, 2018 at 10:39 AM Stephen Smalley wrote: >>>>> On 09/20/2018 06:59 PM, Taras Kondratiuk wrote: >>>>>> Quoting Stephen Smalley (2018-09-20 07:49:12) >>>>>>> On 09/19/2018 10:41 PM, Taras Kondratiuk wrote: >>>>>>>> Quoting Stephen Smalley (2018-09-19 12:00:33) >>>>>>>>> On 09/19/2018 12:52 PM, Taras Kondratiuk wrote: >>>> >>>> ... >>>> >>>>>> IMO it would be more consistent if defcontext cover all "unlabeled" >>>>>> groups. It seems unlikely to me that somebody who currently uses >>>>>> defcontext can somehow rely on mapping invalid labels to unlabeled >>>>>> instead of default context. >>>>> >>>>> Yes, and that seems more consistent with the current documentation in >>>>> the mount man page for defcontext=. >>>>> >>>>> I'd be inclined to change selinux_inode_notifysecctx() to call >>>>> security_context_to_sid_default() directly instead of using >>>>> selinux_inode_setsecurity() and change security_context_to_sid_core() >>>>> and sidtab_search_core() as suggested above to save and use the def_sid >>>>> instead of SECINITSID_UNLABELED always (initializing the context def_sid >>>>> to SECINITSID_UNLABELED as the default). selinux_inode_setsecurity() we >>>>> should leave unchanged, or if we change it at all, it should be more >>>>> like the handling in selinux_inode_setxattr(). The notifysecctx hook is >>>>> invoked by the filesystem to notify the security module of the file's >>>>> existing security context, so in that case we always want the _default >>>>> behavior, whereas the setsecurity hook is invoked by the vfs or the >>>>> filesystem to set the security context of a file to a new value, so in >>>>> that case we would only use the _force interface if the caller had >>>>> CAP_MAC_ADMIN. >>>>> >>>>> Paul, what say you? NB This would be a user-visible behavior change for >>>>> mounts specifying defcontext= on xattr filesystems; files with invalid >>>>> contexts will then show up with the defcontext value instead of the >>>>> unlabeled context. If that's too risky, then we'd need a flag or >>>>> something to security_context_to_sid_default() to distinguish the >>>>> behaviors and only set it when called from selinux_inode_notifysecctx(). >>>> >>>> Visible changes like this are always worrisome, even though I think it >>>> is safe to assume that the defcontext option is not widely used. I'd >>>> feel much better if this change was opt-in. >>>> >>>> Which brings about it's own problems. We have the policy capability >>>> functionality, but that is likely a poor fit for this as the policy >>>> capabilities are usually controlled by the Linux distribution while >>>> the mount options are set by the system's administrator when the >>>> filesystem is mounted. We could add a toggle somewhere in selinuxfs, >>>> but I really dislike that idea, and would prefer to find a different >>>> solution if possible. I'm not sure how much flak we would get for >>>> introducing a new mount option, but perhaps that is the best way to >>>> handle this: defcontext would continue to behave as it does now, but >>>> new option X would behave as mentioned in this thread. >>>> >>>> Thoughts? >>> >>> The new option X will also mean "default context", so it will be pretty >>> hard to come up with a different but a sensible name. I'm afraid that >>> having two options with hardly distinguishable semantics will be very >>> confusing. >>> >>> What about a kernel config option that modifies the behavior of >>> defcontext? >> >> First, the existing documentation for defcontext= leaves open the door >> to the proposed new behavior. From mount(8): >> "You can set the default security context for unlabeled files using >> defcontext= option. This overrides the value set for unlabeled files in >> the policy and requires a filesystem that supports xattr labeling." >> >> "Unlabeled files" could just mean files without any xattr, or it could >> mean all files that either lack an xattr or have an invalid one under >> the policy, since both sets of files are currently mapped to the >> unlabeled context. > > This may be true for the major SELinux policies being shipped by > distributions, but can we say this holds true for *all* SELinux > policies in use today? > >> Second, under what conditions would this situation break existing >> userspace? The admin would have had to mount an xattr filesystem with >> defcontext= specified, and that filesystem would have to both contain >> files without any xattrs and files with invalid ones (otherwise how they >> are treated by the kernel is irrelevant), and the policy would need to >> distinguish access to files without xattrs vs files with invalid ones. >> Seems unlikely. > > I think you answered your own question. Yes, it is unlikely, but I > *really* dislike changing visible behavior like this without some > explicit action on behalf of the user/admin. We've done it in the > past and it has left me uneasy each time. > >> Third, the fact that policy maintainers remapped both SECINITSID_FILE >> (the default value for defcontext) and SECINITSID_UNLABELED (used for >> invalid contexts) to the unlabeled context (getting rid of file_t as a >> separate type, aliased to unlabeled_t) long ago suggests that there is >> no meaningful difference there. > > Related to the comments above. > >> I'm inclined to just change the behavior for defcontext= unconditionally >> and have it apply to both native and xattr labeling. If that's a no-go, >> then the simplest solution is to just leave defcontext= behavior >> unchanged for xattr labeling and only implement the new semantics for >> native labeling. That's just a matter of adding a flag to >> security_context_to_sid_default() and only setting it when calling from >> selinux_inode_notifysecctx(). > > Neither option is very appealing to me, but that doesn't mean I'm saying "no". > > From a sanity and consistency point of view I think option #1 (change > the defcontext behavior) is a better choice, and I tend to favor this > consistency even with the understanding that it could result in some > unexpected behavior for users. However, if we get complaints, I'm > going to revert this without a second thought. In that case, I'd suggest splitting it into two patches; first one only enables the new behavior for native labeling filesystems (as per the above, via a flag to security_context_to_sid_default), and the second patch drops the flag and does it unconditionally. Then you can always revert the latter without affecting the former. > > So to answer your question Taras, go ahead and prepare a patch so we > can take a look. A bit of fair warning that it might get delayed > until after the upcoming merge window since we are already at -rc5; I > want this to have plenty of time in -next. > > Thanks guys. >