diff mbox series

libsemanage: Include user name in ROLE_REMOVE audit events

Message ID 20180824111516.16407-1-vmojzis@redhat.com (mailing list archive)
State Not Applicable
Headers show
Series libsemanage: Include user name in ROLE_REMOVE audit events | expand

Commit Message

Vit Mojzis Aug. 24, 2018, 11:15 a.m. UTC 
Use "previous" user name when no new user is available in
semanage_seuser_audit. Otherwise "id=0" is logged instead of
"acct=user_name" ("id=0" is hard coded value).

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1622045
---
 libsemanage/src/seusers_local.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Nicolas Iooss Sept. 5, 2018, 8:01 p.m. UTC | #1 
On Fri, Aug 24, 2018 at 1:16 PM Vit Mojzis <vmojzis@redhat.com> wrote:
>
> Use "previous" user name when no new user is available in
> semanage_seuser_audit. Otherwise "id=0" is logged instead of
> "acct=user_name" ("id=0" is hard coded value).
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1622045

Hi,
Thanks for the patch! Reviewing it took some time because I was quite
unfamiliar with the audit logs generated by semanage and was wondering
where "id=0" come from, and when I tried to use semanage login and
semanage user to get these audit logs, I got surprised by something in
semanage user documentation...

Anyway, for the record, "id=0" comes from the 0 in the call to
audit_log_semanage_message() that occurs in semanage_seuser_audit(),
and according to libaudit source code [1], id is only used when name
is NULL. So your patch looks good to me and I merged it.

Thanks,
Nicolas

[1] https://github.com/linux-audit/audit-userspace/blob/e42602b7b246ae62e7a12e9cd91f0ac37b1b1968/lib/audit_logging.c#L586

> ---
>  libsemanage/src/seusers_local.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
> index 413ebddd..5fbb09e4 100644
> --- a/libsemanage/src/seusers_local.c
> +++ b/libsemanage/src/seusers_local.c
> @@ -71,17 +71,18 @@ static int semanage_seuser_audit(semanage_handle_t * handle,
>         const char *sep = "-";
>         int rc = -1;
>         strcpy(msg, "login");
> +       if (previous) {
> +               name = semanage_seuser_get_name(seuser);
> +               psename = semanage_seuser_get_sename(previous);
> +               pmls = semanage_seuser_get_mlsrange(previous);
> +               proles = semanage_user_roles(handle, psename);
> +       }
>         if (seuser) {
>                 name = semanage_seuser_get_name(seuser);
>                 sename = semanage_seuser_get_sename(seuser);
>                 mls = semanage_seuser_get_mlsrange(seuser);
>                 roles = semanage_user_roles(handle, sename);
>         }
> -       if (previous) {
> -               psename = semanage_seuser_get_sename(previous);
> -               pmls = semanage_seuser_get_mlsrange(previous);
> -               proles = semanage_user_roles(handle, psename);
> -       }
>         if (audit_type != AUDIT_ROLE_REMOVE) {
>                 if (sename && (!psename || strcmp(psename, sename) != 0)) {
>                         strcat(msg,sep);
> --
> 2.14.3
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
Nicolas Iooss Sept. 5, 2018, 8:04 p.m. UTC | #2 
On Wed, Sep 5, 2018 at 10:01 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, Aug 24, 2018 at 1:16 PM Vit Mojzis <vmojzis@redhat.com> wrote:
> >
> > Use "previous" user name when no new user is available in
> > semanage_seuser_audit. Otherwise "id=0" is logged instead of
> > "acct=user_name" ("id=0" is hard coded value).
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1622045
>
> Hi,
> Thanks for the patch! Reviewing it took some time because I was quite
> unfamiliar with the audit logs generated by semanage and was wondering
> where "id=0" come from, and when I tried to use semanage login and
> semanage user to get these audit logs, I got surprised by something in
> semanage user documentation...
>
> Anyway, for the record, "id=0" comes from the 0 in the call to
> audit_log_semanage_message() that occurs in semanage_seuser_audit(),
> and according to libaudit source code [1], id is only used when name
> is NULL. So your patch looks good to me and I merged it.
>
Oops, on a closer inspection it seems that your patch is not so good:
when seuser is NULL, semanage_seuser_get_name(seuser) dereferences a
NULL pointer and crashes. This needed to be name =
semanage_seuser_get_name(previous); Sorry for this, I'll send a fix.

Nicolas

> [1] https://github.com/linux-audit/audit-userspace/blob/e42602b7b246ae62e7a12e9cd91f0ac37b1b1968/lib/audit_logging.c#L586
>
> > ---
> >  libsemanage/src/seusers_local.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
> > index 413ebddd..5fbb09e4 100644
> > --- a/libsemanage/src/seusers_local.c
> > +++ b/libsemanage/src/seusers_local.c
> > @@ -71,17 +71,18 @@ static int semanage_seuser_audit(semanage_handle_t * handle,
> >         const char *sep = "-";
> >         int rc = -1;
> >         strcpy(msg, "login");
> > +       if (previous) {
> > +               name = semanage_seuser_get_name(seuser);
> > +               psename = semanage_seuser_get_sename(previous);
> > +               pmls = semanage_seuser_get_mlsrange(previous);
> > +               proles = semanage_user_roles(handle, psename);
> > +       }
> >         if (seuser) {
> >                 name = semanage_seuser_get_name(seuser);
> >                 sename = semanage_seuser_get_sename(seuser);
> >                 mls = semanage_seuser_get_mlsrange(seuser);
> >                 roles = semanage_user_roles(handle, sename);
> >         }
> > -       if (previous) {
> > -               psename = semanage_seuser_get_sename(previous);
> > -               pmls = semanage_seuser_get_mlsrange(previous);
> > -               proles = semanage_user_roles(handle, psename);
> > -       }
> >         if (audit_type != AUDIT_ROLE_REMOVE) {
> >                 if (sename && (!psename || strcmp(psename, sename) != 0)) {
> >                         strcat(msg,sep);
> > --
> > 2.14.3
> >
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
diff mbox series

Patch

diff --git a/libsemanage/src/seusers_local.c b/libsemanage/src/seusers_local.c
index 413ebddd..5fbb09e4 100644
--- a/libsemanage/src/seusers_local.c
+++ b/libsemanage/src/seusers_local.c
@@ -71,17 +71,18 @@  static int semanage_seuser_audit(semanage_handle_t * handle,
 	const char *sep = "-";
 	int rc = -1;
 	strcpy(msg, "login");
+	if (previous) {
+		name = semanage_seuser_get_name(seuser);
+		psename = semanage_seuser_get_sename(previous);
+		pmls = semanage_seuser_get_mlsrange(previous);
+		proles = semanage_user_roles(handle, psename);
+	}
 	if (seuser) {
 		name = semanage_seuser_get_name(seuser);
 		sename = semanage_seuser_get_sename(seuser);
 		mls = semanage_seuser_get_mlsrange(seuser);
 		roles = semanage_user_roles(handle, sename);
 	}
-	if (previous) {
-		psename = semanage_seuser_get_sename(previous);
-		pmls = semanage_seuser_get_mlsrange(previous);
-		proles = semanage_user_roles(handle, psename);
-	}
 	if (audit_type != AUDIT_ROLE_REMOVE) {
 		if (sename && (!psename || strcmp(psename, sename) != 0)) {
 			strcat(msg,sep);