Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On 3/3/2022 3:36 PM, Paul Moore wrote:
On Wed, Feb 2, 2022 at 7:20 PM Casey Schaufler wrote:
Add a list for auxiliary record data to the audit_buffer structure.
Add the audit_stamp information to the audit_buffer as there's no
guarantee that there will be an audit_context containing the stamp
associated with the event. At audit_log_end() time create auxiliary
records (none are currently defined) as have been added to the list.
Signed-off-by: Casey Schaufler
---
kernel/audit.c | 84 --
1 file changed, 74 insertions(+), 10 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index f012c3786264..559fb14e0380 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -191,15 +191,25 @@ static struct audit_ctl_mutex {
* should be at least that large. */
#define AUDIT_BUFSIZ 1024
+/* The audit_context_entry contains data required to create an
+ * auxiliary record.
+ */
+struct audit_context_entry {
+ struct list_headlist;
+ int type; /* Audit record type */
+};
Looking at how this ends up being used later in the patchset I think
we would be better off if we stored a fully formed audit_buffer in the
struct above instead of data fields which we would use to generate an
audit_buffer in audit_log_end(). This helps tie the buffer generation
logic in with the existing code with which it is most closely related,
it allows us to report errors back to the caller as audit_log_end()
doesn't historically return an error code, and it helps us get ahead
of any future data lifetime issues we might run into by storing the
data in this audit struct.
OK, I'll buy that.
This would also simplify things with respect to the audit_buffer
struct. Instead of having a dedicated struct for the aux data, you
could simply leverage the existing sk_buff list mechanisms:
I can't say that "simply" is the adverb I'd choose, but sure,
I can do this.
struct audit_buffer {
struct sk_buff *skb; /* part of @skb_list, kept for audit_log funcs */
struct sk_buff_head skb_list;
struct audit_context *ctx;
struct audit_stamp stamp;
gfp_t gfp_mask;
}
The only sneaky bit in the struct above is that we likely want to
preserve audit_buffer::skb as a dedicated skb pointer so we don't have
to modify all of the audit_log_*() functions; you could of course, but
I'm guessing there is little appetite for that in the context of this
patchset.
I will give it a go without making the massive interface change.
Adding a new aux record would involve calling some private audit
function (no one outside of the audit subsystem should need access)
that would allocate a new skb similar to what we do in
audit_buffer_alloc() and add it to the end of the sk_buff_head list
via skb_queue_tail() and resetting audit_buffer::skb to point to the
newly allocated skb.
Good naming may be tricky as we need to indicate that a new buffer is
being allocated for an attached aux record and that the buffer to which
it's being attached is going to temporarily be in a curious state.
audit_buffer_add_aux() seems wordy, but it's what I'll start with lacking
a better suggestion.
This would allow all of the existing
audit_log*() functions to work correctly, and when you are done you
can restore the "main" skb with skb_peek().
audit_buffer_close_aux()
If for some reason you
need to fail the new aux record mid-creation you just dequeue the list
tail, free the skb, and skb_peek() the "main" skb back into place.
Why do I always get nervous when I hear "just" and "skb" in the
same sentence?
/* The audit_buffer is used when formatting an audit record. The caller
* locks briefly to get the record off the freelist or to allocate the
* buffer, and locks briefly to send the buffer to the netlink layer or
* to place it on a transmit queue. Multiple audit_buffers can be in
* use simultaneously. */
struct audit_buffer {
- struct sk_buff *skb; /* formatted skb ready to send */
- struct audit_context *ctx; /* NULL or associated context */
- gfp_tgfp_mask;
+ struct sk_buff *skb; /* formatted skb ready to send */
+ struct audit_context*ctx; /* NULL or associated context */
+ struct list_headaux_records;/* aux record data */
+ struct audit_stamp stamp; /* event stamp */
+ gfp_t gfp_mask;
};
...
@@ -2408,6 +2418,60 @@ void audit_log_end(struct audit_buffer *ab)
wake_up_interruptible(&kauditd_wait);
} else
audit_log_lost("rate limit exceeded");
+}
+
+/**
+ * audit_log_end - end one audit record
+ * @ab: the audit_buffer
+ *
+ * Let __audit_log_end() handle the message while the buffer housekeeping
+ * is done here.
+ * If there are other records that have been deferred for the event
+ * create them here.
+ */
+void audit_log_end(struct audit_buffer *ab)
+{
+
Re: [PATCH v32 26/28] Audit: Add record for multiple object security contexts
On 3/3/2022 3:36 PM, Paul Moore wrote:
On Wed, Feb 2, 2022 at 7:23 PM Casey Schaufler wrote:
Create a new audit record AUDIT_MAC_OBJ_CONTEXTS.
An example of the MAC_OBJ_CONTEXTS (1421) record is:
type=MAC_OBJ_CONTEXTS[1421]
msg=audit(1601152467.009:1050):
obj_selinux=unconfined_u:object_r:user_home_t:s0
When an audit event includes a AUDIT_MAC_OBJ_CONTEXTS record
the "obj=" field in other records in the event will be "obj=?".
An AUDIT_MAC_OBJ_CONTEXTS record is supplied when the system has
multiple security modules that may make access decisions based
on an object security context.
Signed-off-by: Casey Schaufler
---
include/linux/audit.h | 5
include/uapi/linux/audit.h | 1 +
kernel/audit.c | 59 ++
kernel/auditsc.c | 37
4 files changed, 70 insertions(+), 32 deletions(-)
...
diff --git a/kernel/audit.c b/kernel/audit.c
index e8744e80ef21..3b9ce617b150 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2199,6 +2200,43 @@ int audit_log_task_context(struct audit_buffer *ab)
}
EXPORT_SYMBOL(audit_log_task_context);
+void audit_log_object_context(struct audit_buffer *ab, struct lsmblob *blob)
+{
+ struct audit_context_entry *ace;
+ struct lsmcontext context;
+ int error;
+
+ if (!lsm_multiple_contexts()) {
+ error = security_secid_to_secctx(blob, &context, LSMBLOB_FIRST);
+ if (error) {
+ if (error != -EINVAL)
+ goto error_path;
+ return;
+ }
+ audit_log_format(ab, " obj=%s", context.context);
+ security_release_secctx(&context);
+ } else {
+ /*
+* If there is more than one security module that has a
+* object "context" it's necessary to put the object data
+* into a separate record to maintain compatibility.
+*/
I know this is nitpicky, but I'm going to say it anyway ... the
separate record isn't purely for compatibility reasons, it's for size
reasons. There is a fear that multiple LSM labels could blow past the
record size limit when combined with other fields, so putting them in
their own dedicated record gives us more room. If that wasn't the
case we could just tack them on the end of existing records.
Fair enough. I have no objection to adding commentary that will
help the next developer who comes into this code.
However, converting the existing "obj=" field into "obj=?" when
multiple LSM labels are present *is* a compatibility nod as it allows
existing userspace tooling that expects a single "obj=" field to
continue to work.
Likewise here.
+ audit_log_format(ab, " obj=?");
+ ace = kzalloc(sizeof(*ace), ab->gfp_mask);
+ if (!ace)
+ goto error_path;
+ INIT_LIST_HEAD(&ace->list);
+ ace->type = AUDIT_MAC_OBJ_CONTEXTS;
+ ace->lsm_objs = *blob;
+ list_add(&ace->list, &ab->aux_records);
+ }
+ return;
+
+error_path:
+ audit_panic("error in audit_log_object_context");
+}
+EXPORT_SYMBOL(audit_log_object_context);
+
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On Wed, Feb 2, 2022 at 7:20 PM Casey Schaufler wrote:
>
> Add a list for auxiliary record data to the audit_buffer structure.
> Add the audit_stamp information to the audit_buffer as there's no
> guarantee that there will be an audit_context containing the stamp
> associated with the event. At audit_log_end() time create auxiliary
> records (none are currently defined) as have been added to the list.
>
> Signed-off-by: Casey Schaufler
> ---
> kernel/audit.c | 84 --
> 1 file changed, 74 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/audit.c b/kernel/audit.c
> index f012c3786264..559fb14e0380 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -191,15 +191,25 @@ static struct audit_ctl_mutex {
> * should be at least that large. */
> #define AUDIT_BUFSIZ 1024
>
> +/* The audit_context_entry contains data required to create an
> + * auxiliary record.
> + */
> +struct audit_context_entry {
> + struct list_headlist;
> + int type; /* Audit record type */
> +};
Looking at how this ends up being used later in the patchset I think
we would be better off if we stored a fully formed audit_buffer in the
struct above instead of data fields which we would use to generate an
audit_buffer in audit_log_end(). This helps tie the buffer generation
logic in with the existing code with which it is most closely related,
it allows us to report errors back to the caller as audit_log_end()
doesn't historically return an error code, and it helps us get ahead
of any future data lifetime issues we might run into by storing the
data in this audit struct.
This would also simplify things with respect to the audit_buffer
struct. Instead of having a dedicated struct for the aux data, you
could simply leverage the existing sk_buff list mechanisms:
struct audit_buffer {
struct sk_buff *skb; /* part of @skb_list, kept for audit_log funcs */
struct sk_buff_head skb_list;
struct audit_context *ctx;
struct audit_stamp stamp;
gfp_t gfp_mask;
}
The only sneaky bit in the struct above is that we likely want to
preserve audit_buffer::skb as a dedicated skb pointer so we don't have
to modify all of the audit_log_*() functions; you could of course, but
I'm guessing there is little appetite for that in the context of this
patchset.
Adding a new aux record would involve calling some private audit
function (no one outside of the audit subsystem should need access)
that would allocate a new skb similar to what we do in
audit_buffer_alloc() and add it to the end of the sk_buff_head list
via skb_queue_tail() and resetting audit_buffer::skb to point to the
newly allocated skb. This would allow all of the existing
audit_log*() functions to work correctly, and when you are done you
can restore the "main" skb with skb_peek(). If for some reason you
need to fail the new aux record mid-creation you just dequeue the list
tail, free the skb, and skb_peek() the "main" skb back into place.
> /* The audit_buffer is used when formatting an audit record. The caller
> * locks briefly to get the record off the freelist or to allocate the
> * buffer, and locks briefly to send the buffer to the netlink layer or
> * to place it on a transmit queue. Multiple audit_buffers can be in
> * use simultaneously. */
> struct audit_buffer {
> - struct sk_buff *skb; /* formatted skb ready to send */
> - struct audit_context *ctx; /* NULL or associated context */
> - gfp_tgfp_mask;
> + struct sk_buff *skb; /* formatted skb ready to send */
> + struct audit_context*ctx; /* NULL or associated context */
> + struct list_headaux_records;/* aux record data */
> + struct audit_stamp stamp; /* event stamp */
> + gfp_t gfp_mask;
> };
...
> @@ -2408,6 +2418,60 @@ void audit_log_end(struct audit_buffer *ab)
> wake_up_interruptible(&kauditd_wait);
> } else
> audit_log_lost("rate limit exceeded");
> +}
> +
> +/**
> + * audit_log_end - end one audit record
> + * @ab: the audit_buffer
> + *
> + * Let __audit_log_end() handle the message while the buffer housekeeping
> + * is done here.
> + * If there are other records that have been deferred for the event
> + * create them here.
> + */
> +void audit_log_end(struct audit_buffer *ab)
> +{
> + struct audit_context_entry *entry;
> + struct audit_context mcontext;
> + struct audit_context *mctx;
> + struct audit_buffer *mab;
> + struct list_head *l;
> + struct list_head *n;
> +
> + if (!ab)
> + return;
> +
> + __audit_log_end(ab);
> +
> + if (list_empty(&ab->aux_records)) {
> + audit_buffer_free(ab);
> + return;
> + }
> +
> + if (ab->ctx == NULL) {
> + mcontext.stamp = ab->stamp;
> + mctx = &mc
Re: [PATCH v32 26/28] Audit: Add record for multiple object security contexts
On Wed, Feb 2, 2022 at 7:23 PM Casey Schaufler wrote:
>
> Create a new audit record AUDIT_MAC_OBJ_CONTEXTS.
> An example of the MAC_OBJ_CONTEXTS (1421) record is:
>
> type=MAC_OBJ_CONTEXTS[1421]
> msg=audit(1601152467.009:1050):
> obj_selinux=unconfined_u:object_r:user_home_t:s0
>
> When an audit event includes a AUDIT_MAC_OBJ_CONTEXTS record
> the "obj=" field in other records in the event will be "obj=?".
> An AUDIT_MAC_OBJ_CONTEXTS record is supplied when the system has
> multiple security modules that may make access decisions based
> on an object security context.
>
> Signed-off-by: Casey Schaufler
> ---
> include/linux/audit.h | 5
> include/uapi/linux/audit.h | 1 +
> kernel/audit.c | 59 ++
> kernel/auditsc.c | 37
> 4 files changed, 70 insertions(+), 32 deletions(-)
...
> diff --git a/kernel/audit.c b/kernel/audit.c
> index e8744e80ef21..3b9ce617b150 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -2199,6 +2200,43 @@ int audit_log_task_context(struct audit_buffer *ab)
> }
> EXPORT_SYMBOL(audit_log_task_context);
>
> +void audit_log_object_context(struct audit_buffer *ab, struct lsmblob *blob)
> +{
> + struct audit_context_entry *ace;
> + struct lsmcontext context;
> + int error;
> +
> + if (!lsm_multiple_contexts()) {
> + error = security_secid_to_secctx(blob, &context,
> LSMBLOB_FIRST);
> + if (error) {
> + if (error != -EINVAL)
> + goto error_path;
> + return;
> + }
> + audit_log_format(ab, " obj=%s", context.context);
> + security_release_secctx(&context);
> + } else {
> + /*
> +* If there is more than one security module that has a
> +* object "context" it's necessary to put the object data
> +* into a separate record to maintain compatibility.
> +*/
I know this is nitpicky, but I'm going to say it anyway ... the
separate record isn't purely for compatibility reasons, it's for size
reasons. There is a fear that multiple LSM labels could blow past the
record size limit when combined with other fields, so putting them in
their own dedicated record gives us more room. If that wasn't the
case we could just tack them on the end of existing records.
However, converting the existing "obj=" field into "obj=?" when
multiple LSM labels are present *is* a compatibility nod as it allows
existing userspace tooling that expects a single "obj=" field to
continue to work.
> + audit_log_format(ab, " obj=?");
> + ace = kzalloc(sizeof(*ace), ab->gfp_mask);
> + if (!ace)
> + goto error_path;
> + INIT_LIST_HEAD(&ace->list);
> + ace->type = AUDIT_MAC_OBJ_CONTEXTS;
> + ace->lsm_objs = *blob;
> + list_add(&ace->list, &ab->aux_records);
> + }
> + return;
> +
> +error_path:
> + audit_panic("error in audit_log_object_context");
> +}
> +EXPORT_SYMBOL(audit_log_object_context);
> +
--
paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On 3/3/2022 2:43 PM, Paul Moore wrote: On Thu, Mar 3, 2022 at 5:33 PM Casey Schaufler wrote: On 3/3/2022 2:27 PM, Paul Moore wrote: On Wed, Mar 2, 2022 at 5:32 PM Casey Schaufler wrote: On 2/2/2022 3:53 PM, Casey Schaufler wrote: Add a list for auxiliary record data to the audit_buffer structure. Add the audit_stamp information to the audit_buffer as there's no guarantee that there will be an audit_context containing the stamp associated with the event. At audit_log_end() time create auxiliary records (none are currently defined) as have been added to the list. Signed-off-by: Casey Schaufler I'm really hoping for either Acks or feedback on this approach. The only callers that make use of this functionality in this patchset is in kernel/audit*.c in patches 25/28 and 26/28, yes? Yes. Thanks. I just wanted to make sure you weren't planning on any additional callers in a future revision. I understand that things may change, but I just wanted to make sure there wasn't already something pending. I don't have anything I know about. It's possible that something could be needed when the stacking changes for networking come in, but that's not going to come in for "some time" yet. I think that the container ID record could use it as well. I haven't looked deeply, but it should be usable for any aux record type. Possibly, but I'm intentionally trying to keep that separated at this stage as the ordering is uncertain. If/when both bits of functionality land we can reconcile things as needed; it's all internal implementation details so we don't have to worry too much about changing it later. Agreed, although I'd hate to duplicate mechanism if someone else has an equally functional proposal. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On Thu, Mar 3, 2022 at 5:33 PM Casey Schaufler wrote: > On 3/3/2022 2:27 PM, Paul Moore wrote: > > On Wed, Mar 2, 2022 at 5:32 PM Casey Schaufler > > wrote: > >> On 2/2/2022 3:53 PM, Casey Schaufler wrote: > >>> Add a list for auxiliary record data to the audit_buffer structure. > >>> Add the audit_stamp information to the audit_buffer as there's no > >>> guarantee that there will be an audit_context containing the stamp > >>> associated with the event. At audit_log_end() time create auxiliary > >>> records (none are currently defined) as have been added to the list. > >>> > >>> Signed-off-by: Casey Schaufler > >> I'm really hoping for either Acks or feedback on this approach. > > The only callers that make use of this functionality in this patchset > > is in kernel/audit*.c in patches 25/28 and 26/28, yes? > > Yes. Thanks. I just wanted to make sure you weren't planning on any additional callers in a future revision. I understand that things may change, but I just wanted to make sure there wasn't already something pending. > I think that the container ID record could use it as well. > I haven't looked deeply, but it should be usable for any aux record type. Possibly, but I'm intentionally trying to keep that separated at this stage as the ordering is uncertain. If/when both bits of functionality land we can reconcile things as needed; it's all internal implementation details so we don't have to worry too much about changing it later. -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On 3/3/2022 2:27 PM, Paul Moore wrote: On Wed, Mar 2, 2022 at 5:32 PM Casey Schaufler wrote: On 2/2/2022 3:53 PM, Casey Schaufler wrote: Add a list for auxiliary record data to the audit_buffer structure. Add the audit_stamp information to the audit_buffer as there's no guarantee that there will be an audit_context containing the stamp associated with the event. At audit_log_end() time create auxiliary records (none are currently defined) as have been added to the list. Signed-off-by: Casey Schaufler I'm really hoping for either Acks or feedback on this approach. The only callers that make use of this functionality in this patchset is in kernel/audit*.c in patches 25/28 and 26/28, yes? Yes. I think that the container ID record could use it as well. I haven't looked deeply, but it should be usable for any aux record type. -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v32 24/28] Audit: Add framework for auxiliary records
On Wed, Mar 2, 2022 at 5:32 PM Casey Schaufler wrote: > On 2/2/2022 3:53 PM, Casey Schaufler wrote: > > Add a list for auxiliary record data to the audit_buffer structure. > > Add the audit_stamp information to the audit_buffer as there's no > > guarantee that there will be an audit_context containing the stamp > > associated with the event. At audit_log_end() time create auxiliary > > records (none are currently defined) as have been added to the list. > > > > Signed-off-by: Casey Schaufler > > I'm really hoping for either Acks or feedback on this approach. The only callers that make use of this functionality in this patchset is in kernel/audit*.c in patches 25/28 and 26/28, yes? -- paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
Re: Bug in Queue Statistics?
Hello, On Wednesday, March 2, 2022 4:45:07 PM EST Amjad Gabbar wrote: > Had a couple of concerns that I wanted to discuss: > > 1. > > I was getting a few "auditd queue full" messages in syslog. I had > previously faced similar issues after which I had increased the q_depth and > modified my ruleset to reduce the number of events logged which had > brought down these errors significantly. > I am not sure but the only way I can think that max plugin queue depth used > can be 4294967295 (despite the maxlimit being set to 25000) is if we > dequeue an event before it has been enqueued. Also, the current plugin > queue depth suggests that events are being dequeued continuously leading > to the value decreasing from 4294967295 to 4294967240? I have a feeling this is hard to get into, but is something like the queue gets suspended, reconfigure gets rid of plugins, another reconfigure adds back plugins and this resets the queue depth to 0, but there are events. As it dequeues them it starts getting negative numbers, but the variable is undined int hence the very large numbers. I changed the way that init and reconfigure interact in commit 514d2bd. When the queue is destroyed, it resets all the numbers. Otherwise it will now leave them alone unless q_depth is zero - which means we need to reallocate a queue. > 2. > > Another update that I would like to make is currently, if we reload the > auditd configuration instead of restarting, although the configuration > changes, we do not reset some of the queue statistic variables which I feel > is incorrect. > > https://github.com/linux-audit/audit-userspace/blob/770e4f538103f8a055f46c0 > 4a9e2514f88f175c3/src/auditd-event.c#L1466 > > Ex- If q_depth=400 and the queue overflows, the overflowed variable is set > to 1. On changing the q_depth value to say 1 and doing a reload, the > queue size has changed and basically so has the queue. I feel here we > should reset some of the queue statistic variables like overflowed as it is > incorrect to say that in it's current form the queue has overflown. This > variable is not reset and I feel that it should be. This sounds reasonable. Commit a8d7515 should fix this. > If agreed that this is a reasonable change, would it be ok if I submit a PR > for the same? It's a 1 liner. Thanks, though. > Also, is it possible that point 2 is causing issues leading to point 1 > errors? No. > 3. Would also like to improve the manpage documentation related to > /var/run/auditd.state. Currently it states that it is a dump of the > internal state. I would like to change that to provide a little more > detail about what the internal state contains - such as queue statistics, > priority etc. That might be a lot to document. I don't know if anything else will get added, but if that happens, we'll have to document that. If this was done, I'd say it should go in the auditd man page. > Apart from that I feel that we can also add an additonal field to the > auditd.state file as to when the queue has overflown which may make it > easier to perform ausearch related queries with start time and end time. Would having it record down to the second be enough? IOW, no sub-second time stamp. If just the second is good enough, I can add that. It's only a couple lines of code. > If any of the changes are worth contributing to I would be happy to make > the said changes. I don't plan to write a description of the state report. I'll take a PR, though. Best, -Steve -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit
