Re: [PATCH v17 1/4] Add flags option to get xattr method paired to __vfs_getxattr

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

 



On 10/20/20 6:17 PM, Paul Moore wrote:
On Tue, Oct 20, 2020 at 3:17 PM Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
Add a flag option to get xattr method that could have a bit flag of
XATTR_NOSECURITY passed to it.  XATTR_NOSECURITY is generally then
set in the __vfs_getxattr path when called by security
infrastructure.

This handles the case of a union filesystem driver that is being
requested by the security layer to report back the xattr data.

For the use case where access is to be blocked by the security layer.

The path then could be security(dentry) ->
__vfs_getxattr(dentry...XATTR_NOSECURITY) ->
handler->get(dentry...XATTR_NOSECURITY) ->
__vfs_getxattr(lower_dentry...XATTR_NOSECURITY) ->
lower_handler->get(lower_dentry...XATTR_NOSECURITY)
which would report back through the chain data and success as
expected, the logging security layer at the top would have the
data to determine the access permissions and report back the target
context that was blocked.

Without the get handler flag, the path on a union filesystem would be
the errant security(dentry) -> __vfs_getxattr(dentry) ->
handler->get(dentry) -> vfs_getxattr(lower_dentry) -> nested ->
security(lower_dentry, log off) -> lower_handler->get(lower_dentry)
which would report back through the chain no data, and -EACCES.

For selinux for both cases, this would translate to a correctly
determined blocked access. In the first case with this change a correct avc
log would be reported, in the second legacy case an incorrect avc log
would be reported against an uninitialized u:object_r:unlabeled:s0
context making the logs cosmetically useless for audit2allow.

This patch series is inert and is the wide-spread addition of the
flags option for xattr functions, and a replacement of __vfs_getxattr
with __vfs_getxattr(...XATTR_NOSECURITY).

Signed-off-by: Mark Salyzyn <salyzyn@xxxxxxxxxxx>
Reviewed-by: Jan Kara <jack@xxxxxxx>
Acked-by: Jan Kara <jack@xxxxxxx>
Acked-by: Jeff Layton <jlayton@xxxxxxxxxx>
Acked-by: David Sterba <dsterba@xxxxxxxx>
Acked-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
Acked-by: Mike Marshall <hubcap@xxxxxxxxxxxx>
To: linux-fsdevel@xxxxxxxxxxxxxxx
To: linux-unionfs@xxxxxxxxxxxxxxx
Cc: Stephen Smalley <sds@xxxxxxxxxxxxx>
Cc: linux-kernel@xxxxxxxxxxxxxxx
Cc: linux-security-module@xxxxxxxxxxxxxxx
Cc: kernel-team@xxxxxxxxxxx
...

<snip>
[NOTE: added the SELinux list to the CC line]


Thanks and <ooops>


I'm looking at this patchset in earnest for the first time and I'm a
little uncertain about the need for the new XATTR_NOSECURITY flag;
perhaps you can help me understand it better.  Looking over this
patch, and quickly looking at the others in the series, it seems as
though XATTR_NOSECURITY is basically used whenever a filesystem has to
call back into the vfs layer (e.g. overlayfs, ecryptfs, etc).  Am I
understanding that correctly?  If that assumption is correct, I'm not
certain why the new XATTR_NOSECURITY flag is needed; why couldn't
_vfs_getxattr() be used by all of the callers that need to bypass
DAC/MAC with vfs_getxattr() continuing to perform the DAC/MAC checks?
If for some reason _vfs_getxattr() can't be used, would it make more
sense to create a new stripped/special getxattr function for use by
nested filesystems?  Based on the number of revisions to this
patchset, I'm sure it can't be that simple so please educate me :)

It is hard to please everyone :-}

Yes, calling back through the vfs layer.

I was told not to change or remove the __vfs_getxattr default behaviour, but use the flag to pass through the new behavior. Security concerns requiring the _key_ of the flag to be passed through rather than a blanket bypass. This was also the similar security reasoning not to have a special getxattr call.

[TL;DR]

history and details

When it goes down through the layers again, and into the underlying filesystems, to get the getxattr, the xattributes are blocked, then the selinux _context_ will not be copied into the buffer leaving the caller looking at effectively u:r:unknown:s0. Well, they were blocked, so from the security standpoint that part was accurate, but the evaluation of the context is using the wrong rules and an (cosmetically) incorrect avc report. This also poisons the cache layers that may hold on to the context for future calls (+/- bugs) disturbing the future decisions (we saw that in 4.14 and earlier vintage kernels without this patch, later kernels appeared to clear up the cache bug).

The XATTR_NOSECURITY is used in the overlayfs driver for a substantial majority of the calls for getxattr only if the data is private (ie: on the stack, not returned to the caller) as simplification. A _real_ getxattr is performed when the data is returned to the caller. I expect that subtlety will get lost in the passage of time though.

I had a global in_security flag set when selinux was requesting the xattrs to evaluate security context, denied as a security risk since someone could set the global flag. I had a separate special getxattr function in the earlier patches, denied for security issues as well, and others took issue with an additional confusing call site. I added the flag parameter, and that satisfied the security concerns because the value was only temporarily on the stack parameters and could not be attacked to bypass xattr security. This flag passed to __vfs_getxattr was also preferred from the security standpoint so that __vfs_getxattr got the _key_ to bypass the xattr security checks. There was a brief moment where the get_xattr and set_xattr calls shared a similar single argument that pointed to a common call structure, but th as requested by a few, but then denied once it was seen by stakeholders.




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux