linux-audit / audit-kernel

GitHub mirror of the Linux Kernel's audit repository
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
Other
137 stars 36 forks source link

RFE: add CWD for file-related LSM_AUDIT_DATA_* #96

Closed rgbriggs closed 3 years ago

rgbriggs commented 6 years ago

The following dump_common_audit_data cases could use a CWD record: LSM_AUDIT_DATA_PATH LSM_AUDIT_DATA_FILE LSM_AUDIT_DATA_IOCTL_OP LSM_AUDIT_DATA_DENTRY LSM_AUDIT_DATA_INODE

This should be as simple as calling get_fs_pwd() for these cases.

Another suggestion that was made was to implement an AVC_PATH record.

See also: https://github.com/linux-audit/audit-kernel/issues/65

nefigtut commented 4 years ago

The above is a naive straight-forward fix for this issue which just adds "cwd=" field into the "type=AVC" audit record.

I'm not sure if "use a CWD record" above means a separate audit "type=CWD" record. If yes, there will be more work, as "struct audit_context" and friends should be exposed to "security/lsm_audit.c" (now they are internal to "kernel/audit.c") and some analogue of audit_log_exit() should be created for the LSM case (or audit_log_exit() itself exposed and used).

rgbriggs commented 4 years ago

On 2020-03-24 12:04, Vladis Dronov wrote:

The above is a naive straight-forward fix for this issue which just adds "cwd=" field into the "type=AVC" audit record.

I'm not sure if "use a CWD record" above means a separate audit "type=CWD" record. If yes, there will be more work, as "struct audit_context" and friends should be exposed to "security/lsm_audit.c". Now they are internal to "kernel/audit.c".

The intent was a separate CWD record, that is already defined. There should be no reason to change the existing exposure of struct audit_context and just use audit_context() like every other non-audit subsystem.

There is an example in __audit_getname() and it might make sense to create a audit internal function to address this such as:

void audit_cwd(void) { struct audit_context *context = audit_context();

if (!context->pwd.dentry)
    get_fs_pwd(current->fs, &context->pwd);

}

nefigtut commented 4 years ago

Hm. This way I do not see a simple solution, unfortunately. It looks like we would need to tweak internal API. This is how I see it. Yes, we can export audit_cwd() and call it in the LSM code. But what would emit this CWD record?

For syscalls it is done by audit_log_exit() which emits a number of records (including the CWD one) at the end of syscall processing. But it is internal (static) to auditsc.c and has syscall-only code, so we cannot call it from the LSM code. As I see it now, we could:

1) Write an new audit_log_lsm_exit() which has the same code as audit_log_exit() for emitting CWD records, has lsm-only code and is internal to LSM. So it is called at the end of AVC record processing. This forces us to expose struct audit_context as audit_lsm_log_exit() must be able to access audit context fields. Either (which I like more):

2) We can move common code to audit_log_exit_common(), expose it, make audit_log_syscall_exit() and audit_log_lsm_exit() call it, and call audit_log_lsm_exit() at the end of AVC record processing.

Would you like to see any of these approaches implemented? Or some 3rd one?

rgbriggs commented 4 years ago

On 2020-03-25 05:28, Vladis Dronov wrote:

Hm. This way I do not see simple solution, unfortunately, it looks like we would need to tweak internal API. This is how I see it. Yes, we can export audit_cwd() and call it in the LSM code. But what would emit this CWD record?

The syscall exit would emit that CWD record.

For syscalls it is done by audit_log_exit() which emits a number of records (including the CWD one) at the end of syscall processing. But it is internal (static) to auditsc.c and has syscall-only code, so we cannot call it from the LSM code. As I see it now, we could:

No, you can't and don't need to. When any record is triggered, such as a mandatory audit config change record or other required record, the associated syscall record and all the rest of its accompanying records will be issued on syscall exit.

nefigtut commented 4 years ago

1) audit context is always NULL if "-a never,task" rule is active, which is default in Fedora. so CWD record won't be emitted.

2) ghak#120 fix is necessary pre-req as it enables audit context in case when no audit rules are active. with that the resulting fix is rather straight-forward. 3) testing:

audit context is NULL case (no CWD record):

# uname -r
5.6.0-rc7-vsdau << (with ghak#120 fix and this fix)
# auditctl -l
-a never,task
# chcon system_u:system_r:gpm_t:s0 aaa
chcon: failed to change context of 'aaa' to ‘system_u:system_r:gpm_t:s0’: Permission denied
...
type=AVC msg=audit(1585244338.716:128): avc:  denied  { relabelfrom } for  pid=562 comm="chcon" name="aaa" dev="vda3" ino=33631336 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:system_r:gpm_t:s0 tclass=file permissive=0

# runcon system_u:system_r:gpm_t:s0 cat autest.c 
runcon: ‘cat’: Permission denied
...
type=AVC msg=audit(1585244441.260:129): avc:  denied  { entrypoint } for  pid=565 comm="runcon" path="/usr/bin/cat" dev="vda3" ino=50497724 scontext=system_u:system_r:gpm_t:s0 tcontext=system_u:object_r:bin_t:s0 tclass=file permissive=0

no audit rules case:

# auditctl -D
No rules
# chcon system_u:system_r:gpm_t:s0 aaa
chcon: failed to change context of 'aaa' to ‘system_u:system_r:gpm_t:s0’: Permission denied
...
type=AVC msg=audit(1585244507.504:131): avc:  denied  { relabelfrom } for  pid=570 comm="chcon" name="aaa" dev="vda3" ino=33631336 scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 tcontext=system_u:system_r:gpm_t:s0 tclass=file permissive=0
type=SYSCALL msg=audit(1585244507.504:131): arch=c000003e syscall=188 success=no exit=-13 a0=5597206297a0 a1=7f403a71e726 a2=55972062ac10 a3=1b items=0 ppid=511 pid=570 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="chcon" exe="/usr/bin/chcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=setxattr AUID="root" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
type=CWD msg=audit(1585244507.504:131): cwd="/root"
type=PROCTITLE msg=audit(1585244507.504:131): proctitle=6368636F6E0073797374656D5F753A73797374656D5F723A67706D5F743A733000616161

# runcon system_u:system_r:gpm_t:s0 cat autest.c 
runcon: ‘cat’: Permission denied
...
type=AVC msg=audit(1585244604.168:132): avc:  denied  { entrypoint } for  pid=571 comm="runcon" path="/usr/bin/cat" dev="vda3" ino=50497724 scontext=system_u:system_r:gpm_t:s0 tcontext=system_u:object_r:bin_t:s0 tclass=file permissive=0
type=SYSCALL msg=audit(1585244604.168:132): arch=c000003e syscall=59 success=no exit=-13 a0=7fff6574cc00 a1=7fff6574ce38 a2=7fff6574ce50 a3=0 items=0 ppid=511 pid=571 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=3 comm="runcon" exe="/usr/bin/runcon" subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=(null)ARCH=x86_64 SYSCALL=execve AUID="root" UID="root" GID="root" EUID="root" SUID="root" FSUID="root" EGID="root" SGID="root" FSGID="root"
type=CWD msg=audit(1585244604.168:132): cwd="/root"
type=PROCTITLE msg=audit(1585244604.168:132): proctitle=72756E636F6E0073797374656D5F753A73797374656D5F723A67706D5F743A733000636174006175746573742E63
pcmoore commented 4 years ago

Just as a FYI, patches really should be sent to the relevant kernel mailing lists for discussion.

nefigtut commented 4 years ago

yes, my intent was to have some peer-to-peer or mentor review (by Steve or Richard or someone) of a suggested patch before sending it to a mailing list, as definitely I know little in the audit area as of now. if this is unwelcomed behaviour I'll surely stop.

pcmoore commented 4 years ago

If you send the patch to the mailing list, perhaps with a note that this is just for informal review and not consideration for inclusion into the kernel (yet), then others can more easily benefit from the lessons learned. The number of people following an individual issue tracker is almost always going to be smaller than the number of people following the mailing list.

nefigtut commented 4 years ago

thanks, Paul! great, let me try a wider audience: https://lore.kernel.org/linux-audit/20200402141319.28714-1-vdronov@redhat.com/T/#u

rgbriggs commented 4 years ago

Post v3 https://www.redhat.com/archives/linux-audit/2020-July/msg00007.html https://lkml.org/lkml/2020/7/3/666

rgbriggs commented 4 years ago

2020-07-18: NULL pointer dereference reported due to audit: trigger accompanying records when no rules present for ghak120 : https://lore.kernel.org/linux-audit/20200723200041.7yinlklts47pzmfm@madcap2.tricolour.ca/T/#m8a349df90d56faaa5c28e9ddf4081b05115f5ca8 It was inadvertantly fixed by v3

2020-07-27: Post addendum: https://www.redhat.com/archives/linux-audit/2020-July/msg00132.html https://lkml.org/lkml/2020/7/27/1692

rgbriggs commented 4 years ago

Replacement addendum: https://www.redhat.com/archives/linux-audit/2020-September/msg00041.html

rgbriggs commented 3 years ago

This ghak120 v5 patch effectively reverts all previous patches on this issue: https://www.redhat.com/archives/linux-audit/2020-September/msg00052.html https://lkml.org/lkml/2020/9/22/481 https://patchwork.kernel.org/patch/11792463/

rgbriggs commented 3 years ago

This can be closed since this patch went upstream in v5.9-rc1 d7481b24b816 ("audit: issue CWD record to accompany LSM_AUDITDATA* records")

And further, it ended up being tangled with https://github.com/linux-audit/audit-kernel/issues/120 with this latter's final patch effectively reverting the patch above. In any case, this issue is resolved. ghak120's patches follow:

v5.7-rc1 1320a4052ea1 ("audit: trigger accompanying records when no rules present") v5.8 8ac68dc455d9 ("revert: 1320a4052ea1 ("audit: trigger accompanying records when no rules present")") audit/next 6d915476e67d ("audit: trigger accompanying records when no rules present")