Re: [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE

From: Paul Moore
Date: Mon Sep 19 2016 - 14:00:39 EST

On Fri, Sep 9, 2016 at 11:37 AM, Vivek Goyal <vgoyal@xxxxxxxxxx> 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 <vgoyal@xxxxxxxxxx>
> ---
> 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