Re: [PATCH v10 3/5] overlayfs: add __get xattr method

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7/24/19 10:48 PM, Amir Goldstein wrote:
On Wed, Jul 24, 2019 at 10:57 PM Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
Because of the overlayfs getxattr recursion, the incoming inode fails
to update the selinux sid resulting in avc denials being reported
against a target context of u:object_r:unlabeled:s0.
This description is too brief for me to understand the root problem.
What's wring with the overlayfs getxattr recursion w.r.t the selinux
security model?

__vfs_getxattr (the way the security layer acquires the target sid without recursing back to security to check the access permissions) calls get xattr method, which in overlayfs calls vfs_getxattr on the lower layer (which then recurses back to security to check permissions) and reports back -EACCES if there was a denial (which is OK) and _no_ sid copied to caller's inode security data, bubbles back to the security layer caller, which reports an invalid avc: message for u:object_r:unlabeled:s0 (the uninitialized sid instead of the sid for the lower filesystem target). The blocked access is 100% valid, it is supposed to be blocked. This does however result in a cosmetic issue that makes it impossible to use audit2allow to construct a rule that would be usable to fix the access problem.

This problem would exist for any (in tree or out-of-tree) union filesystems that need to reflect the __vfs_getxattr call into a __vfs_getxattr call to the underlying filesystem.


Please give an example of your unprivileged mounter use case
to explain.

The mounter merely does not have access to the targets in one of the referenced filesystems (for override creds = on)

In Android would be init, it does not have access to a subset of u:object_r:*_exec:s0 and u::objects_r:vendor*:s0 labels. Based on a need to access basis.

This gets worse if we add override creds = off, because the multitude of callers could be denied access, and rightfully so, and we would have no clue how to construct rules to grant them permissions using standard tools like audit2allow.

I will figure out how to explain this in the commit message ... but do tell me if I did not 'connect' you to the underlying problem so I can make it clear to _everyone_.


CC Vivek because I could really never understand all this.

Solution is to add a _get xattr method that calls the __vfs_getxattr
handler so that the context can be read in, rather than being denied
with an -EACCES when vfs_getxattr handler is called.
. . .
@@ -972,6 +989,7 @@ static const struct xattr_handler ovl_own_xattr_handler = {
  static const struct xattr_handler ovl_other_xattr_handler = {
         .prefix = "", /* catch all */
         .get = ovl_other_xattr_get,
+       .__get = __ovl_other_xattr_get,
         .set = ovl_other_xattr_set,
  };


Not very professional of me to comment on the proposed solution
without understanding the problem, but my nose says this cannot
be the right solution and if it is, then you better find a much better
name for the API then __get() and document it properly.

Yes __get (instead of the existing get which checks sepolicy) was my idea. get_wo_security was a close alternative.

We worked through 5 "solutions", this one privately appeared to please the security folks. In fact the solution was their suggestion because they noticed that __vfs_getxattr was meant to bypass sepolicy checking, yet when nested through overlayfs, it called vfs_getxattr for the lower filesystems and blocked necessary content (sid actually) from the upper call in order to properly log the denial. I had originally copied just the sid to the upper caller by adding layering violations that creeped them out ;-/




[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux