http://bugzilla.suse.com/show_bug.cgi?id=1089349 Fabian Vogt changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |1089315 -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c1 --- Comment #1 from Takashi Iwai --- The patch looks reasonable to me, but I'd also leave to persons who have a better clue. Goldwyn? -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 Goldwyn Rodrigues changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|kernel-maintainers@forge.pr |rgoldwyn@suse.com |ovo.novell.com | -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c4 --- Comment #4 from Goldwyn Rodrigues --- On second thoughts, this is a security risk. If ACL is not be copied, the access permissions will change over an overlayfs mount. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c6 Goldwyn Rodrigues changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nfbrown@suse.com --- Comment #6 from Goldwyn Rodrigues --- (In reply to Fabian Vogt from comment #5)

(In reply to Goldwyn Rodrigues from comment #4)

...

On second thoughts, this is a security risk.

The handling for security_inode_copy_up_xattr is the same.

Not quite. The security_inode_copy_up_xattr copies the labels which are used by selinux to impose security restrictions. It just skips selinux (the only user) specific xattr.

...

If ACL is not be copied, the access permissions will change over an overlayfs mount.

ACLs are handled separately AFAICT. The only reason system.nfs4_acl exists as xattr is to provide userspace with the extended information NFSv4 ACLs provide over POSIX ACLs.

However, I'd say that this is a configuration issue by the system administrator - if the upper layer doesn't support a feature, it must not be relied on.

I don't think there's a better way to handle this, but I'd like to be proven otherwise.

There is already a discussion on this. https://www.spinics.net/lists/linux-nfs/msg61045.html Workaround is to use the exported FS without ACL. Adding Neil for inputs on NFS4 client. Neil: Can/Should empty nfs4_acls be suppressed? -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 Goldwyn Rodrigues changed: What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(nfbrown@suse.com) -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c9 Fabian Vogt changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #766955|0 |1 is obsolete| | --- Comment #9 from Fabian Vogt --- Created attachment 767216 --> http://bugzilla.suse.com/attachment.cgi?id=767216&action=edit 0001-ovl-Allow-copying-of-files-to-the-upper-layer-from-c.patch (In reply to Neil Brown from comment #8)

It is safe for overlayfs to ignore the system.nfs4_acl attribute (which is not empty btw). It is informational only. I would be a bit more comfortable with the patch if it was conditional on the attribute name starting "system."

Ok, I added that. Still works fine here.

and/or if it was upstream.

That's what this bug report is about :-)

There should be no security risk that - falling back on the mode bits for access checks should always be more restrictive (I hope).

Currently overlayfs has the following behaviour on NFSv4:

listxattr foo system.nfs4_acl touch foo listxattr foo (empty)

That might be unexpected, but is essentially unavoidable. It would be possible (but the implementation wouldn't be pretty I guess) to never provide the nfs4_acl attr through overlayfs at all to avoid this inconsistency, at the expense of losing the information completely instead of making it volatile. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c11 Goldwyn Rodrigues changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(rgoldwyn@suse.com | |) | --- Comment #11 from Goldwyn Rodrigues --- (In reply to Fabian Vogt from comment #10)

Any news here? Patch got submitted, but AFAICT didn't land.

I followed up. However, Miklos says it would be better if we can suppress system.nfs4_acl if it is equal to inode->i_mode. However, nfs4_acl seems to be opaque to the client and is interpreted by knfsd only.

From what I read now, ignoring "system." does pose a security risk. A file which is allowed read for a user from a system.posix_acl_access or system.nfs4_acl will become unreadable after a copy_up operation and vice versa.

Let me look further how we can hide system.nfs4_acl -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1089349 http://bugzilla.suse.com/show_bug.cgi?id=1089349#c13 --- Comment #13 from Goldwyn Rodrigues --- (In reply to Fabian Vogt from comment #12)

(In reply to Goldwyn Rodrigues from comment #11)

...

(In reply to Fabian Vogt from comment #10)

...

Any news here? Patch got submitted, but AFAICT didn't land.

I followed up. However, Miklos says it would be better if we can suppress system.nfs4_acl if it is equal to inode->i_mode. However, nfs4_acl seems to be opaque to the client and is interpreted by knfsd only.

From what I read now, ignoring "system." does pose a security risk.

AFAICT, no. It's the same security risk as copying a file to a different file system.

Here is proof with your patch applied. The goldwyn(1000) and blossom (1001) are two users. The NFS4 ACLs: goldwyn@nfsacl:/nfs/tmp> nfs4_getfacl aa.txt A::OWNER@:rwatTcCy D::1000:r A::1000:tcy D::1001:r A::1001:tcy A::GROUP@:rtcy A::EVERYONE@:rtcy goldwyn@nfsacl:/nfs/tmp> nfs4_getfacl b.txt A::OWNER@:rwatTcCy A::GROUP@:rtcy A:g:458:rtcy A::EVERYONE@:rtcy nfsacl:~ # mount /dev/vdb /upper nfsacl:~ # mount -t overlay overlay -o lowerdir=/nfs,upperdir=/upper/upper,workdir=/upper/work /mnt/ goldwyn@nfsacl:/mnt/tmp> ls aa.txt b.txt goldwyn@nfsacl:/mnt/tmp> cat aa.txt aa goldwyn@nfsacl:/mnt/tmp> cat b.txt bb For another user: blossom@nfsacl:/mnt/tmp> cat aa.txt cat: aa.txt: Permission denied After first user edits: goldwyn@nfsacl:/mnt/tmp> echo abc >> aa.txt Other user can now read the file because a copy_up happened: blossom@nfsacl:/mnt/tmp> cat aa.txt aa abc

overlayfs can only be as secure as the least common denominator of upper and lower layers. So I argue that by mounting it, the admin made a conscious decision.

This policy may not hold for a paranoid admin ;) I understand that just using acl as the mount option is barring all files copy_up. Trying to find a better approach than allowing everything. -- You are receiving this mail because: You are on the CC list for the bug.

https://bugzilla.suse.com/show_bug.cgi?id=1089349 https://bugzilla.suse.com/show_bug.cgi?id=1089349#c15 Fabian Vogt changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(fvogt@suse.com) |needinfo?(ddiss@suse.com), | |needinfo?(rgoldwyn@suse.com | |) --- Comment #15 from Fabian Vogt --- (In reply to David Disseldorp from comment #14)

It looks as though this has been addressed in mainline via c61ca5572508516b41039aecb23d936466076950 (v5.7+), which additionally adds a XATTR_POSIX_ACL_ACCESS/XATTR_POSIX_ACL_DEFAULT/XATTR_SECURITY_PREFIX check before ignoring vfs_setxattr -EOPNOTSUPP errors.

@Fabian: can this now be closed?

Great, that's essentially what my patch does, so I assume it works as well. What about Goldwyn's concerns with this approach? -- You are receiving this mail because: You are on the CC list for the bug.

https://bugzilla.suse.com/show_bug.cgi?id=1089349 https://bugzilla.suse.com/show_bug.cgi?id=1089349#c17 David Disseldorp changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |RESOLVED Resolution|--- |FIXED Flags|needinfo?(ddiss@suse.com), | |needinfo?(rgoldwyn@suse.com | |) | --- Comment #17 from David Disseldorp --- (In reply to Goldwyn Rodrigues from comment #16)

(In reply to Fabian Vogt from comment #15)

...

Great, that's essentially what my patch does, so I assume it works as well.

What about Goldwyn's concerns with this approach?

I suppose if it is good enough for upstream inclusion, it is good enough for me.

Thanks Goldwyn. Closing... -- You are receiving this mail because: You are on the CC list for the bug.