search

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
140 stars 37 forks source link

Q: add subj attr to AUDIT_INTEGRITY_RULE record #52

Closed rgbriggs closed 5 years ago

rgbriggs commented 7 years ago

The AUDIT_INTEGRITY_RULE record is unaccompanied (NULL context) and does not include subject attributes. Are the subject attributes available? If so, include them. Alternately, should this record be accompanying a SYSCALL record in which case the context pointer should be provided.

stevegrubb commented 7 years ago

The integrity logging is a complete mess. I do not think they are running anything by the linux-audit mail list for our ack. For example, they are logging > and < symbols between name and value.

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/security/integrity/ima/ima_policy.c?h=v4.13.3#n591

Their work has to be redone from scratch,.

pcmoore commented 6 years ago

I'm thinking the IMA records should be accompanied records.

stevegrubb commented 6 years ago

The IMA records need to get completely redone as they violate all auditing requirements. These records should not have a SYSCALL record attached. Only events triggered by a syscall audit rule should be multipart.

pcmoore commented 6 years ago

I haven't looked at the IMA records enough to be able to give any opinion on their content, however, when it comes to whether a record should by accompanied or not, I can't think of any good reason why we would not want to connect all of the information we have when an audit event is triggered.

stevegrubb commented 6 years ago

The reason is because diskspace is at a premium and more information means more parsing which really does impact performance. If you have 1 IMA event, you probably have a lot on the way because the system was not setup correctly. If the IMA folks have tooling around events, adding the minimal amount of information to complete it is probably less disruptive than adding a whole new record. But that is really minor compared to the damage caused to what used to be almost good records. Someone really needs to check in on the IMA events and work with them to fix things back.

pcmoore commented 6 years ago

So the reason why you prefer unaccompanied records is due to storage demands and offline processing? Noted.

rgbriggs commented 6 years ago

Given the conclusion of ghak51 (syscall support forced on with enable), I agree IMA records should accompany syscall records unless we can factor out task info to its own record. I also understand the mess created by gt and lt operators in the place of eq to connect field names with their values, but don't have a useful solution yet.

stevegrubb commented 6 years ago

The issue that is being reported was against IMA records that were almost perfect from an audit point of view. Check the IMA events on RHEL 7 for reference. At some point, the IMA people wrecked the audit records and that is what I'm pointing out in my first comment. IOW, a syscall record was not needed because they had the important information in the event except one thing.

So, we went from they need one field and everything is perfect, to what the heck happened to these events and why were we not included?

rgbriggs commented 6 years ago

One option is to submit a revert patch citing that it breaks userspace. Linus does not take kindly to reports that stuff breaks userspace. It was commit 3dd0c8d06511c7c61c62305fcf431ca28884d263 ("ima: provide ">" and "<" operators for fowner/uid/euid rules.").

Another might be to send a fixup patch that moves the operator into the value field (perhaps immediately after the field/value separator).

pcmoore commented 6 years ago

While yes, Linus' does hate userspace breakage, asking to revert patches that are over a year old can be difficult and often brings into question the significance of the breakage. I would suggest a fixup patch in conjunction with reminding the IMA/EVM folks that they should be sending audit related patches to the linux-audit mailing list for review.

rgbriggs commented 6 years ago

On 2018-03-07 16:40, Paul Moore wrote:

While yes, Linus' does hate userspace breakage, asking to revert patches that are over a year old can be difficult and often brings into question the significance of the breakage.

I was thinking the same, (and should have been explicit about that) which is why I suggested the alternative.

Steve, if this had been reported earlier with a revert patch submitted upstream, the revert would have been supported by the community and sent a strong signal to the patch submitters that the audit patch review procedure on the linux-audit mailing list is important.

I would suggest a fixup patch in conjunction with reminding the IMA/EVM folks that they should be sending audit related patches to the linux-audit mailing list for review.

Sending a fixup patch also works, but doesn't send as strong a signal that this breaks things and doesn't give quite as much control to the patch submitters to choose their desired format for the fix but they still have the option of providing feedback or even submitting a corrective patch that they are happier with.

Steve, that said, do you have a preferred format for the value portion of that field to express that operation? I'd recommend untrustedstring at least so that the shell won't be fooled by false redirections. I'd just put the op character as the first character of the value string, but I realise that would break parsing of the value field for interpretation, so that suggests that each field that has an operator option need its own operator field too, such as uid_op=">", euid_op="<" but since fowner doesn't appear to be an interpreted field yet (or the field dictionary is out of date) it could include the operator in the fowner=">..." value field. It probably makes more sense to treat them all the same though and append an fowner_op=">" field.

pcmoore commented 6 years ago

Sending a fixup patch also works, but doesn't send as strong a signal that this breaks things and doesn't give quite as much control to the patch submitters to choose their desired format for the fix but they still have the option of providing feedback or even submitting a corrective patch that they are happier with.

I think we missed the opportunity to send a "strong signal" with a revert, it's too late for that now, a patch fixing the problem is the best we can expect at this point. At this point as long as we fix the problem we should be satisfied. We can, and should, remind them that patches which affect the audit records should be CC'd to the linux-audit list.

stevegrubb commented 6 years ago

I wished this was discussed on the mail list with the IMA people. I have no idea why they are recording '<>' operators. To fix this means we need them to explain what they were trying to do. It's fine for them to have those operators for policy rules. They just don't belong in the audit trail.

rgbriggs commented 6 years ago

Upstream patchsets submitted for discussion by IMA team: v1: https://www.redhat.com/archives/linux-audit/2018-May/msg00155.html v2: https://www.redhat.com/archives/linux-audit/2018-May/msg00207.html v3: https://www.redhat.com/archives/linux-audit/2018-June/msg00025.html

rgbriggs commented 5 years ago

V3 Upstreamed: 8a3bcaf6ecd3 ima: Call audit_log_string() rather than logging it untrusted 3d2859d5d4c3 ima: Use audit_log_format() rather than audit_log_string() 2afd020aaeee ima: Do not audit if CONFIG_INTEGRITY_AUDIT is not set dba31ee75941 ima: Differentiate auditing policy rules from "audit" actions

rgbriggs commented 5 years ago

Addressing the audit message format violations in new issue https://github.com/linux-audit/audit-kernel/issues/113 .