New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: inconsistent usage of current/tsk in audit_filter_rules #82

Closed
WOnder93 opened this Issue May 16, 2018 · 2 comments

Comments

Projects
None yet
1 participant
@WOnder93
Member

WOnder93 commented May 16, 2018

The function audit_filter_rules accepts a struct task_struct *tsk parameter, which can be different from current (representing the currently executing task). However, in some places current is used instead of tsk.

s/current/tsk/:

		case AUDIT_SESSIONID:
			sessionid = audit_get_sessionid(current);
			result = audit_comparator(sessionid, f->op, f->val);
			break;

These two comparisons call the in_group_p/in_egroup_p functions which implicitly use the current variable:

		case AUDIT_GID:
			result = audit_gid_comparator(cred->gid, f->op, f->gid);
			if (f->op == Audit_equal) {
				if (!result)
					result = in_group_p(f->gid);
			} else if (f->op == Audit_not_equal) {
				if (result)
					result = !in_group_p(f->gid);
			}
			break;
		case AUDIT_EGID:
			result = audit_gid_comparator(cred->egid, f->op, f->gid);
			if (f->op == Audit_equal) {
				if (!result)
					result = in_egroup_p(f->gid);
			} else if (f->op == Audit_not_equal) {
				if (result)
					result = !in_egroup_p(f->gid);
			}
			break;

They should be replaced by functions that use the struct cred data from tsk. Since the kernel currently doesn't provide a function that would accept a user provided struct cred *, they either need to be added to include/linux/cred.h and kernel/groups.c or open coded in the audit code (it's just a few lines of code... still it is probably better to add them globally).

Original ML discussion: https://www.redhat.com/archives/linux-audit/2018-May/msg00084.html
Quick link to in_group_p implementation: https://elixir.bootlin.com/linux/v4.17-rc5/source/kernel/groups.c#L219

@WOnder93

This comment has been minimized.

Member

WOnder93 commented May 18, 2018

I submitted patches for this issue here:
https://www.redhat.com/archives/linux-audit/2018-May/msg00095.html
https://www.redhat.com/archives/linux-audit/2018-May/msg00096.html

In the end I just replaced the in_[e]group_p functions with search_groups calls as this is the only part of the in_[e]group_p implementations that wasn't already covered by the main comparison. For more details please see the related discussion in the mailing list.

pcmoore added a commit that referenced this issue May 21, 2018

audit: Fix wrong task in comparison of session ID
The audit_filter_rules() function in auditsc.c compared the session ID
with the credentials of the current task, while it should use the
credentials of the task given to audit_filter_rules() as a parameter
(tsk).

GitHub issue:
#82

Fixes: 8fae477 ("audit: add support for session ID user filter")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
[PM: not user visible, dropped stable]
Signed-off-by: Paul Moore <paul@paul-moore.com>

pcmoore added a commit that referenced this issue Jun 19, 2018

audit: Fix extended comparison of GID/EGID
The audit_filter_rules() function in auditsc.c used the in_[e]group_p()
functions to check GID/EGID match, but these functions use the current
task's credentials, while the comparison should use the credentials of
the task given to audit_filter_rules() as a parameter (tsk).

Note that we can use group_search(cred->group_info, ...) as a
replacement for both in_group_p and in_egroup_p as these functions only
compare the parameter to cred->fsgid/egid and then call group_search.

In fact, the usage of in_group_p was even more incorrect: it compares to
cred->fsgid (which is usually equal to cred->egid) and not cred->gid.

GitHub issue:
#82

Fixes: 37eebe3 ("audit: improve GID/EGID comparation logic")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
@WOnder93

This comment has been minimized.

Member

WOnder93 commented Jun 20, 2018

Fixed upstream in 5b71388 and af85d17.

@WOnder93 WOnder93 closed this Jun 20, 2018

hohoxu pushed a commit to hohoxu/n5kernel that referenced this issue Aug 16, 2018

audit: Fix extended comparison of GID/EGID
The audit_filter_rules() function in auditsc.c used the in_[e]group_p()
functions to check GID/EGID match, but these functions use the current
task's credentials, while the comparison should use the credentials of
the task given to audit_filter_rules() as a parameter (tsk).

Note that we can use group_search(cred->group_info, ...) as a
replacement for both in_group_p and in_egroup_p as these functions only
compare the parameter to cred->fsgid/egid and then call group_search.

In fact, the usage of in_group_p was even more incorrect: it compares to
cred->fsgid (which is usually equal to cred->egid) and not cred->gid.

GitHub issue:
linux-audit/audit-kernel#82

Fixes: 37eebe3 ("audit: improve GID/EGID comparation logic")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Signed-off-by: Paul Moore <paul@paul-moore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment