[Bug 125] with BSM auditing, cron editing thru ssh session causes cron jobs to fail

[bugzilla-daemon at mindrot.org]

http://bugzilla.mindrot.org/show_bug.cgi?id=125





------- Additional Comments From djm at mindrot.org  2004-04-26 11:12 -------
(From update of attachment 618)
This is on the list to be fixed before 3.9p1.

>-	if (!allowed_user(pw))
>-		return (NULL);
>+	if (pw != NULL && !allowed_user(pw))
>+		pw = NULL;

These shouldn't be necessary - we take steps to ensure that pw is never NULL,
so these just obscure the real changes.

>+	if (pw != NULL) {
>+		pw = pwcopy(pw);
>+#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
>+		solaris_audit_save_pw(pw);
>+#endif /* BSM */
>+	}
>+	return (pw);

Why do you return pw here? We fake one later for invalid users anyway.

>+#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)

Rather than this slightly verbose test, perhaps you should add:

#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
# define USE_BSD 1
#endif

to defines.h and just do "#ifdef USE_BSM" everywhere.

>+	if (!authenticated) {
>+		PRIVSEP(solaris_audit_bad_pw("public key"));
>+	}
>+#endif /* BSM */

>--- openbsd-compat/Makefile.in~	2004-01-21 01:07:23.000000000 -0500
>+++ openbsd-compat/Makefile.in	2004-03-03 17:37:39.243034000 -0500

Please avoid reformatting the dependancy lists - the changes obscure any real
additions that you make be making. BTW we used to keep the dependancy lists in
a prettier format, but it was too much work to maintain :)

>Index: openbsd-compat/bsd-solaris.c
>--- openbsd-compat/bsd-solaris.c~	2004-03-03 17:37:39.253019000 -0500
>+++ openbsd-compat/bsd-solaris.c	2004-03-03 17:38:15.103435000 -0500
>@@ -0,0 +1,447 @@
>+/*
>+ * Copyright 1988-2002 Sun Microsystems, Inc.  All rights reserved.
>+ * Use is subject to license terms.

What is the lineage of this code? We need to be very careful about importing
code from vendors.

>+	solaris_audit_record(1, gettext("logins disabled by /etc/nologin"),
>+	    AUE_openssh);

I'm not sure whether we will add a dependancy on gettext right now, given that
we don't use it anywhere else.

>+void
>+solaris_audit_logout(void)
>+{
>+	char    textbuf[BSM_TEXTBUFSZ];
>+
>+	(void) snprintf(textbuf, sizeof (textbuf),
>+		gettext("sshd logout %s"), sav_name);
>+
>+	solaris_audit_record(0, textbuf, AUE_logout);
>+}

A lot of this code is pretty repetitive. Perhaps it could be factored out into
a common varargs function. E.g.

void
solaris_write_audit(int what, const char *fmt, ...)
{
	va_list args;
	char textbuf[BSM_TEXTBUFSZ];

	va_start(args, fmt);
	vsnprintf(textbuf, sizeof(textbuf), fmt, args);
	va_end(args);

	solaris_audit_record(0, textbuf, what);
}

Also, in future could you please attach patches directly into bugzilla? It
makes them more easy to review and discuss.




------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.