lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <51b05437-c84d-4611-b294-f4944128728f@schaufler-ca.com>
Date: Wed, 6 Aug 2025 09:03:24 -0700
From: Casey Schaufler <casey@...aufler-ca.com>
To: Paul Moore <paul@...l-moore.com>, eparis@...hat.com,
 linux-security-module@...r.kernel.org, audit@...r.kernel.org
Cc: jmorris@...ei.org, serge@...lyn.com, keescook@...omium.org,
 john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
 stephen.smalley.work@...il.com, linux-kernel@...r.kernel.org,
 selinux@...r.kernel.org, Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v5 3/5] Audit: Add record for multiple task security
 contexts

On 8/5/2025 12:39 PM, Paul Moore wrote:
> On Jul 16, 2025 Casey Schaufler <casey@...aufler-ca.com> wrote:
>> Replace the single skb pointer in an audit_buffer with a list of
>> skb pointers. 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 as have been added to the list. Functions are
>> created to manage the skb list in the audit_buffer.
>>
>> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
>> An example of the MAC_TASK_CONTEXTS record is:
>>
>>     type=MAC_TASK_CONTEXTS
>>     msg=audit(1600880931.832:113)
>>     subj_apparmor=unconfined
>>     subj_smack=_
>>
>> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record the
>> "subj=" field in other records in the event will be "subj=?".
>> An AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
>> multiple security modules that may make access decisions based on a
>> subject security context.
>>
>> Refactor audit_log_task_context(), creating a new audit_log_subj_ctx().
>> This is used in netlabel auditing to provide multiple subject security
>> contexts as necessary.
>>
>> Suggested-by: Paul Moore <paul@...l-moore.com>
>> Signed-off-by: Casey Schaufler <casey@...aufler-ca.com>
>> ---
>>  include/linux/audit.h        |  16 +++
>>  include/uapi/linux/audit.h   |   1 +
>>  kernel/audit.c               | 207 +++++++++++++++++++++++++++++------
>>  net/netlabel/netlabel_user.c |   9 +-
>>  security/apparmor/lsm.c      |   3 +
>>  security/lsm.h               |   4 -
>>  security/lsm_init.c          |   5 -
>>  security/security.c          |   3 -
>>  security/selinux/hooks.c     |   3 +
>>  security/smack/smack_lsm.c   |   3 +
>>  10 files changed, 202 insertions(+), 52 deletions(-)
> If there were no other issues with this patch I would have just fixed
> this up during the merge (I did it in my review branch already), but
> since you're no longer dependent on the LSM init rework changes (and
> I've dropped the subj/obj counting in the latest revision), just go
> ahead and base your next revision on the audit tree or Linus' tree as
> one normally would.

OK. Can I assume that the dependency on the init changes has dropped?

>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 226c8ae00d04..c7dea6bfacdd 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
> ..
>
>> +/**
>> + * audit_log_subj_ctx - Add LSM subject information
>> + * @ab: audit_buffer
>> + * @prop: LSM subject properties.
>> + *
>> + * Add a subj= field and, if necessary, a AUDIT_MAC_TASK_CONTEXTS record.
>> + */
>> +int audit_log_subj_ctx(struct audit_buffer *ab, struct lsm_prop *prop)
>>  {
>> -	struct lsm_prop prop;
>>  	struct lsm_context ctx;
>> +	char *space = "";
>>  	int error;
>> +	int i;
>>  
>> -	security_current_getlsmprop_subj(&prop);
>> -	if (!lsmprop_is_set(&prop))
>> +	security_current_getlsmprop_subj(prop);
>> +	if (!lsmprop_is_set(prop))
>>  		return 0;
>>  
>> -	error = security_lsmprop_to_secctx(&prop, &ctx, LSM_ID_UNDEF);
>> -	if (error < 0) {
>> -		if (error != -EINVAL)
>> -			goto error_path;
>> +	if (audit_subj_secctx_cnt < 2) {
>> +		error = security_lsmprop_to_secctx(prop, &ctx, LSM_ID_UNDEF);
>> +		if (error < 0) {
>> +			if (error != -EINVAL)
>> +				goto error_path;
>> +			return 0;
>> +		}
>> +		audit_log_format(ab, " subj=%s", ctx.context);
>> +		security_release_secctx(&ctx);
>>  		return 0;
>>  	}
>> -
>> -	audit_log_format(ab, " subj=%s", ctx.context);
>> -	security_release_secctx(&ctx);
>> +	/* Multiple LSMs provide contexts. Include an aux record. */
>> +	audit_log_format(ab, " subj=?");
>> +	error = audit_buffer_aux_new(ab, AUDIT_MAC_TASK_CONTEXTS);
>> +	if (error)
>> +		goto error_path;
>> +
>> +	for (i = 0; i < audit_subj_secctx_cnt; i++) {
>> +		error = security_lsmprop_to_secctx(prop, &ctx,
>> +						   audit_subj_lsms[i]->id);
>> +		if (error < 0) {
>> +			/*
>> +			 * Don't print anything. An LSM like BPF could
>> +			 * claim to support contexts, but only do so under
>> +			 * certain conditions.
>> +			 */
>> +			if (error == -EOPNOTSUPP)
>> +				continue;
>> +			if (error != -EINVAL)
>> +				audit_panic("error in audit_log_task_context");
> Argh ... please read prior review comments a bit more carefully.  As was
> pointed out in the v4 posting you're using the wrong function name here.
>
> https://lore.kernel.org/audit/fc242f4c853fee16e587e9c78e1f282e@paul-moore.com

Yeah, that was sloppy. Sorry. Will fix.

>> +		} else {
>> +			audit_log_format(ab, "%ssubj_%s=%s", space,
>> +					 audit_subj_lsms[i]->name, ctx.context);
>> +			space = " ";
>> +			security_release_secctx(&ctx);
>> +		}
>> +	}
>> +	audit_buffer_aux_end(ab);
>>  	return 0;
>>  
>>  error_path:
>> -	audit_panic("error in audit_log_task_context");
>> +	audit_panic("error in audit_log_subj_ctx");
>>  	return error;
>>  }
>> +EXPORT_SYMBOL(audit_log_subj_ctx);
> ..
>
>> @@ -2423,25 +2575,16 @@ int audit_signal_info(int sig, struct task_struct *t)
>>  void audit_log_end(struct audit_buffer *ab)
>>  {
>>  	struct sk_buff *skb;
>> -	struct nlmsghdr *nlh;
>>  
>>  	if (!ab)
>>  		return;
>>  
>> -	if (audit_rate_check()) {
>> -		skb = ab->skb;
>> -		ab->skb = NULL;
>> +	while ((skb = skb_dequeue(&ab->skb_list)))
>> +		__audit_log_end(skb);
>>  
>> -		/* setup the netlink header, see the comments in
>> -		 * kauditd_send_multicast_skb() for length quirks */
>> -		nlh = nlmsg_hdr(skb);
>> -		nlh->nlmsg_len = skb->len - NLMSG_HDRLEN;
>> -
>> -		/* queue the netlink packet and poke the kauditd thread */
>> -		skb_queue_tail(&audit_queue, skb);
>> +	/* poke the kauditd thread */
>> +	if (audit_rate_check())
>>  		wake_up_interruptible(&kauditd_wait);
>> -	} else
>> -		audit_log_lost("rate limit exceeded");
> .. here is another case where you've missed/ignored previous feedback.
> I believe this is the second revision in the history of this patchset
> where you've missed feedback; *please* try to do better Casey, stuff like
> this wastes time and drags things out longer than needed.
>
> https://lore.kernel.org/audit/fc242f4c853fee16e587e9c78e1f282e@paul-moore.com

My bad. I've been staring at this patch for over five years and sometimes
can't see where it has, hasn't or needs to be changed. I mucked this up
while pulling the indentation fixes into the separate patch.

>>  	audit_buffer_free(ab);
>>  }
> --
> paul-moore.com
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ