NACK: [X] [CVE-2015-1350] [PATCH] fs: Avoid premature clearing of capabilities
Stefan Bader
stefan.bader at canonical.com
Mon Aug 20 13:13:59 UTC 2018
On 20.08.2018 08:58, Kleber Souza wrote:
> On 08/16/18 13:41, José Pekkarinen wrote:
>> CVE-2015-1350
>>
>> Buglink: https://launchpad.net/bugs/1415636
>>
>> Currently, notify_change() clears capabilities or IMA attributes by
>> calling security_inode_killpriv() before calling into ->setattr. Thus it
>> happens before any other permission checks in inode_change_ok() and user
>> is thus allowed to trigger clearing of capabilities or IMA attributes
>> for any file he can look up e.g. by calling chown for that file. This is
>> unexpected and can lead to user DoSing a system.
>>
>> Fix the problem by calling security_inode_killpriv() at the end of
>> inode_change_ok() instead of from notify_change(). At that moment we are
>> sure user has permissions to do the requested change.
>>
>
> As Stefan mentioned, the complete commit message should be kept. So the
> line below shouldn't be removed.
>
> References: CVE-2015-1350
>> Reviewed-by: Christoph Hellwig <hch at lst.de>
>> Signed-off-by: Jan Kara <jack at suse.cz>
>
> As Stefan also mentioned, please add the original SHA1 of the patch
> mentioning if it's a clean cherry pick "(cherry picked from ...)" or a
> backport "(backported from ...)" as
>
> (backported from 030b533c4fd4d2ec3402363323de4bb2983c9cee)
>> Signed-off-by: José Pekkarinen <jose.pekkarinen at canonical.com>
>
> I'm not NAK'ing or ACK'ing the patch, if the patch is fine we can
> include those fixes when applying it if others agree as well.
>
>
> Thanks,
> Kleber
>
>>
>> ---
>> fs/attr.c | 23 +++++++++++++++++------
>> 1 file changed, 17 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/attr.c b/fs/attr.c
>> index df05bc1..870e4b6 100644
>> --- a/fs/attr.c
>> +++ b/fs/attr.c
>> @@ -68,7 +68,7 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr)
>>
>> /* If force is set do it anyway. */
>> if (ia_valid & ATTR_FORCE)
>> - return 0;
>> + goto kill_priv;
>>
>> /* Make sure a caller can chown. */
>> if ((ia_valid & ATTR_UID) && !chown_ok(inode, attr->ia_uid))
>> @@ -95,6 +95,19 @@ int inode_change_ok(const struct inode *inode, struct iattr *attr)
>> return -EPERM;
>> }
>>
>> +kill_priv:
>> + /* User has permission for the change */
>> + if (ia_valid & ATTR_KILL_PRIV) {
>> + int error;
>> + struct dentry * dentry = d_find_alias(inode);
>> +
>> + if (dentry) {
>> + error = security_inode_killpriv(dentry);
>> + if (error)
>> + return error;
>> + }
>> + }
>> +
>> return 0;
Additionally to above reasons, this is basically a submission Khaled has made in
the past and Theadeu had proposed the use of d_obtain_alias, but _also_ said a
matching dput() would be needed. Plus, if one really reads the code, it is
obvious that the return value of d_obtain_alias() is nevel NULL, hence that part
should look like in Khaled's v3 submission:
+kill_priv:
+ /* User has permission for the change */
+ if (ia_valid & ATTR_KILL_PRIV) {
+ int error;
+ struct dentry *dentry;
+
+ dentry = d_obtain_alias(inode);
+ if (!IS_ERR(dentry)) {
+ error = security_inode_killpriv(dentry);
+ dput(dentry);
+ return error;
+ }
+ }
+
-Stefan
PS: Same reply for the Trusty patch which could be made a single patch
submission with the note that the first hunk needs to accept a fuzz 1.
>> }
>> EXPORT_SYMBOL(inode_change_ok);
>> @@ -250,13 +263,11 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
>> if (!(ia_valid & ATTR_MTIME_SET))
>> attr->ia_mtime = now;
>> if (ia_valid & ATTR_KILL_PRIV) {
>> - attr->ia_valid &= ~ATTR_KILL_PRIV;
>> - ia_valid &= ~ATTR_KILL_PRIV;
>> error = security_inode_need_killpriv(dentry);
>> - if (error > 0)
>> - error = security_inode_killpriv(dentry);
>> - if (error)
>> + if (error < 0)
>> return error;
>> + if (error == 0)
>> + ia_valid = attr->ia_valid &= ~ATTR_KILL_PRIV;
>> }
>>
>> /*
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20180820/3bd7d4ae/attachment.sig>
More information about the kernel-team
mailing list