[GIT PULL] Audit fixes for v5.15 (#1)
by Paul Moore
Hi Linus,
One small audit patch to add a pointer NULL check, please merge for
the next v5.15-rcX release.
Thanks,
-Paul
--
The following changes since commit 67d69e9d1a6c889d98951c1d74b19332ce0565af:
audit: move put_tree() to avoid trim_trees refcount underflow and UAF
(2021-08-24 18:52:36 -0400)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git
tags/audit-pr-20211019
for you to fetch changes up to 6e3ee990c90494561921c756481d0e2125d8b895:
audit: fix possible null-pointer dereference in audit_filter_rules
(2021-10-18 18:27:47 -0400)
----------------------------------------------------------------
audit/stable-5.15 PR 20211019
----------------------------------------------------------------
Gaosheng Cui (1):
audit: fix possible null-pointer dereference in audit_filter_rules
kernel/auditsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--
paul moore
www.paul-moore.com
3 years, 2 months
[PATCH -next,v3 0/2] Audit: fix warning and check priority early
by Gaosheng Cui
v3:
drop the redundant commit message and add a Fixes tag to the first
patch
v2:
audit: fix possible null-pointer dereference in audit_filter_rules
audit: return early if the rule has a lower priority
v1:
audit: return early if the rule has a lower priority
Gaosheng Cui (2):
audit: fix possible null-pointer dereference in audit_filter_rules
audit: return early if the rule has a lower priority
kernel/auditsc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
2.30.0
3 years, 2 months
[PATCH -next,v2 0/2] Audit: fix warning and check priority early
by Gaosheng Cui
v2:
audit: fix possible null-pointer dereference in audit_filter_rules
audit: return early if the rule has a lower priority
v1:
audit: return early if the rule has a lower priority
Gaosheng Cui (2):
audit: fix possible null-pointer dereference in audit_filter_rules
audit: return early if the rule has a lower priority
kernel/auditsc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
--
2.30.0
3 years, 2 months
audit=1 on 'append' and 'linuxefi' lines
by Joe Wulf
On the 'append' and 'linuxefi' lines for their respective isolinux and grub.cfg lines on a working custom RHEL7 ISO I add the 'audit=1' option.I can see in the logs during the anaconda/kickstart boot and build target system process many audit records.
Is there a way to have anaconda/kickstart place those audit records in the chroot'ed /var/log/audit/audit log file for the resultant target system?
Thank you.R,-Joe Wulf
3 years, 2 months
[PATCH -next] audit: return early if the rule has a lower priority
by Gaosheng Cui
It is not necessary for audit_filter_rules() functions to check
audit fileds of the rule with a lower priority, and if we did,
there might be some unintended effects, such as the ctx->ppid
may be changed unexpectedly, so return early if the rule has
a lower priority.
Signed-off-by: Gaosheng Cui <cuigaosheng1(a)huawei.com>
---
kernel/auditsc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 4ba3b8573ff4..eee14dfad0fa 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -470,6 +470,9 @@ static int audit_filter_rules(struct task_struct *tsk,
u32 sid;
unsigned int sessionid;
+ if (ctx && rule->prio <= ctx->prio)
+ return 0;
+
cred = rcu_dereference_check(tsk->cred, tsk == current || task_creation);
for (i = 0; i < rule->field_count; i++) {
@@ -737,8 +740,6 @@ static int audit_filter_rules(struct task_struct *tsk,
}
if (ctx) {
- if (rule->prio <= ctx->prio)
- return 0;
if (rule->filterkey) {
kfree(ctx->filterkey);
ctx->filterkey = kstrdup(rule->filterkey, GFP_ATOMIC);
--
2.30.0
3 years, 2 months
Audit container ID patchset v11 comments
by Paul Moore
Hi Richard and audit aficionados,
Now that the io_uring fixes are in linux-next and no one is screaming
too loudly about it, it's time to turn the focus back to the audit
container ID patchset. Unfortunately I've lost Richard's v11 draft
from my inbox so I'm going to provide my feedback in this mail, broken
up into sections for each patch; hopefully it isn't too painful to
read ...
*** [PATCH ghak90 v11 01/11] audit: collect audit task parameters
- This would come out during the forward-port/rebase, but the io_uring
specific accessors are no longer needed.
- Minor stuff, but if audit_alloc() returns the tsk->audit pointer you
could simplify things a bit, see the the audit_alloc() case below. I
also moved the loginuid/sessionid setting into audit_alloc() so we do
less work in the audit_set_loginuid() case.
static audit_task_info *audit_alloc_task(struct task_struct *tsk)
{
if (!tsk->audit)
tsk->audit = kmem_cache_alloc(audit_task_cache, GFP_KERNEL);
return tsk->audit;
}
int audit_alloc(struct task_struct *tsk)
{
int rc;
struct audit_task_info *ati;
ati = audit_alloc_task(tsk);
if (!ati)
return -ENOMEM;
ati->loginuid = audit_get_loginuid(current);
ati->sessionid = audit_get_sessionid(current);
rc = audit_alloc_syscall(tsk);
if (rc) {
tsk->audit = NULL;
kmem_cache_free(audit_task_cache, ati);
}
return rc;
}
- I'm keeping this comment separate from the one above, although it is
very much related and could simplify things further. Since we are
calling audit_alloc_task() from copy_process (copy_process ->
audit_alloc -> audit_alloc_task) and copy_process will fail to create
the process if audit_alloc()/audit_alloc_task() fails, why not fold
audit_alloc_task() into audit_alloc() and just drop all of the other
audit_alloc_task() calls? Unless the kernel completely fails, all of
the other audit_alloc_task() callers should already have the
current->audit field allocated. Can you think of any case where this
wouldn't work? Am I missing something (maybe in the later patches)?
- Related to the above, if we assume that a valid task_struct *always*
has a task_struct->audit field then there are a number of
task_struct->audit checks in this patchset that can probably be
removed.
*** [PATCH ghak90 v11 02/11] audit: add container id
- Perhaps it gets used more in later patches, but audit_get_contid()
is only used by one caller and it seems like it could be easily
replaced by _audit_contobj_get() and a dereference. Doing so would
shrink/simplify the patch a bit.
- I noticed that the audit_set_contid() function definition comment
block mentions setting the contid on the "current" task, which isn't
always correct, see proc_contid_write() and the fact that it takes an
arbitrary task_struct as a parameter.
- Another minor point, but when injecting the task into an existing
container you can leverage the _audit_contobj_get() return value (and
do some other simplifications), for example:
/* task injection to existing container */
if (current != cont->owner) {
spin_unlock(&_audit_contobj_list_lock);
rc = -ENOTUNIQ;
goto error;
}
newcont = _audit_contobj_get(cont);
- I might change the "error" jump label in audit_set_contid() to "out"
since that code is executed in both the normal and error paths.
- You are calling _audit_contobj_put(oldcont) twice at the end of
audit_set_contid() in the normal/non-error case; this doesn't seem
correct.
- The RCU read lock taken around _audit_contobj_get_bytask() in
audit_alloc_task() could probably be moved inside audit_alloc_task() -
although see my comments for patch #1 - as anyone getting a reference
should probably be holding the RCU lock.
- The locking around and inside of _audit_contobj_put() is
interesting. We should consider moving the spinlock inside the
function similar to the RCU lock and _audit_contobj_get_bytask()
comment above, but I think we also might be able to help limit when
the spinlock is taken as well. There are also some corner cases with
respect to contobj lifetimes and owner references that need
correction. I believe something like this should improve things:
static void audit_contobj_free(struct rcu_head *entry)
{
struct audit_contobj *cont = container_of(entry, struct audit_contobj, rcu);
put_task_struct(cont->owner);
kfree(cont);
}
static void _audit_contobj_put(struct audit_contobj *cont)
{
if (!cont)
return;
if (!refcount_dec_and_lock(&cont->refcount, &_audit_contobj_list_lock))
return;
list_del_rcu(&cont->list);
spin_unlock(&_audit_contobj_list_lock);
call_rcu(&cont->rcu, audit_contobj_free);
}
int audit_set_contid(struct task_struct *tsk, u64 contid)
{
/* ... */
/* NOTE: rcu lock, tasklist lock, and task lock already held */
h = audit_hash_contid(contid);
list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) {
if (refcount_read(&cont->refcount) == 0)
continue;
if (cont->id == contid) {
/* task injection to existing container */
if (current != cont->owner) {
rc = -ENOTUNIQ;
/* cleanup, error handling, etc. */
}
newcont = _audit_contobj_get(cont);
break;
}
}
if (!newcont) {
newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
if (!newcont) {
rc = -ENOMEM;
/* cleanup, error handling, etc. */
}
INIT_LIST_HEAD(&newcont->list);
newcont->id = contid;
newcont->owner = get_task_struct(current);
refcount_set(&newcont->refcount, 1);
spin_lock(&_audit_contobj_list_lock);
list_add_rcu(&newcont->list, &audit_contid_hash[h]);
spin_unlock(&_audit_contobj_list_lock);
}
info->count = newcont;
/* ... */
}
*** [PATCH ghak90 v11 03/11] audit: log container info of syscalls
- It would be good to explain the significance of the "record=" field
in the AUDIT_CONTAINER_ID record in this patch. Currently it is just
an increasing counter with no linkage to anything, if it has meaning
then it should be documented here, if not then we should consider
dropping it.
- It looks like you created audit_contobj_get_bytask() here because
audit_contobj is defined in audit.c; if you defined audit_contobj and
declared the necessary supporting functions in kernel/audit.h these
shenanigans wouldn't be necessary, correct? I suspect this would
improve/eliminate a number of other functions in this patch too.
- Nit picky, but I wonder if audit_contobj_put() would make more sense
named audit_contobj_mput()?
- Why does audit_log_container_id_ctx() take an audit_context
parameter when it always internally references the current task for
the audit container ID? Why not simply take the audit_context from
the current task_struct?
*** [PATCH ghak90 v11 04/11] audit: add contid support for signalling
the audit daemon
- The signal caller reset in audit_free() is likely not what we want
as a task could signal/kill auditd and exit, clearing it's info before
auditd could request the signal caller info. We should be able to
remove the audit_sig_adtsk variable as it is unnecessary.
- I think the only time we should "put" the audit_sig_cid contobj is
when auditd requests the AUDIT_SIGNAL_INFO or AUDIT_SIGNAL_INFO2 info
(clear it in both cases as userspace might not be acid aware).
- Locking and refcounting of the contobj in the signal case is tricky,
we've talked about that in past drafts, and it's complicated here by
some locking that doesn't seem correct in all cases (see my comments
regarding patch #3). Working in the suggestions from patch #3 I end up
with something like this:
static void _audit_contobj_put(struct audit_contobj *cont)
{
if (!cont)
return;
if (!refcount_dec_and_lock(&cont->refcount, &_audit_contobj_list_lock))
return;
list_del_rcu(&cont->list);
spin_unlock(&_audit_contobj_list_lock);
if (refcount_read(&cont->sigcount) == 0)
call_rcu(&cont->rcu, audit_contobj_free);
}
static struct audit_contobj *_audit_contobj_get_sig(void)
{
struct audit_cont_obj *cont = current->audit->cont;
refcount_inc(&cont->sigcount);
return cont;
}
static void _audit_contobj_put_sig(struct audit_contobj *cont)
{
if (refcount_dec_and_test(&cont->sigcount) == 0 &&
refcount_read(&cont->refcount) == 0))
audit_contobj_free(cont->rcu);
}
*** [PATCH ghak90 v11 05/11] audit: add support for non-syscall
auxiliary records
- Porting needed due to the io_uring changes, but it seems okay otherwise.
*** [PATCH ghak90 v11 06/11] audit: add containerid support for user records
- Nothing to say here.
*** [PATCH ghak90 v11 07/11] audit: add containerid filtering
- Likely porting is needed, but okay otherwise.
*** [PATCH ghak90 v11 08/11] audit: add support for containerid to
network namespaces
- While the atomicity of refcount is overkill here, it might be wise
to use refcount_t in place of the integer counter in
audit_contobj_netns so we get all the nice rollover, etc. protections.
- There are some interesting locking choices going on here, but I
think some of the changes in the prior patches are going to have an
impact here so I'm not going to look too hard at that right now, I'll
revisit it in the next revision.
- See my previous comments about the "record=" field, they apply here as well.
*** [PATCH ghak90 v11 09/11] audit: contid check descendancy and nesting
- I realize it's tricky, but the commit description could use a bit
more work to make the relationships between the current task, the
target task, and the audit container ID owner more clear. Examples
are never a bad thing.
- The changes in audit_free() look to be misplaced? No?
*** [PATCH ghak90 v11 10/11] audit: track container nesting
- In the commit description the network namespace example talks about
"... spawning peer tasks 2 and 3 ..." when I think it should read "...
spawning peer tasks in audit container ID 2 and 3 ...".
- Is the change in audit_receive_msg(AUDIT_SIGNAL_INFO2) misplaced?
- AUDIT_MESSAGE_TEXT_MAX in audit_receive_msg() is *really* big, like
multiple pages big on a 4k PAGE_SIZE system, perhaps start with
something like "(AUDIT_MESSAGE_TEXT_MAX < PAGE_SIZE ?
AUDIT_MESSAGE_TEXT_MAX : PAGE_SIZE)"?
- In audit_set_contid() couldn't you use "current" in the call to
_audit_contobj_get_bytask()?
- It looks like you drop the audit_contobj->parent reference in the
signal caller case, but I don't see where you drop it in the normal
case? I suspect it would be in the same place where you drop
audit_contobj->owner.
- Perhaps I've been looking at these patches too long today, but can
you explain why the change to audit_contid_comparator is necessary?
It's not clear to me at the moment.
*** [PATCH ghak90 v11 11/11] audit: add capcontid to set contid
outside init_user_ns
- The access control logic in audit_set_capcontid() doesn't seem to
match the commit description, I was expecting something more like
this:
int audit_set_capcontid(...)
{
/* ... */
rc = -EPERM;
if (tsk == current || !task_is_descendant(current, tsk))
rc = -EXDEV;
else if (current_user_ns() == &init_user_ns &&
capable(CAP_AUDIT_CONTROL))
rc = 0;
else if (audit_get_capcontid(current))
rc = 0;
if (!rc)
info->capcontid = !!enable;
/* ... */
}
--
paul moore
www.paul-moore.com
3 years, 2 months
audit-3.0.6 released
by Steve Grubb
Hello,
I've just released a new version of the audit daemon. It can be
downloaded from http://people.redhat.com/sgrubb/audit. It will also be
in rawhide soon. The ChangeLog is:
- Fixed various issues when dealing with corrupted logs
- Make IPX packet interpretation dependent on the ipx header file existing
- Add b32/b64 support to ausyscall (Egor Ignatov)
- Add support for armv8l (Egor Ignatov)
- Fix auditctl list of syscalls in PPC (Egor Ignatov)
- auditd.service now restarts auditd under some conditions (Timothée Ravier)
The main driver for this release is that there are a scattering of bug
reports of segfaults on the previous release. The auparse library has been
documented for years to fabricate 2 non-existing fields, seresult and seperm.
Somehow, seresult was added to SELINUX_ERR over the years and this was not
noticed. So, when auparse is done with an event and is cleaning up, it thinks
it owns the seresult field and frees it. On the SELINUX_ERR record, it's a
real field that can't be freed and that leads to the segfault. The code doing
cleanup was refactored to not make the decision based on the field's name. The
resulting code should be slightly faster.
SHA256: c3e44d77513a42401d417dd0ceb203cf23886cb89402dea7b9494faa3f4fcc5e
Please let me know if you run across any problems with this release.
-Steve
3 years, 2 months
