* [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE
@ 2016-09-09 15:37 Vivek Goyal
2016-09-19 18:00 ` Paul Moore
0 siblings, 1 reply; 3+ messages in thread
From: Vivek Goyal @ 2016-09-09 15:37 UTC (permalink / raw)
To: linux-security-module
Cc: Stephen Smalley, Paul Moore, Daniel J Walsh, linux kernel mailing list
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@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(-)
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) {
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE
2016-09-09 15:37 [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE Vivek Goyal
@ 2016-09-19 18:00 ` Paul Moore
2016-09-20 20:49 ` Paul Moore
0 siblings, 1 reply; 3+ messages in thread
From: Paul Moore @ 2016-09-19 18:00 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-security-module, Stephen Smalley, Paul Moore,
Daniel J Walsh, linux kernel mailing list
On Fri, Sep 9, 2016 at 11:37 AM, Vivek Goyal <vgoyal@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 <vgoyal@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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE
2016-09-19 18:00 ` Paul Moore
@ 2016-09-20 20:49 ` Paul Moore
0 siblings, 0 replies; 3+ messages in thread
From: Paul Moore @ 2016-09-20 20:49 UTC (permalink / raw)
To: Vivek Goyal
Cc: linux-security-module, Stephen Smalley, Paul Moore,
Daniel J Walsh, linux kernel mailing list
On Mon, Sep 19, 2016 at 2:00 PM, Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Sep 9, 2016 at 11:37 AM, Vivek Goyal <vgoyal@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 <vgoyal@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.
FYI, the testing went well so I just sent another pull request to
James for v4.9.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-09-20 20:49 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 15:37 [PATCH] lsm,audit,selinux: Introduce a new audit data type LSM_AUDIT_DATA_FILE Vivek Goyal
2016-09-19 18:00 ` Paul Moore
2016-09-20 20:49 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.