Project

General

Profile

Bug #8984

fix for 6764 breaks ACL inheritance

Added by Paul Henson over 1 year ago. Updated 3 days ago.

Status:
Feedback
Priority:
Normal
Assignee:
-
Category:
-
Start date:
2018-08-10
Due date:
% Done:

0%

Estimated time:
(Total: 0.00 h)
Difficulty:
Medium
Tags:
needs-triage

Description

Consider a directory configured as:

drwx-ws---+ 2 henson cpp 3 Jan 23 12:35 dropbox/
user:henson:rwxpdDaARWcC--:f-i----:allow
owner@:--------------:f-i----:allow
group@:--------------:f-i----:allow
everyone@:--------------:f-i----:allow
owner@:rwxpdDaARWcC--:-di----:allow
group:cpp:-wx-----------:-------:allow
owner@:rwxpdDaARWcC--:-------:allow

A new file created in this directory ends up looking like:

rw-r--r-+ 1 astudent cpp 0 Jan 23 12:39 testfile
user:henson:rw-pdDaARWcC--:------I:allow
owner@:--------------:------I:allow
group@:--------------:------I:allow
everyone@:--------------:------I:allow
owner@:rw-p--aARWcCos:-------:allow
group@:r-----a-R-c--s:-------:allow
everyone@:r-----a-R-c--s:-------:allow

with extraneous group@ and everyone@ entries allowing read access that shouldn't exist.

Per Albert Lee on the zfs mailing list:

"aclinherit=passthrough/passthrough-x should still
ignore the requested mode when an inheritable ACE for owner@ group@,
or everyone@ is present in the parent directory.

It appears there was an oversight in my fix for
https://www.illumos.org/issues/6764 which made calling zfs_acl_chmod
from zfs_acl_inherit unconditional. I think the parent ACL check for
aclinherit=passthrough needs to be reintroduced in zfs_acl_inherit."

We have a large number of faculty who use dropbox directories like the example to have students submit projects. All of these directories are now allowing every student to read what the other students copy in :(, which is a big problem for us.Given this issue allows access that should not be granted, I think it is a security issue.


Subtasks

Bug #9722: Still broken ZFS ACL inheritance from #6764, #8984 only fixed the aclmode=passthrough caseNew

Actions

History

#2

Updated by Prakash Surya over 1 year ago

  • Subject changed from fix for 6764 breaks ACL inheritance to fix for 6764 breaks ACL inheritance
#3

Updated by Electric Monk over 1 year ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit e9bacc6d1a71ea3f7082038b2868de8c4dd98bdc

commit  e9bacc6d1a71ea3f7082038b2868de8c4dd98bdc
Author: Dominik Hassler <hadfl@omniosce.org>
Date:   2018-03-06T17:53:11.000Z

    8984 fix for 6764 breaks ACL inheritance
    Reviewed by: Sam Zaydel <szaydel@racktopsystems.com>
    Reviewed by: Paul B. Henson <henson@acm.org>
    Reviewed by: Prakash Surya <prakash.surya@delphix.com>
    Approved by: Matthew Ahrens <mahrens@delphix.com>

#4

Updated by Andriy Gapon over 1 year ago

A FreeBSD user reports that they still have a related problem.
They say that a similar issue exists for aclinherit=restricted.
Details: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216886#c8

What do you think?

#5

Updated by Paul Henson over 1 year ago

Hmm, I don't use restricted, so I never noticed it. Testing on omnios with the current fix applied:

  1. zfs set aclinherit=restricted export/user/henson
  2. mkdir testdir
  3. chmod A=owner@:rwxpdDaARWc:fi:allow testdir
  4. touch testdir/testfile
  5. ls -V testdir/testfile

rwxr--r-+ 1 root cpp 0 May 4 11:03 testdir/testfile
owner@:rwxpdDaARWc---:------I:allow
owner@:rw-p--aARWcCos:-------:allow
group@:r-----a-R-c--s:-------:allow
everyone@:r-----a-R-c--s:-------:allow

The documentation for restricted says "removes the write_acl and write_owner permissions when the ACE is inherited", it doesn't make any mention of mode, as opposed to the passthrough documentation "files are created with a mode determined by the inheritable ACEs. If no inheritable ACEs exist that affect the mode, then the mode is set in accordance to the requested mode from the application."

I dunno. If it behaves differently now than before 6764 was committed, it should probably be fixed to do what it used to do.

#6

Updated by Paul Henson over 1 year ago

Reviewing the commit for 6764, while there was an explicit check for passthrough removed, I don't see any explicit mention of restricted. It's hard to say from just looking at the diff if something would have changed for that scenario. Does somebody have a box from before that commit available to test it empirically :)?

#7

Updated by Yuri Pankov over 1 year ago

That's why we have zfs-tests' ACL test cases -- those should be updated to check for the behavior we need, not the behavior we have currently.

#8

Updated by Andy Fiddaman over 1 year ago

I fired up an OmniOS r151014 machine in AWS. As far as I can tell, that never had 6764 backported to it.

root@ip-10-152-178-106.ec2.internal:/test# uname -a
SunOS ip-10-152-178-106.ec2.internal 5.11 omnios-b13298f i86pc i386 i86xpv
root@ip-10-152-178-106.ec2.internal:/test# cat /etc/release
  OmniOS v11 r151014
  Copyright 2015 OmniTI Computer Consulting, Inc. All rights reserved.
  Use is subject to license terms.
root@ip-10-152-178-106.ec2.internal:/root# zfs create -o mountpoint=/test syspool/test
root@ip-10-152-178-106.ec2.internal:/root# zfs set aclinherit=restricted syspool/test
root@ip-10-152-178-106.ec2.internal:/root# cd /test
root@ip-10-152-178-106.ec2.internal:/test# mkdir testdir
root@ip-10-152-178-106.ec2.internal:/test# /bin/chmod A=owner@:rwxpdDaARWc:fi:allow testdir
root@ip-10-152-178-106.ec2.internal:/test# touch testdir/testfile

root@ip-10-152-178-106.ec2.internal:/test# /bin/ls -l testdir
total 1
-rw-r--r--   1 root     root           0 May  8 12:07 testfile

root@ip-10-152-178-106.ec2.internal:/test# /bin/ls -V testdir/testfile
-rw-r--r--   1 root     root           0 May  8 12:07 testdir/testfile
                 owner@:rw-p--aARWcCos:-------:allow
                 group@:r-----a-R-c--s:-------:allow
              everyone@:r-----a-R-c--s:-------:allow

So that's a slightly different behaviour to today.

#9

Updated by Harry Schmalzbauer about 1 year ago

Andy Fiddaman wrote:

I fired up an OmniOS r151014 machine in AWS. As far as I can tell, that never had 6764 backported to it.

[...]

So that's a slightly different behaviour to today.

Thanks a lot for verifiying.
At least it's clear that behaviour after #6764 and also after #8984 changed – unintentionally.

So please also bring back the former behaviour also for aclinherit=restricted.
I'm the FreeBSD user who reported the persistent issue Andriy mentioned.

Lots of my real-world setups (with Samba and NFSv4 ACLs) are still on hold for updating because #6764 broke production operation and it's still broken after #8984 for those systems, where I didn't change the default aclinherit=restricted property (on FreeBSD this is the default). These are more than I was aware... I very often have single directories where I set a special inhert ACE; in real world ;-)

Thanks,
-harry

#10

Updated by Harry Schmalzbauer about 1 year ago

Is it possible/wanted to change the status from closed to anything else?
Or shall I file a separate issue?

Thanks,

-harry

#11

Updated by Gordon Ross 3 days ago

  • Status changed from Closed to Feedback

We still have problems here. See also #9722

Also available in: Atom PDF