On 10/20/20 6:17 PM, Paul Moore wrote:
> On Tue, Oct 20, 2020 at 3:17 PM Mark Salyzyn <[email protected]> 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 <[email protected]>
>> Reviewed-by: Jan Kara <[email protected]>
>> Acked-by: Jan Kara <[email protected]>
>> Acked-by: Jeff Layton <[email protected]>
>> Acked-by: David Sterba <[email protected]>
>> Acked-by: Darrick J. Wong <[email protected]>
>> Acked-by: Mike Marshall <[email protected]>
>> To: [email protected]
>> To: [email protected]
>> Cc: Stephen Smalley <[email protected]>
>> Cc: [email protected]
>> Cc: [email protected]
>> Cc: [email protected]
> ...
>
>> <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.
On Wed, Oct 21, 2020 at 8:07 AM Mark Salyzyn <[email protected]> wrote:
> On 10/20/20 6:17 PM, Paul Moore wrote:
> > On Tue, Oct 20, 2020 at 3:17 PM Mark Salyzyn <[email protected]> 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 <[email protected]>
> >> Reviewed-by: Jan Kara <[email protected]>
> >> Acked-by: Jan Kara <[email protected]>
> >> Acked-by: Jeff Layton <[email protected]>
> >> Acked-by: David Sterba <[email protected]>
> >> Acked-by: Darrick J. Wong <[email protected]>
> >> Acked-by: Mike Marshall <[email protected]>
> >> To: [email protected]
> >> To: [email protected]
> >> Cc: Stephen Smalley <[email protected]>
> >> Cc: [email protected]
> >> Cc: [email protected]
> >> Cc: [email protected]
> > ...
> >
> >> <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
http://www.paul-moore.com