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.
