RFE: add CWD for file-related LSM_AUDIT_DATA_* #96
Comments
|
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). |
|
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);
}
|
|
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:
Would you like to see any of these approaches implemented? Or some 3rd one? |
|
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.
|
audit context is NULL case (no CWD record): no audit rules case: |
|
Just as a FYI, patches really should be sent to the relevant kernel mailing lists for discussion. |
|
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. |
|
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. |
|
thanks, Paul! great, let me try a wider audience: |
Set a current working directory in an audit context for the following record types in dump_common_audit_data(): LSM_AUDIT_DATA_PATH, LSM_AUDIT_DATA_FILE, LSM_AUDIT_DATA_IOCTL_OP, LSM_AUDIT_DATA_DENTRY, LSM_AUDIT_DATA_INODE so a separate CWD record is emitted later. Link: linux-audit/audit-kernel#96 Signed-off-by: Vladis Dronov <vdronov@redhat.com>
The LSM_AUDIT_DATA_* records for PATH, FILE, IOCTL_OP, DENTRY and INODE are incomplete without the task context of the AUDIT Current Working Directory record. Add it. This record addition can't use audit_dummy_context to determine whether or not to store the record information since the LSM_AUDIT_DATA_* records are initiated by various LSMs independent of any audit rules. context->in_syscall is used to determine if it was called in user context like audit_getname. Please see the upstream issue linux-audit/audit-kernel#96 Adapted from Vladis Dronov's v2 patch. Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
The LSM_AUDIT_DATA_* records for PATH, FILE, IOCTL_OP, DENTRY and INODE are incomplete without the task context of the AUDIT Current Working Directory record. Add it. This record addition can't use audit_dummy_context to determine whether or not to store the record information since the LSM_AUDIT_DATA_* records are initiated by various LSMs independent of any audit rules. context->in_syscall is used to determine if it was called in user context like audit_getname. Please see the upstream issue #96 Adapted from Vladis Dronov's v2 patch. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Signed-off-by: Paul Moore <paul@paul-moore.com>
|
2020-07-18: NULL pointer dereference reported due to 2020-07-27: Post addendum: |
Issue ghak120 enabled syscall records to accompany required records when no rules are present to trigger the storage of syscall context. A reported issue showed that the cwd was not always initialized. That issue was already resolved, but a review of all other records that could be triggered at the time of a syscall record revealed other potential values that could be missing or misleading. Initialize them. The fds array is reset to -1 after the first syscall to indicate it isn't valid any more, but was never set to -1 when the context was allocated to indicate it wasn't yet valid. The audit_inode* functions can be called without going through getname_flags() or getname_kernel() that sets audit_names and cwd, so set the cwd if it has not already been done so due to audit_names being valid. The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was missed with the ghak96 patch, so add that case here. Please see issue linux-audit/audit-kernel#120 Please see issue linux-audit/audit-kernel#96 Passes audit-testsuite. Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
|
Replacement addendum: |
When there are no audit rules registered, mandatory records (config, etc.) are missing their accompanying records (syscall, proctitle, etc.). This is due to audit context dummy set on syscall entry based on absence of rules that signals that no other records are to be printed. Clear the dummy bit if any record is generated. The proctitle context and dummy checks are pointless since the proctitle record will not be printed if no syscall records are printed. The fds array is reset to -1 after the first syscall to indicate it isn't valid any more, but was never set to -1 when the context was allocated to indicate it wasn't yet valid. The audit_inode* functions can be called without going through getname_flags() or getname_kernel() that sets audit_names and cwd, so set the cwd if it has not already been done so due to audit_names being valid. The LSM dump_common_audit_data() LSM_AUDIT_DATA_NET:AF_UNIX case was missed with the ghak96 patch, so add that case here. Thanks to bauen1 <j2468h@googlemail.com> for reporting LSM situations in which context->cwd is not valid, inadvertantly fixed by the ghak96 patch. Please see upstream github issue linux-audit/audit-kernel#120 This is also related to upstream github issue linux-audit/audit-kernel#96 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
|
This ghak120 v5 patch effectively reverts all previous patches on this issue: |
When there are no audit rules registered, mandatory records (config, etc.) are missing their accompanying records (syscall, proctitle, etc.). This is due to audit context dummy set on syscall entry based on absence of rules that signals that no other records are to be printed. Clear the dummy bit if any record is generated, open coding this in audit_log_start(). The proctitle context and dummy checks are pointless since the proctitle record will not be printed if no syscall records are printed. The fds array is reset to -1 after the first syscall to indicate it isn't valid any more, but was never set to -1 when the context was allocated to indicate it wasn't yet valid. Check ctx->pwd in audit_log_name(). The audit_inode* functions can be called without going through getname_flags() or getname_kernel() that sets audit_names and cwd, so set the cwd in audit_alloc_name() if it has not already been done so due to audit_names being valid and purge all other audit_getcwd() calls. Revert the LSM dump_common_audit_data() LSM_AUDIT_DATA_* cases from the ghak96 patch since they are no longer necessary due to cwd coverage in audit_alloc_name(). Thanks to bauen1 <j2468h@googlemail.com> for reporting LSM situations in which context->cwd is not valid, inadvertantly fixed by the ghak96 patch. Please see upstream github issue linux-audit/audit-kernel#120 This is also related to upstream github issue linux-audit/audit-kernel#96 Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
When there are no audit rules registered, mandatory records (config, etc.) are missing their accompanying records (syscall, proctitle, etc.). This is due to audit context dummy set on syscall entry based on absence of rules that signals that no other records are to be printed. Clear the dummy bit if any record is generated, open coding this in audit_log_start(). The proctitle context and dummy checks are pointless since the proctitle record will not be printed if no syscall records are printed. The fds array is reset to -1 after the first syscall to indicate it isn't valid any more, but was never set to -1 when the context was allocated to indicate it wasn't yet valid. Check ctx->pwd in audit_log_name(). The audit_inode* functions can be called without going through getname_flags() or getname_kernel() that sets audit_names and cwd, so set the cwd in audit_alloc_name() if it has not already been done so due to audit_names being valid and purge all other audit_getcwd() calls. Revert the LSM dump_common_audit_data() LSM_AUDIT_DATA_* cases from the ghak96 patch since they are no longer necessary due to cwd coverage in audit_alloc_name(). Thanks to bauen1 <j2468h@googlemail.com> for reporting LSM situations in which context->cwd is not valid, inadvertantly fixed by the ghak96 patch. Please see upstream github issue #120 This is also related to upstream github issue #96 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> Signed-off-by: Paul Moore <paul@paul-moore.com>
|
This can be closed since this patch went upstream in v5.9-rc1 And further, it ended up being tangled with #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 |
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: #65
The text was updated successfully, but these errors were encountered: