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

[Paul Moore]

On Wed, Oct 21, 2020 at 8:07 AM Mark Salyzyn <salyzyn@xxxxxxxxxxx> wrote:
> 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.

Thanks for the background!

-- 
paul moore
www.paul-moore.com