linux-kernel - Re: [RFC PATCH RESEND] vfs: Move security_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54BFD090.4000907@schaufler-ca.com>
Date:	Wed, 21 Jan 2015 08:15:12 -0800
From:	Casey Schaufler <casey@...aufler-ca.com>
To:	Stephen Smalley <sds@...ho.nsa.gov>,
	James Morris <jmorris@...ei.org>,
	Ben Hutchings <ben@...adent.org.uk>
CC:	Alexander Viro <viro@...iv.linux.org.uk>,
	linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org,
	LKML <linux-kernel@...r.kernel.org>, 770492@...s.debian.org,
	Ben Harris <bjh21@....ac.uk>, oss-security@...ts.openwall.com,
	John Johansen <john.johansen@...onical.com>,
	Paul Moore <paul@...l-moore.com>,
	Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [RFC PATCH RESEND] vfs: Move security_inode_killpriv() after
 permission checks

On 1/21/2015 6:03 AM, Stephen Smalley wrote:
> On 01/20/2015 06:17 PM, James Morris wrote:
>> On Sat, 17 Jan 2015, Ben Hutchings wrote:
>>
>>> chown() and write() should clear all privilege attributes on
>>> a file - setuid, setgid, setcap and any other extended
>>> privilege attributes.
>>>
>>> However, any attributes beyond setuid and setgid are managed by the
>>> LSM and not directly by the filesystem, so they cannot be set along
>>> with the other attributes.
>>>
>>> Currently we call security_inode_killpriv() in notify_change(),
>>> but in case of a chown() this is too early - we have not called
>>> inode_change_ok() or made any filesystem-specific permission/sanity
>>> checks.
>>>
>>> Add a new function setattr_killpriv() which calls
>>> security_inode_killpriv() if necessary, and change the setattr()
>>> implementation to call this in each filesystem that supports xattrs.
>>> This assumes that extended privilege attributes are always stored in
>>> xattrs.
>> It'd be useful to get some input from LSM module maintainers on this. 
>>
>> e.g. doesn't SELinux already handle this via policy directives?
> There have been a couple postings of a similar patch set [1] by Jan
> Kara, although I don't believe that series addressed chown().
>
> If I am reading the patches correctly, they (correctly) don't affect
> SELinux or Smack labels; they are just calling the existing
> security_inode_killpriv() hook, which is only implemented for the
> capability module to remove the security.capability xattr.

The description of the change should say that. I can easily
imagine an enthusiastic test developer reading the existing
description and filing bugs because SELinux, Smack and whatever
other xattr based systems might be around don't clear their
attributes. If the intent wasn't clear to the first person to
use xattrs for security purposes, I shouldn't expect the new
and inexperienced to see it.

My position softens. Document it correctly, and I'm fine with it.

>
> [1] http://marc.info/?l=linux-security-module&m=141890696325054&w=2
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/