[Pkg-shadow-devel] audit support

Wed Sep 3 13:08:17 UTC 2008

Hello,

Thanks for your answers.

I found some issues in userdel.c (see attached patch).

In a future version, I will change these 1 and 0 results to
SHADOW_AUDIT_SUCCESS and SHADOW_AUDIT_FAILURE.

On Tue, Sep 02, 2008 at 08:14:59PM -0400, sgrubb at redhat.com wrote:
> On Tuesday 02 September 2008 18:11:01 Nicolas François wrote:
> > IIRC, AUDIT_ADD_GROUP/AUDIT_ADD_USER were not defined in some previous
> > versions of libaudit.
> 
> True. But these defines go way back. All the way to audit-1.0.5. That was Oct 
> 2005.
> 
> 
> > Do you think it might be useful to redefine it to AUDIT_USER_CHAUTHTOK if
> > configure detects such case?
> 
> The problem is that AUDIT_USER_CHAUTHTOK means that the password or 
> credentials was updated. I suppose you could allow that. But its clearly 
> wrong.

OK. Then I will just fail in configure if these constants are not defined.

Note that AUDIT_USER_CHAUTHTOK is still used wrongly in groupmod and
usermod (at least).

For these two, I think I can find the right audit type in some cases, but
in other cases, when there is a change but not addition or removal of an
account, there might be nothing to log (e.g. the current "changing
comment" in usermod.c)

> > I've also added some log in case of *_unlock() failures (this means the
> > system is left in an inconsistent way).
> 
> What the audit system is intended to record is the willful change of user 
> information. If a failure is due to something the user did, we want to record 
> it. If there is a resource problem that the user has no influence over like 
> malloc failing, we do not want to cause the user to be singled out in the 
> audit logs. If however, they are doing something they are not allowed to do, 
> we want it recorded. If we have started modifying files and the admin will 
> have to "undo" or correct something, we want that recorded.
> 
> 
> > Note that "adding SELinux user mapping" in useradd.c is not part of
> > upstream shadow. It might be easier for your later maintenance to change
> > the audit_logger in the patch that introduce this.
> 
> Ok. Any chance of merging that patch too? :)

It's planned, but more complex for me than the audit logging.

> SE Linux introduced the notion 
> of roles around 2 years ago. When a user is created, he needs to be mapped to 
> a default role befpre they ever try to log in. Maybe this can be conditional 
> in configure as --with-seinux? We get beat up sometimes for selinux patches 
> being hard to find and other distros not knowing everything they needed to 
> apply. It helps everybody if its all out in the open.
> 
> 
> > I have some questions regarding the type to use in some cases:
> > (AUDIT_ADD_GROUP, AUDIT_ADD_USER, AUDIT_USER_CHAUTHTOK?)
> >  * "changing user defaults" message in useradd.c?
> 
> I suppose there is AUDIT_USYS_CONFIG. But this is usually associated with hw 
> clock changes. We could change it to that if you want. I was primarily 
> noticing that rpm was adding users & groups to systems and it was not being 
> recorded as such.

This one is just to notify that /etc/default/useradd was changed.
(no changes in the passwd, shadow, group or gshadow databases)
So AUDIT_USYS_CONFIG looks better to me.

> >    AUDIT_USER_CHAUTHTOK or no audit log
> >  * "adding user to group" in useradd
> >    You changed it to AUDIT_ADD_USER, but it is a modification in the group
> >    file.
> 
> I was thinking that was a failure in the act of adding a user. If its adding a 
> user to an existing group, then its not adding a group. If you see a place 
> where it was adding a new group, then we should record that as adding a 
> group. Adding a user to a group is still adding a user - but modifying a 
> group.

OK. I will check. It might be that it will cancel the addition of the
user, and then AUDIT_ADD_USER is the right type.

> >  * gr_unlock() failure in useradd
> >    (i.e. the user was added, and the group file was possibly changed, and
> >    there were a later failure when the group file was unlocked)
> 
> This is sufficient to call adding a user if a group was not added. If a group 
> was added, and it failed we need to record that.

I read the second sentence as "If a group was intended to be added, and it
failed..."
right?

> > Is there a need to report to audit if an error is detected before any
> > changes are committed to the user/group databases (and before the change
> > is reported to audit)?
> 
> Only permission problems or things that are the user's fault. If malloc fails 
> or something beyond the user's control, we don't need to blame them for it 
> until files are starting to be modified. At that point, we need to have a 
> success/fail event since things have been changed and the admin may need to 
> review the state of affairs.
> 
> 
> > Here are some examples:
> >  + *_lock() failures
> >    Those failures should be caused by another account management program
> >    still running.
> 
> Not really if no file has been changed and the lock failure is not due to 
> permission problems.

OK. I will remove the failure logging (unless the intend to add a
group/user was logged before).

> >  + useradd <invalid user name>
> >  + useradd <already existing user>
> 
> Not really. No change to any file was made. If we have an audit message 
> already leave it. If we don't have one, no need to add it since this has been 
> through CAPP and LSPP evals.

OK. No changes.

> One thing I will mention is that long term, i think shadow-utils needs some 
> work consolidating logging and audit.

I fully agree with that.
Would it make sense to change the libaudit logging to:
 * After options processing
   => log the intended actions
      (e.g. useradd will (it depends on the configuration/options)
      probably add an user and a group => "adding user" + "adding group",
      result=1)
      (or should I explicitly log "intent to add user")
 * On failure (there is usually a fail_exit function)
   => log which action failed
      (e.g. "adding user" or "adding group", or both failed, result=0)
 * On success
   => nothing (the intent was logged, an no failures)
      (or should I explicitly log "user added")
 * Log failures that (may) require admin action
   e.g. the user was successfully removed, but we could not remove the
   home directory.

For syslog logs, I would also like to define a logging policy
 * Log changes to system files
 * Log failures that result in or indicate inconsistent system files
   (e.g. failure to unlock /etc/passwd, failure to open /etc/passwd)
(* Log some attempts to change system files)

> Many are identical.

You mean syslog and libaudit receive the same log?
Or some libaudit logs cannot be differentiated?

Should I avoid these two cases?

Best Regards,
-- 
Nekral
-------------- next part --------------
diff -u src/userdel.c src/userdel.c

--- src/userdel.c	2008-09-02 09:03:20.000000000 -0400
+++ src/userdel.c	(copie de travail)
@@ -172,7 +172,7 @@
 #ifdef WITH_AUDIT
 		audit_logger (AUDIT_DEL_USER, Prog,
 			      "deleting user from group", user_name, user_id,
-			      0);
+			      1);
 #endif
 		SYSLOG ((LOG_INFO, "delete `%s' from group `%s'\n",
 			 user_name, ngrp->gr_name));
@@ -272,7 +272,7 @@
 #ifdef WITH_AUDIT
 		audit_logger (AUDIT_DEL_USER, Prog,
 			      "deleting user from shadow group", user_name,
-			      user_id, 0);
+			      user_id, 1);
 #endif
 		SYSLOG ((LOG_INFO, "delete `%s' from shadow group `%s'\n",
 			 user_name, nsgrp->sg_name));
@@ -798,16 +798,13 @@
 			fprintf (stderr,
 				 _("%s: error removing directory %s\n"),
 				 Prog, user_home);
-#ifdef WITH_AUDIT
-			audit_logger (AUDIT_DEL_USER, Prog,
-				      "deleting home directory", user_name,
-				      user_id, 1);
-#endif
 			errors++;
 		}
 #ifdef WITH_AUDIT
+		else {
 		audit_logger (AUDIT_DEL_USER, Prog,
 			      "deleting home directory", user_name, user_id, 1);
+		}
 #endif
 	}
 
