Received-SPF: pass (google.com: domain of linux-kernel+bounces-51313-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 03 Feb 2024 17:25:12 -0500
Message-ID: <1e8f6bbac94d62da330262af871aaa98@paul-moore.com>
Precedence: bulk
MIME-Version: 1.0 
Content-Type: text/plain; charset=utf-8 
Content-Disposition: inline 
Content-Transfer-Encoding: 8bit
From: Paul Moore <paul@paul-moore.com>
To: Fan Wu <wufan@linux.microsoft.com>, corbet@lwn.net, zohar@linux.ibm.com, jmorris@namei.org, serge@hallyn.com, tytso@mit.edu, ebiggers@kernel.org, axboe@kernel.dk, agk@redhat.com, snitzer@kernel.org, eparis@redhat.com
Cc: linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org, linux-security-module@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-block@vger.kernel.org, dm-devel@lists.linux.dev, audit@vger.kernel.org, linux-kernel@vger.kernel.org, Deven Bowers <deven.desai@linux.microsoft.com>, Fan Wu <wufan@linux.microsoft.com>
Subject: Re: [PATCH RFC v12 10/20] ipe: add permissive toggle
References: <1706654228-17180-11-git-send-email-wufan@linux.microsoft.com>
In-Reply-To: <1706654228-17180-11-git-send-email-wufan@linux.microsoft.com>

On Jan 30, 2024 Fan Wu <wufan@linux.microsoft.com> wrote:
> 
> IPE, like SELinux, supports a permissive mode. This mode allows policy
> authors to test and evaluate IPE policy without it effecting their
> programs. When the mode is changed, a 1404 AUDIT_MAC_STATUS
> be reported.
> 
> This patch adds the following audit records:
> 
>     audit: MAC_STATUS enforcing=0 old_enforcing=1 auid=4294967295
>       ses=4294967295 enabled=1 old-enabled=1 lsm=ipe res=1
>     audit: MAC_STATUS enforcing=1 old_enforcing=0 auid=4294967295
>       ses=4294967295 enabled=1 old-enabled=1 lsm=ipe res=1
> 
> The audit record only emit when the value from the user input is
> different from the current enforce value.
> 
> Signed-off-by: Deven Bowers <deven.desai@linux.microsoft.com>
> Signed-off-by: Fan Wu <wufan@linux.microsoft.com>
> ---
> v2:
>   + Split evaluation loop, access control hooks,
>     and evaluation loop from policy parser and userspace
>     interface to pass mailing list character limit
> 
> v3:
>   + Move ipe_load_properties to patch 04.
>   + Remove useless 0-initializations
>   + Prefix extern variables with ipe_
>   + Remove kernel module parameters, as these are
>     exposed through sysctls.
>   + Add more prose to the IPE base config option
>     help text.
>   + Use GFP_KERNEL for audit_log_start.
>   + Remove unnecessary caching system.
>   + Remove comments from headers
>   + Use rcu_access_pointer for rcu-pointer null check
>   + Remove usage of reqprot; use prot only.
>   + Move policy load and activation audit event to 03/12
> 
> v4:
>   + Remove sysctls in favor of securityfs nodes
>   + Re-add kernel module parameters, as these are now
>     exposed through securityfs.
>   + Refactor property audit loop to a separate function.
> 
> v5:
>   + fix minor grammatical errors
>   + do not group rule by curly-brace in audit record,
>     reconstruct the exact rule.
> 
> v6:
>   + No changes
> 
> v7:
>   + Further split lsm creation into a separate commit from the
>     evaluation loop and audit system, for easier review.
>   + Propagating changes to support the new ipe_context structure in the
>     evaluation loop.
>   + Split out permissive functionality into a separate patch for easier
>     review.
>   + Remove permissive switch compile-time configuration option - this
>     is trivial to add later.
> 
> v8:
>   + Remove "IPE" prefix from permissive audit record
>   + align fields to the linux-audit field dictionary. This causes the
>     following fields to change:
>       enforce -> permissive
> 
>   + Remove duplicated information correlated with syscall record, that
>     will always be present in the audit event.
>   + Change audit types:
>     + AUDIT_TRUST_STATUS -> AUDIT_MAC_STATUS
>       + There is no significant difference in meaning between
>         these types.
> 
> v9:
>   + Clean up ipe_context related code
> 
> v10:
>   + Change audit format to comform with the existing format selinux is
>     using
>   + Remove the audit record emission during init to align with selinux,
>     which does not perform this action.
> 
> v11:
>   + Remove redundant code
> 
> v12:
>   + Remove redundant code
> ---
>  security/ipe/audit.c | 27 ++++++++++++++++--
>  security/ipe/audit.h |  1 +
>  security/ipe/eval.c  | 11 +++++--
>  security/ipe/eval.h  |  1 +
>  security/ipe/fs.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/security/ipe/audit.c b/security/ipe/audit.c
> index 79b7af25085c..ed390d32c641 100644
> --- a/security/ipe/audit.c
> +++ b/security/ipe/audit.c
> @@ -27,6 +27,9 @@
>  				    "new_active_pol_version=%hu.%hu.%hu "\
>  				    "new_policy_digest=" IPE_AUDIT_HASH_ALG ":"
>  
> +#define AUDIT_ENFORCE_CHANGE_FMT "enforcing=%d old_enforcing=%d auid=%u ses=%u "\
> +				 "enabled=1 old-enabled=1 lsm=ipe res=1"

It looks like you only ever use this in one place, it would probably be
better to skip the macro definition in this case as it can make it
easier to have a disconnect between the format string and the data.

>  static const char *const audit_op_names[__IPE_OP_MAX + 1] = {
>  	"EXECUTE",
>  	"FIRMWARE",
> @@ -95,8 +98,8 @@ void ipe_audit_match(const struct ipe_eval_ctx *const ctx,
>  	if (!ab)
>  		return;
>  
> -	audit_log_format(ab, "ipe_op=%s ipe_hook=%s pid=%d comm=",
> -			 op, audit_hook_names[ctx->hook],
> +	audit_log_format(ab, "ipe_op=%s ipe_hook=%s enforcing=%d pid=%d comm=",
> +			 op, audit_hook_names[ctx->hook], READ_ONCE(enforce),
>  			 task_tgid_nr(current));
>  	audit_log_untrustedstring(ab, get_task_comm(comm, current));
>  
> @@ -210,3 +213,23 @@ void ipe_audit_policy_load(const struct ipe_policy *const p)
>  
>  	audit_log_end(ab);
>  }
> +
> +/**
> + * ipe_audit_enforce - Audit a change in IPE's enforcement state.
> + * @new_enforce: The new value enforce to be set.
> + * @old_enforce: The old value currently in enforce.
> + */
> +void ipe_audit_enforce(bool new_enforce, bool old_enforce)
> +{
> +	struct audit_buffer *ab;
> +
> +	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_MAC_STATUS);
> +	if (!ab)
> +		return;
> +
> +	audit_log_format(ab, AUDIT_ENFORCE_CHANGE_FMT, new_enforce, old_enforce,
> +			 from_kuid(&init_user_ns, audit_get_loginuid(current)),
> +			 audit_get_sessionid(current));
> +
> +	audit_log_end(ab);
> +}

--
paul-moore.com