openssh bugs - Apr 2004 - [Bug 125] with BSM auditing, cron editing thru ssh session causes cron jobs to fail

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

dtucker at zip.com.au changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|major                       |enhancement





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

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

Lloyd.Parkes at eds.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|enhancement                 |normal





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

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





------- Additional Comments From Lloyd.Parkes at eds.com  2004-04-26 10:43
-------
This bug report is not an enhancement request (IMHO). OpenSSH is simply not
compatible with something that we could reasonably expect it to be compatible
with.



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

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

djm at mindrot.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Severity|normal                      |enhancement



------- Additional Comments From djm at mindrot.org  2004-04-26 10:50 -------
If you want to see this change, then test patches. Don't interfere with bugs
until you have contributed something more than words.



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

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





------- Additional Comments From djm at mindrot.org  2004-04-26 10:51 -------
Created an attachment (id=618)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=618&action=view)
Unpacked patch for commenting




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

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.

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





------- Additional Comments From dtucker at zip.com.au  2004-04-26 11:48 -------
Created an attachment (id=619)
 --> (http://bugzilla.mindrot.org/attachment.cgi?id=619&action=view)
(DO NOT USE) Work-in-progress BSM patch for comment.

I'd like to see the hooks in sshd made generic (kind of a tiny "audit
api"
which any platform could implement as much or as little of as it needs).  For
example, see the implementation of CUSTOM_LOGIN_FAILED (which should be part of
it, BTW).   AIX, at least, has an audit API that could use those generic hooks
too.

Also, instead of lots of little "audit_event_TYPE()" functions, I
think it
should be "audit_event(TYPE)".	This also means less monitor calls
(which would
be tricky for varargs functions?).  Attached is a diff from a local tree where
I've been playing with this, this is for comment only, and has not been
tested.


Also changed bsd-solaris.c -> port-solaris.c.



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

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





------- Additional Comments From djm at mindrot.org  2004-04-26 15:08 -------
Yes, making the audit functionality generic would be nice.

wrt varargs functions: I can't see how to nicely do them through the
monitor,
unless there exist both (...) and (va_list) variants of the same function. My
comments on the patch were more about factoring out common code than modifying
the exposed API.



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

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





------- Additional Comments From dtucker at zip.com.au  2004-04-26 15:20 -------
I've been playing with it some more, and the hooks currently look like this:

enum sshaudit_event_type {
        AUTH_PASSWORD,
        AUTH_PUBKEY,
        AUTH_HOSTBASED,
        LOGIN_INTERACTIVE,
        LOGIN_NONINTERACT,
        NOLOGIN,
        EXCEED_MAXTRIES,
        ROOT_NOT_CONSOLE,
        LOGOUT
};
typedef enum sshaudit_event_type sshaudit_event_t;

void sshaudit_init(Authctxt *);
void sshaudit_connect_from(const char *, int);
void sshaudit_event(sshaudit_event_t);




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