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

[Mark Salyzyn]

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.