On Fri, Sep 9, 2016 at 11:37 AM, Vivek Goyal <vgo...@redhat.com> wrote: > Hi, > > Right now LSM_AUDIT_DATA_PATH type contains "struct path" in union "u" of > common_audit_data. This information is used to print path of file > at the same time it is also used to get to dentry and inode. And this > inode information is used to get to superblock and device and print > device information. > > This does not work well for layered filesystems like overlay where dentry > contained in path is overlay dentry and not the real dentry of underlying > file system. That means inode retrieved from dentry is also overlay > inode and not the real inode. > > seliux helpers like file_path_has_perm() are doing checks on inode retrieved > from file_inode(). This returns the real inode and not the overlay inode. > That means we are doing check on real inode but for audit purposes we > are printing details of overlay inode and that can be confusing while > debugging. > > Hence, introduce a new type LSM_AUDIT_DATA_FILE which carries file > information and inode retrieved is real inode using file_inode(). That > way right avc denied information is given to user. > > For example, following is one example avc before the patch. > > type=AVC msg=audit(1473360868.399:214): avc: denied { read open } for > pid=1765 comm="cat" > path="/root/git/selinux-testsuite/tests/overlay/container1/merged/readfile" > dev="overlay" ino=21443 > scontext=unconfined_u:unconfined_r:test_overlay_client_t:s0:c10,c20 > tcontext=unconfined_u:object_r:test_overlay_files_ro_t:s0 tclass=file > permissive=0 > > It looks as follows after the patch. > > type=AVC msg=audit(1473360017.388:282): avc: denied { read open } for > pid=2530 comm="cat" > path="/root/git/selinux-testsuite/tests/overlay/container1/merged/readfile" > dev="dm-0" ino=2377915 > scontext=unconfined_u:unconfined_r:test_overlay_client_t:s0:c10,c20 > tcontext=unconfined_u:object_r:test_overlay_files_ro_t:s0 tclass=file > permissive=0 > > Notice that now dev information points to "dm-0" device instead of "overlay" > device. This makes it clear that check failed on underlying inode and not > on the overlay inode. > > Signed-off-by: Vivek Goyal <vgo...@redhat.com> > --- > include/linux/lsm_audit.h | 2 ++ > security/lsm_audit.c | 13 +++++++++++++ > security/selinux/hooks.c | 16 ++++++++-------- > 3 files changed, 23 insertions(+), 8 deletions(-)
Hi Vivek, Sorry for the delay, this fell through the my normal filters as it didn't go to the SELinux and/or audit mailing list. However, this patch looks reasonable to me and I think it is something we want in 4.9 with the rest of the LSM/overlayfs bits. I'm building a test kernel right now and assuming all goes well, I'll push this up to James since it looks like we've got another week before the merge window opens. -Paul > Index: pcmoore-linux/include/linux/lsm_audit.h > =================================================================== > --- pcmoore-linux.orig/include/linux/lsm_audit.h 2016-09-08 > 14:56:10.173159741 -0400 > +++ pcmoore-linux/include/linux/lsm_audit.h 2016-09-08 14:56:14.066159741 > -0400 > @@ -59,6 +59,7 @@ struct common_audit_data { > #define LSM_AUDIT_DATA_INODE 9 > #define LSM_AUDIT_DATA_DENTRY 10 > #define LSM_AUDIT_DATA_IOCTL_OP 11 > +#define LSM_AUDIT_DATA_FILE 12 > union { > struct path path; > struct dentry *dentry; > @@ -75,6 +76,7 @@ struct common_audit_data { > #endif > char *kmod_name; > struct lsm_ioctlop_audit *op; > + struct file *file; > } u; > /* this union contains LSM specific data */ > union { > Index: pcmoore-linux/security/lsm_audit.c > =================================================================== > --- pcmoore-linux.orig/security/lsm_audit.c 2016-09-08 14:56:10.173159741 > -0400 > +++ pcmoore-linux/security/lsm_audit.c 2016-09-08 14:56:14.067159741 -0400 > @@ -245,6 +245,19 @@ static void dump_common_audit_data(struc > } > break; > } > + case LSM_AUDIT_DATA_FILE: { > + struct inode *inode; > + > + audit_log_d_path(ab, " path=", &a->u.file->f_path); > + > + inode = file_inode(a->u.file); > + if (inode) { > + audit_log_format(ab, " dev="); > + audit_log_untrustedstring(ab, inode->i_sb->s_id); > + audit_log_format(ab, " ino=%lu", inode->i_ino); > + } > + break; > + } > case LSM_AUDIT_DATA_IOCTL_OP: { > struct inode *inode; > > Index: pcmoore-linux/security/selinux/hooks.c > =================================================================== > --- pcmoore-linux.orig/security/selinux/hooks.c 2016-09-08 14:56:10.173159741 > -0400 > +++ pcmoore-linux/security/selinux/hooks.c 2016-09-09 09:15:37.001159741 > -0400 > @@ -1761,8 +1761,8 @@ static inline int file_path_has_perm(con > { > struct common_audit_data ad; > > - ad.type = LSM_AUDIT_DATA_PATH; > - ad.u.path = file->f_path; > + ad.type = LSM_AUDIT_DATA_FILE; > + ad.u.file = file; > return inode_has_perm(cred, file_inode(file), av, &ad); > } > > @@ -1784,8 +1784,8 @@ static int file_has_perm(const struct cr > u32 sid = cred_sid(cred); > int rc; > > - ad.type = LSM_AUDIT_DATA_PATH; > - ad.u.path = file->f_path; > + ad.type = LSM_AUDIT_DATA_FILE; > + ad.u.file = file; > > if (sid != fsec->sid) { > rc = avc_has_perm(sid, fsec->sid, > @@ -2365,8 +2365,8 @@ static int selinux_bprm_set_creds(struct > new_tsec->sid = old_tsec->sid; > } > > - ad.type = LSM_AUDIT_DATA_PATH; > - ad.u.path = bprm->file->f_path; > + ad.type = LSM_AUDIT_DATA_FILE; > + ad.u.file = bprm->file; > > if (new_tsec->sid == old_tsec->sid) { > rc = avc_has_perm(old_tsec->sid, isec->sid, > @@ -3833,8 +3833,8 @@ static int selinux_kernel_module_from_fi > > /* finit_module */ > > - ad.type = LSM_AUDIT_DATA_PATH; > - ad.u.path = file->f_path; > + ad.type = LSM_AUDIT_DATA_FILE; > + ad.u.file = file; > > fsec = file->f_security; > if (sid != fsec->sid) { -- paul moore www.paul-moore.com