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

[Paul Moore]

On Mon, Sep 19, 2016 at 2:00 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> 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.

FYI, the testing went well so I just sent another pull request to
James for v4.9.

-- 
paul moore
www.paul-moore.com