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
