Re: [PATCH v2 4/6] Audit: Add record for multiple task security contexts
On March 12, 2025 7:51:36 PM Paul Moore wrote:
On Mar 7, 2025 Casey Schaufler wrote:
...
diff --git a/include/linux/security.h b/include/linux/security.h
index 540894695c4b..79a9bf4a7cdd 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -168,6 +168,7 @@ struct lsm_prop {
extern const char *const lockdown_reasons[LOCKDOWN_CONFIDENTIALITY_MAX+1];
extern u32 lsm_active_cnt;
+extern u32 lsm_subjctx_cnt;
I'm not loving this. More below, but I'd really like to hide some of
this detail inside a LSM API/hook if possible.
Thinking more about this I think we can't go with a LSM_MAX_PROPS, or
similar determined at build time since we have the ability to toggle LSMs
at boot. Need to think on this some more, but the answer is going to have
to be a variable and not a #define.
The LSM init work I'm doing right now directly impacts this, and I'm in the
final stages now. Let me see what looks reasonable and I'll get back to you.
--
paul-moore.com
Re: [PATCH v1] audit,module: restore audit logging in load failure case
On 2025-03-07 14:41, Steve Grubb wrote:
> On Thursday, March 6, 2025 4:41:40 PM Eastern Standard Time Richard Guy
> Briggs wrote:
> > On 2024-10-24 16:41, Paul Moore wrote:
> > > On Oct 23, 2024 Richard Guy Briggs wrote:
> > > > The move of the module sanity check to earlier skipped the audit
> > > > logging
> > > > call in the case of failure and to a place where the previously used
> > > > context is unavailable.
> > > >
> > > > Add an audit logging call for the module loading failure case and get
> > > > the module name when possible.
> > > >
> > > > Link: https://issues.redhat.com/browse/RHEL-52839
> > > > Fixes: 02da2cbab452 ("module: move check_modinfo() early to
> > > > early_mod_check()") Signed-off-by: Richard Guy Briggs
> > > > ---
> > > >
> > > > kernel/module/main.c | 4 +++-
> > > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > > > index 49b9bca9de12..1f482532ef66 100644
> > > > --- a/kernel/module/main.c
> > > > +++ b/kernel/module/main.c
> > > > @@ -3057,8 +3057,10 @@ static int load_module(struct load_info *info,
> > > > const char __user *uargs,> >
> > > > * failures once the proper module was allocated and
> > > > * before that.
> > > > */
> > > >
> > > > - if (!module_allocated)
> > > > + if (!module_allocated) {
> > > > + audit_log_kern_module(info->name ? info->name :
> "(unavailable)");
> > > >
> > > > mod_stat_bump_becoming(info, flags);
> > > >
> > > > + }
> > >
> > > We probably should move the existing audit_log_kern_module() to just
> > > after the elf_validity_cache_copy() call as both info->name and
> > > info->mod->name should be as valid as they are going to get at that
> > > point. If we do that then we only have two cases we need to worry about,
> > > a failed module_sig_check() or a failed elf_validity_cache_copy(), and
> > > in both cases we can use "(unavailable)" without having to check
> > > info->name first.
> >
> > Fair enough.
> >
> > > However, assuming we move the audit_log_kern_module() call up a bit as
> > > described above, I'm not sure there is much value in calling
> > > audit_log_kern_module() with an "(unavailable)" module name in those
> > > early two cases. We know it's an attempted module load based on the
> > > SYSCALL record, seeing an associated "(unavailable)" KERN_MODULE record
> > > doesn't provide us with any more information than if we had simply
> > > skipped the KERN_MODULE record.
> >
> > Understood. I wonder if the absence of the record in the error case
> > will leave us guessing if we lost a record from the event? We will have
> > the error code from the SYSCALL record but not much more than that, and
> > some of those error codes could just as well be generated after that
> > point too. This would be a similar situation to the vanishing fields in
> > an existing record, but is likely easier to mitigate than a
> > non-last-field vanishing or shifting around in an existing record.
> >
> > Steve? Atilla? Any comments?
>
> We should only get the events if we have syscall rules that would result in
> kernel module loading/unloading events. I suppose that by the time audit
> loads it's rules, many modules have already loaded. So, I don't think we need
> to worry about early cases. But when we do get a kernel module load/unload
> event based on the rules, we need it's name - even in failures. We need to be
> able to determine what was attempted since this could affect system
> integrity.
Ok, yes, agreed. But that information isn't available yet for a number
of error cases. It hasn't even been looked for yet after
module_sig_check() and can't be reliably found until halfway through
elf_validity_cache_copy().
So, we should provide the record and make a best effort to fill in that
information if we can. We could try more aggressively to extract it
from the info blob, but I don't know if that is worth the effort because
a number of the steps leading up to it are necessary to check the
validity and set up structures to extract it.
> -Steve
>
> > > Untested, but this is what I'm talking about:
> > >
> > > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > > index 0050ef288ab3..eaa10e3c7eca 100644
> > > --- a/include/linux/audit.h
> > > +++ b/include/linux/audit.h
> > > @@ -417,7 +417,7 @@ extern int __audit_log_bprm_fcaps(struct linux_binprm
> > > *bprm,>
> > > extern void __audit_log_capset(const struct cred *new, const struct cred
> > > *old); extern void __audit_mmap_fd(int fd, int flags);
> > > extern void __audit_openat2_how(struct open_how *how);
> > >
> > > -extern void __audit_log_kern_module(char *name);
> > > +extern void __audit_log_kern_module(const char *name);
> > >
> > > extern void __audit_fanotify(u32 response, struct
> > > fanotify_response_info_audit_rule *friar); extern void
> > > __audit_tk_injoffset(struct timespec64 off
[PATCH v5 0/1] ipe: add errno field to IPE policy load auditing
Hello, When deployment of a new IPE policy fails, there is no audit trail. The failure is written to stderr, but not to the system log. So, users of IPE require a way to identify when and why an operation fails, allowing them to both respond to violations of policy and be notified of potentially malicious actions on their systems with respect to IPE. Previous Postings - v4: https://lore.kernel.org/linux-security-module/1741385035-22090-2-git-send-email-jasjivsi...@linux.microsoft.com/ v3: https://lore.kernel.org/linux-security-module/1740784265-19829-1-git-send-email-jasjivsi...@linux.microsoft.com/ v2: https://lore.kernel.org/linux-security-module/1740696377-3986-1-git-send-email-jasjivsi...@linux.microsoft.com/ v1: https://lore.kernel.org/linux-security-module/1739569319-22015-1-git-send-email-jasjivsi...@linux.microsoft.com/ Changelog - v5: * changed audit log format from AUDIT_POLICY_LOAD_FAIL_FMT to AUDIT_POLICY_LOAD_NULL_FMT. * added success case in IPE policy errno documentation. * removed unnecessary errno documentation in new_policy(), update_policy(), ipe_new_policy() and ipe_update_policy(). * merged success and failed case together in new_policy() for easy maintenance. v4: * added a seperate errno table to IPE AUDIT_IPE_POLICY_LOAD documentation. * fixed error code handling that happens when memdup_user_nul is called in new_policy() and update_policy(). * added additional errno documentation to new_policy(), update_policy(), ipe_new_policy() and ipe_update_policy(). * added ENOKEY and EKEYREJECTED to IPE errno table documentation. v3: * used ERR_PTR(rc) directly rather than assigning to struct ipe_policy. * removed unnecessary var from update_policy(). * removed unnecessary error handling from update_policy(). v2: * added additional IPE audit log information to commit to show the errno case. * changed log format from AUDIT_POLICY_LOAD_NULL_FMT to AUDIT_POLICY_LOAD_FAIL_FMT. * removed unnecessary res var from ipe_audit_policy_load(). * handled security fs failure case in new_policy() and update_policy(). * handled insufficent failure case in new_policy() and update_policy(). Jasjiv Singh (1): ipe: add errno field to IPE policy load auditing Documentation/admin-guide/LSM/ipe.rst | 69 +++ security/ipe/audit.c | 19 ++-- security/ipe/fs.c | 25 ++ security/ipe/policy.c | 17 --- security/ipe/policy_fs.c | 28 --- 5 files changed, 113 insertions(+), 45 deletions(-) -- 2.34.1
[PATCH v5 1/1] ipe: add errno field to IPE policy load auditing
Users of IPE require a way to identify when and why an operation fails, allowing them to both respond to violations of policy and be notified of potentially malicious actions on their systems with respect to IPE. This patch introduces a new error field to the AUDIT_IPE_POLICY_LOAD event to log policy loading failures. Currently, IPE only logs successful policy loads, but not failures. Tracking failures is crucial to detect malicious attempts and ensure a complete audit trail for security events. The new error field will capture the following error codes: * -ENOKEY: Key used to sign the IPE policy not found in the keyring * -ESTALE: Attempting to update an IPE policy with an older version * -EKEYREJECTED: IPE signature verification failed * -ENOENT: Policy was deleted while updating * -EEXIST: Same name policy already deployed * -ERANGE: Policy version number overflow * -EINVAL: Policy version parsing error * -EPERM: Insufficient permission * -ENOMEM: Out of memory (OOM) * -EBADMSG: Policy is invalid Here are some examples of the updated audit record types: AUDIT_IPE_POLICY_LOAD(1422): audit: AUDIT1422 policy_name="Test_Policy" policy_version=0.0.1 policy_digest=sha256:84EFBA8FA71E62AE0A537FAB962F8A2BD1053964C4299DCA 92BFFF4DB82E86D3 auid=1000 ses=3 lsm=ipe res=1 errno=0 The above record shows a new policy has been successfully loaded into the kernel with the policy name, version, and hash with the errno=0. AUDIT_IPE_POLICY_LOAD(1422) with error: audit: AUDIT1422 policy_name=? policy_version=? policy_digest=? auid=1000 ses=3 lsm=ipe res=0 errno=-74 The above record shows a policy load failure due to an invalid policy (-EBADMSG). By adding this error field, we ensure that all policy load attempts, whether successful or failed, are logged, providing a comprehensive audit trail for IPE policy management. Signed-off-by: Jasjiv Singh --- Documentation/admin-guide/LSM/ipe.rst | 69 +++ security/ipe/audit.c | 19 ++-- security/ipe/fs.c | 25 ++ security/ipe/policy.c | 17 --- security/ipe/policy_fs.c | 28 --- 5 files changed, 113 insertions(+), 45 deletions(-) diff --git a/Documentation/admin-guide/LSM/ipe.rst b/Documentation/admin-guide/LSM/ipe.rst index f93a467db628..dc7088451f9d 100644 --- a/Documentation/admin-guide/LSM/ipe.rst +++ b/Documentation/admin-guide/LSM/ipe.rst @@ -423,7 +423,7 @@ Field descriptions: Event Example:: - type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 + type=1422 audit(1653425529.927:53): policy_name="boot_verified" policy_version=0.0.0 policy_digest=sha256:820EEA5B40CA42B51F68962354BA083122A20BB846F26765076DD8EED7B8F4DB auid=4294967295 ses=4294967295 lsm=ipe res=1 errno=0 type=1300 audit(1653425529.927:53): arch=c03e syscall=1 success=yes exit=2567 a0=3 a1=5596fcae1fb0 a2=a07 a3=2 items=0 ppid=184 pid=229 auid=4294967295 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=pts0 ses=4294967295 comm="python3" exe="/usr/bin/python3.10" key=(null) type=1327 audit(1653425529.927:53): PROCTITLE proctitle=707974686F6E3300746573742F6D61696E2E7079002D66002E2E @@ -433,24 +433,55 @@ This record will always be emitted in conjunction with a ``AUDITSYSCALL`` record Field descriptions: -+++---+---+ -| Field | Value Type | Optional? | Description of Value | -+++===+===+ -| policy_name| string | No| The policy_name | -+++---+---+ -| policy_version | string | No| The policy_version | -+++---+---+ -| policy_digest | string | No| The policy hash | -+++---+---+ -| auid | integer| No| The login user ID | -+++---+---+ -| ses| integer| No| The login session ID | -+++---+---+ -| lsm| string | No| The lsm name associated with the event| -+++---+---+ -| res| integer| No
Re: [PATCH v1 1/1] audit: Mark audit_log_vformat() with __printf() attribute
On Wed, Mar 12, 2025 at 04:16:36PM -0400, Paul Moore wrote: > On Wed, Mar 12, 2025 at 4:02 PM Andy Shevchenko > wrote: > > > > audit_log_vformat() is using printf() type of format, and compiler > > is not happy about this: > > > > kernel/audit.c:1978:9: error: function ‘audit_log_vformat’ might be a > > candidate for ‘gnu_printf’ format attribute > > [-Werror=suggest-attribute=format] > > kernel/audit.c:1987:17: error: function ‘audit_log_vformat’ might be a > > candidate for ‘gnu_printf’ format attribute > > [-Werror=suggest-attribute=format] > > > > Fix the compilation errors by adding __printf() attribute. > It would be good to list the compiler version/flags that triggers this > error in the patch description CONFIG_WERROR=y (this is default), gcc (Debian 14.2.0-17) 14.2.0, `make W=1`. > as I've compiled the audit code quite a bit and haven't seen these errors :) Good for you, I have 100% reproducibility of this :-) I'll do a v2 today. -- With Best Regards, Andy Shevchenko
[PATCH v2 1/1] audit: Mark audit_log_vformat() with __printf() attribute
audit_log_vformat() is using printf() type of format, and GCC compiler
(Debian 14.2.0-17) is not happy about this:
kernel/audit.c:1978:9: error: function ‘audit_log_vformat’ might be a candidate
for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
kernel/audit.c:1987:17: error: function ‘audit_log_vformat’ might be a
candidate for ‘gnu_printf’ format attribute [-Werror=suggest-attribute=format]
Fix the compilation errors (`make W=1` when CONFIG_WERROR=y, which is default)
by adding __printf() attribute.
Signed-off-by: Andy Shevchenko
---
v2: added necessary technical information to the commit message (Paul)
kernel/audit.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 5f5bf85bcc90..f365e1bbeac6 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1956,8 +1956,8 @@ static inline int audit_expand(struct audit_buffer *ab,
int extra)
* will be called a second time. Currently, we assume that a printk
* can't format message larger than 1024 bytes, so we don't either.
*/
-static void audit_log_vformat(struct audit_buffer *ab, const char *fmt,
- va_list args)
+static __printf(2, 0)
+void audit_log_vformat(struct audit_buffer *ab, const char *fmt, va_list args)
{
int len, avail;
struct sk_buff *skb;
--
2.47.2
