Bug 125 - add BSM audit support
Summary: add BSM audit support
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: -current
Hardware: Other Solaris
: P2 enhancement
Assignee: OpenSSH Bugzilla mailing list
URL:
Keywords: patch
: 363 (view as bug list)
Depends on:
Blocks: 914
  Show dependency treegraph
 
Reported: 2002-02-28 00:43 AEDT by Andrew Sydelko
Modified: 2005-03-10 09:06 AEDT (History)
8 users (show)

See Also:


Attachments
Update of bug #2 patch to 3.4p1 (9.96 KB, application/octet-stream)
2002-07-14 13:31 AEST, John R. Jackson
no flags Details
Gzip'd tar file with 3.4p1 and 3.5p1 versions of the patch (11.49 KB, application/octet-stream)
2003-01-04 06:32 AEDT, John R. Jackson
no flags Details
Gzip'd tar file with 3.6p1 (and later) patch (17.15 KB, application/octet-stream)
2003-07-10 08:23 AEST, John R. Jackson
no flags Details
Gzip'd tar file with patches and documentation through 3.7.1p1 (20.34 KB, application/octet-stream)
2003-09-20 08:32 AEST, John R. Jackson
no flags Details
Gzip'd tar file with patches and documentation through 3.7.1p2 (21.66 KB, application/octet-stream)
2003-11-15 16:00 AEDT, John R. Jackson
no flags Details
Gzip'd tar file with patches and documentation through 3.8p1 (24.87 KB, application/octet-stream)
2004-03-04 10:33 AEDT, John R. Jackson
no flags Details
Unpacked patch for commenting (29.14 KB, patch)
2004-04-26 10:51 AEST, Damien Miller
no flags Details | Diff
(DO NOT USE) Work-in-progress BSM patch for comment. (29.14 KB, patch)
2004-04-26 11:48 AEST, Darren Tucker
no flags Details | Diff
(DO NOT USE) More work-in-progress for comment. (28.24 KB, patch)
2004-05-31 23:25 AEST, Darren Tucker
no flags Details | Diff
Add intrumentation for audit to sshd (still work in progress). (17.85 KB, patch)
2004-12-20 16:24 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks in patch #753 for BSM auditting (work in progress) (13.77 KB, patch)
2004-12-20 16:26 AEDT, Darren Tucker
no flags Details | Diff
Add intrumentation for audit to sshd (still work in progress). (16.03 KB, patch)
2004-12-20 20:26 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks in patch #753 for BSM auditting (work in progress) (15.98 KB, patch)
2004-12-20 20:28 AEDT, Darren Tucker
no flags Details | Diff
Add audit hooks to sshd (18.10 KB, patch)
2005-01-30 00:46 AEDT, Darren Tucker
no flags Details | Diff
Add audit hooks to sshd (18.80 KB, patch)
2005-01-30 16:24 AEDT, Darren Tucker
no flags Details | Diff
Add audit hooks to sshd. (21.68 KB, patch)
2005-01-30 21:33 AEDT, Darren Tucker
no flags Details | Diff
Add audit hooks to sshd (21.96 KB, patch)
2005-01-31 11:48 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks for BSM auditting (still work in progress) (17.04 KB, patch)
2005-02-03 00:43 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks for BSM auditting (17.03 KB, patch)
2005-02-06 12:40 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks for BSM auditting (17.38 KB, patch)
2005-02-11 17:30 AEDT, Darren Tucker
no flags Details | Diff
Use audit hooks for BSM auditting (14.74 KB, patch)
2005-02-15 19:47 AEDT, Darren Tucker
djm: ok+
Details | Diff
allow audit events earlier (1.65 KB, patch)
2005-03-05 10:12 AEDT, Darren Tucker
no flags Details | Diff
send audit close event earlier (1005 bytes, patch)
2005-03-05 11:35 AEDT, Darren Tucker
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Sydelko 2002-02-28 00:43:21 AEDT
This is to re-open bug #2. With BSM auditing turned on under Solaris,
editing a crontab through an ssh session causes all cron jobs for that
user to fail to run (with a "cron audit problem). Darren Moffat and Sun
supposedly have provided patches to fix this, but they have yet to be
included.

At the very least, can you allow the patches to be available so that we
may fix this fairly mission-critical bug in our network.

Thanks,

--andy.
Comment 1 Kevin Steves 2002-03-07 11:28:42 AEDT
Sun has not yet provided patches for BSM.
Comment 2 John R. Jackson 2002-07-14 13:31:50 AEST
Created attachment 131 [details]
Update of bug #2 patch to 3.4p1
Comment 3 John R. Jackson 2002-07-14 13:38:34 AEST
The above attachment includes an update of the original patch by Darren J.
Moffat at Sun for this problem.  I took his 3.1p1 based patch and:

  * updated it to 3.4p1

  * added autoconf support to auto-detect the need for the extra functions

  * fixed one incredibly minor potential (probably would never happen)
    memory leak

  * added support for earlier versions of Solaris (pre-IPv6, e.g. 2.6)

Note that if BSM is enabled, the code disables (with a warning) the
privilege separation feature.  This is because the audit functions must
be done as root, which is the parent of the two processes, and the data
would not flow back down into the child.  At least, I didn't see any
easy way to do it (but I didn't look all that hard).

If BSM is not enabled, privilege separation will be used (or not) as it
would without the patch.

John R. Jackson, Technical Software Specialist, jrj@purdue.edu
Comment 4 Ben Lindstrom 2002-07-20 03:58:52 AEST
*** Bug 363 has been marked as a duplicate of this bug. ***
Comment 5 Ben Lindstrom 2002-07-20 04:05:42 AEST
One does not just override the sshd_config.  Fail hard if Privsep and BSM are 
used together.
Comment 6 Brian King 2002-11-16 07:20:48 AEDT
One of the suggested work-a-rounds was to set "UseLogin yes" in sshd_config. 
This does not work 100% of the time. SSH clients used in non-interactive modes 
still exhibit the problem.

e.g.:

ssh {hostname} "crontab -l >crontmp ; crontab crontmp"

Will corrupt your cron audit file just the same as if "UseLogin no" was set.
Comment 7 John R. Jackson 2003-01-04 06:29:51 AEDT
The following attachment updates the suggested patch to 3.5p1.

The attachment is a gzip'd tar file.  Once you download it, ungzip it and then
untar it into a temp directory (or use the 'z' option of GNU tar).  Then look at
the README for more information.
Comment 8 John R. Jackson 2003-01-04 06:32:15 AEDT
Created attachment 192 [details]
Gzip'd tar file with 3.4p1 and 3.5p1 versions of the patch
Comment 9 John R. Jackson 2003-07-10 08:23:17 AEST
Created attachment 355 [details]
Gzip'd tar file with 3.6p1 (and later) patch

This version of the patch applies against 3.6p1 (and later).

In addition to being an update to a new release, this version removes the
previous restriction that privilege separation had to be disabled. That feature
may now be on at the same time this patch is operating.

See the README in the tar file for complete details.
Comment 10 John R. Jackson 2003-09-20 08:32:15 AEST
Created attachment 438 [details]
Gzip'd tar file with patches and documentation through 3.7.1p1

This version of the patch applies against 3.7.1p1.

See the README in the tar file for complete details.
Comment 11 Brian King 2003-09-24 00:58:34 AEST
I've tried applying the latest patch to 3.7.1p2.
It doesn't seem to compile.

Undefined                       first referenced
 symbol                             in file
solaris_audit_success               sshd.o
solaris_audit_save_name             auth.o
solaris_audit_nologin               session.o
solaris_audit_save_port             sshd.o
solaris_audit_maxtrys               auth1.o
solaris_audit_logout                sshlogin.o
solaris_audit_not_console           auth1.o
solaris_audit_save_command          session.o
solaris_audit_save_pw               auth.o
solaris_audit_bad_pw                auth1.o
solaris_audit_save_ttyn             session.o
solaris_audit_save_host             sshd.o
ld: fatal: Symbol referencing errors. No output written to sshd
collect2: ld returned 1 exit status
make: *** [sshd] Error 1

I've looked for the symbols in all the libraries in /lib and can't find those. 
I assumed they should have been in /lib/libbsm.so, but none of those symbols 
contain the "solaris_" prefix. Most have _similar_ sounding symbols in the 
library, but some do not. Am I missing a required library?
Comment 12 Darren Tucker 2003-09-24 20:25:24 AEST
Try deleting openbsd-compat/*.a (or better yet, do "make clean") and rebuilding.
Comment 13 Brian King 2003-09-24 22:29:20 AEST
Found it. The patch to "openbsd-compat/Makefile.in" failed (I guess because of 
differences between p1 & p2). I manually added the "bsd-solaris.o" to the 
COMPAT line, and it compiled fine.

Comment 14 John R. Jackson 2003-11-15 16:00:37 AEDT
Created attachment 500 [details]
Gzip'd tar file with patches and documentation through 3.7.1p2

Gzip'd tar file with patches and documentation through 3.7.1p2

This version of the patch applies against 3.7.1p2. Very minor changes
from the 3.7.1p1 version.  Regenerated the patch so no fuzz or offset
warnings.

See the README in the tar file for complete details.
Comment 15 John R. Jackson 2004-03-04 10:33:53 AEDT
Created attachment 560 [details]
Gzip'd tar file with patches and documentation through 3.8p1

This version of the patch applies against 3.8p1. Very minor changes
from the 3.7.1p2 version.  Regenerated the patch so no fuzz or offset
warnings.

See the README in the tar file for complete details.
Comment 16 Lloyd Parkes 2004-04-26 10:43:40 AEST
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.
Comment 17 Damien Miller 2004-04-26 10:50:53 AEST
If you want to see this change, then test patches. Don't interfere with bugs
until you have contributed something more than words.
Comment 18 Damien Miller 2004-04-26 10:51:46 AEST
Created attachment 618 [details]
Unpacked patch for commenting
Comment 19 Damien Miller 2004-04-26 11:12:45 AEST
Comment on attachment 618 [details]
Unpacked patch for commenting

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.
Comment 20 Darren Tucker 2004-04-26 11:48:04 AEST
Created attachment 619 [details]
(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.
Comment 21 Damien Miller 2004-04-26 15:08:33 AEST
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.
Comment 22 Darren Tucker 2004-04-26 15:20:16 AEST
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);
Comment 23 Darren Tucker 2004-05-31 23:25:29 AEST
Created attachment 647 [details]
(DO NOT USE) More work-in-progress for comment.

More work on the sshd hooks, the implementation in port-solaris.c is still
incomplete.  I'm interested on feedback about (a) whether or not the interface
(see sshaudit.h) is sane and (b) if it's adequate for other systems requiring
this kind of intrumentation.
Comment 24 Damien Miller 2004-06-01 08:36:57 AEST
Comment on attachment 647 [details]
(DO NOT USE) More work-in-progress for comment.

>+#ifdef AUDIT_EVENTS
>+			PRIVSEP(audit_event(LOGIN_EXCEED_MAXTRIES));
>+#endif

How about removing the #ifdefs and just making audit_event a no-op for the
non-audit case?

>Index: sshaudit.h
>===================================================================
>RCS file: sshaudit.h
>diff -N sshaudit.h
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ sshaudit.h	31 May 2004 12:10:20 -0000
>@@ -0,0 +1,22 @@
>+#include "auth.h"
>+
>+#ifndef SSHAUDIT_H
>+# define SSHAUDIT_H
>+enum audit_event_type {
>+	AUTH_FAILED,	/* ? */
>+        LOGIN_SUCCESS,
>+	LOGIN_EXCEED_MAXTRIES,
>+	LOGIN_FAIL_BADPW,
>+	ILLEGAL_USER,
>+	ROOT_NOT_CONSOLE,
>+        NOLOGIN,
>+        LOGOUT
>+};

indenting looks wonky here

>+#if defined(HAVE_GETAUDIT_ADDR)
>+	(void) aug_get_machine(sav_host, &sav_machine[0], &sav_iptype);
>+	debug3("BSM audit: sav_iptype=%ld", (long)sav_iptype);
>+#else
>+	ia = inet_addr(host);
>+	memcpy(&sav_machine[0], &ia, sizeof(sav_machine[0]));
>+	sav_iptype = 0;			/* not used, but just in case */
>+#endif

How does this cope with INET6 addresses? or is this code path only taken when
the machine is old ipv4 API only?
Comment 25 Darren Tucker 2004-08-17 18:58:31 AEST
Sorry folks.  I had the best of intentions but it's going to miss 3.9.
Comment 26 Darren Tucker 2004-12-20 16:24:32 AEDT
Created attachment 753 [details]
Add intrumentation for audit to sshd (still work in progress).
Comment 27 Darren Tucker 2004-12-20 16:26:21 AEDT
Created attachment 754 [details]
Use audit hooks in patch #753 for BSM auditting (work in progress)
Comment 28 Darren Tucker 2004-12-20 18:53:07 AEDT
Comment on attachment 618 [details]
Unpacked patch for commenting

Do these two audit calls in session.c work when privsep is enabled?  It seems
they're called after the privilege drop in that case... (or does BSM do
something clever like caching a descriptor so privilege is not required?)

> 		fclose(f);
>+#if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
>+		solaris_audit_nologin();
>+#endif /* BSM */


>+# if defined(HAVE_BSM_AUDIT_H) && defined(HAVE_LIBBSM)
>+		if (command != NULL)
>+			solaris_audit_save_command(command);
>+# endif /* BSM */
> 		do_setusercontext(pw);
> #endif /* HAVE_OSF_SIA */
Comment 29 Darren Tucker 2004-12-20 20:26:19 AEDT
Created attachment 755 [details]
Add intrumentation for audit to sshd (still work in progress).
Comment 30 Darren Tucker 2004-12-20 20:28:00 AEDT
Created attachment 756 [details]
Use audit hooks in patch #753 for BSM auditting (work in progress)

Relocate some audit calls so they're called from the monitor where possible.
Comment 31 Darren Tucker 2005-01-30 00:46:07 AEDT
Created attachment 793 [details]
Add audit hooks to sshd

.fixes the problem of lack of privilege in the nologin and save_command events.
 The nologin handling is ugly though.
Comment 32 Darren Tucker 2005-01-30 16:24:04 AEDT
Created attachment 794 [details]
Add audit hooks to sshd

OK, I think this one is ready.	(Don't get excited yet folks, it's just the
hooks at this stage.)

I dropped the /etc/nologin handling because it was ugly.  With a little
restructuring to do_nologin it can be done cleanly, but it can wait.

Things that ought to be looked at in this patch:

 - the audit hooks in the monitor are enabled unconditionally post-auth. 
audit_event() is pretty harmless, but audit_run_command takes a string.

 - should audit_run_command and/or the monitor do sanity checking (strnvis? 
enforce a max length?)
Comment 33 Darren Tucker 2005-01-30 21:33:45 AEDT
Created attachment 795 [details]
Add audit hooks to sshd.

Now separates authentication and session events, since one SSH2 connection may
carry multiple sessions.  (eg "ssh -N" will record a successful authentication
but zero sessions.)

Adds some more auth types (hostbased, gssapi).	Adds comments (so it must be
done!)
Comment 34 Darren Tucker 2005-01-31 11:48:26 AEDT
Created attachment 796 [details]
Add audit hooks to sshd

audit_cleanup() has been replaced with the CONNECTION_CLOSE and
CONNECTION_ABANDON events.  Other minor cleanups.

Note that the hooks are (well, should be) now all privsep-aware, so once it's
ported the BSM audit module ought to work fine with privsep.

Now, some questions for the BSM cognoscenti:

- is there a limit on the size of the comand that can be written to the audit
log and if so, what?

- why does the original patch save the tty in sav_tty and then not use it?

- how does BSM differentiate between authentication events and session events?
eg the SSH2 protocol allows zero, one or many sessions (ie shells or commands)
to be associated with a single authentication (ie SSH connection).  At the
moment, the audit hooks differentiate between a session (ie pty allocated) and
a command (no pty allocated).  The original patch seemed to mix these two (it
will write a single login event after authentication but a logout event at
every session close).

- is there a reference on the format of the audit records?  the au_* man pages
seem to cover *how* to write them but not *what* to write.
Comment 35 Damien Miller 2005-02-02 15:46:21 AEDT
Comment on attachment 796 [details]
Add audit hooks to sshd

>+/* helper to return supplied username */
>+static const char *
>+audit_username(void)
>+{
>+	static const char unknownuser[] = "(unknown user)";
>+
>+	if (the_authctxt == NULL || the_authctxt->user == NULL)
>+		return (unknownuser);
>+	return (the_authctxt->user);

What about when !authctxt->valid, shouldn't it return "Invalid User" or
something to prevent leakage of mistyped passwords into logs?

>+void
>+audit_connection_from(const char *host, int port)
>+{
>+	debug("%s: euid %d connection from %s port %d", __func__, geteuid(),
>+	    host, port);
>+}

Remember: __func__ is verboten :)

>+/*
>+ * Called when various events occur (see audit.h for a list of possible
>+ * events and what they mean).
>+ */
>+void
>+audit_event(ssh_audit_event_t event)
>+{
>+	char *eventstr[] = {
>+		"LOGIN_EXCEED_MAXTRIES",
>+		"LOGIN_ROOT_DENIED",
>+		"AUTH_SUCCESS",
>+		"AUTH_FAIL_NONE",
>+		"AUTH_FAIL_PASSWD",
>+		"AUTH_FAIL_KBDINT",
>+		"AUTH_FAIL_PUBKEY",
>+		"AUTH_FAIL_HOSTBASED",
>+		"AUTH_FAIL_GSSAPI",
>+		"INVALID_USER",
>+		"NOLOGIN",
>+		"CONNECTION_CLOSE",
>+		"CONNECTION_ABANDON",
>+		"AUDIT_UNKNOWN"
>+	};

Rather than maintaining this list and the mapping for auth method names earlier
in this file, would it be nicer to whack them all into an array-of-struct (int,
char*, char*) and provide lookup functions?

Looks OK otherwise
Comment 36 Darren Tucker 2005-02-03 00:25:35 AEDT
Patch #796 has been committed so the hooks are in.  Will attach my current
working patch for the BSM part shortly.
Comment 37 Darren Tucker 2005-02-03 00:43:56 AEDT
Created attachment 800 [details]
Use audit hooks for BSM auditting (still work in progress)
Comment 38 Darren Tucker 2005-02-06 12:40:07 AEDT
Created attachment 804 [details]
Use audit hooks for BSM auditting

I think this is ready to start testing.  I have put up a snapshot with the
patch applied at:
http://www.zip.com.au/~dtucker/tmp/openssh-audit-bsm.tar.gz

There's some code in the patch #if'ed out.  I think the code in question should
be removed but it's left there for discussion.

Remaining issues:

- what is the correct way to construct the device identifier part of Terminal
ID?  The telnet events seem to use something other than a source port.

- what is the value of logging the command supplied to sshd?  It seems to be an
attempt to mimic AUE_rshd but it's not equivalent since there may be zero, one
or many command sessions supplied in a given sshd session.

  Would this not be better handled by using the built-in "ex" class? Appending
it as text token to the logout event seems wrong for a couple of reasons:
  - it'll only ever record the last command supplied
  - by my read the text tokens are limited to 255 bytes in length (or 127 if
the "length" field is unsigned, the docs don't say).

  If it's really required then should it not be a separate event number?

- why does the patch call GetAuditFunc(&now, sizeof (now))?  AFAICT the "now"
struct is never used after being populated.

- why does the original patch save the tty name?  AFAICT it's never used.

- should all of the au_* functions have their return codes checked, or is
checking au_close() adequate?

- what values should be specified with the return token?  praudit seems to
interpret the "process error" as an errno.  Are these just picked arbitrarily
by the application, with zero as success?  I noticed that later patches try to
use error numbers from 240 - 255, outside of the errno range, is this
advisable?  And are these expected to be stable for a given application (ie
sshd)?
Comment 39 Phil Dibowitz 2005-02-06 13:33:12 AEDT
For what it's worth, the security team at USC will begin testing this as a
replacement for are inferior in-house auditd patch around the last week in
February - we've been pushed back a bit.

-Phil
Comment 40 Darren Tucker 2005-02-11 17:30:17 AEDT
Created attachment 820 [details]
Use audit hooks for BSM auditting

Update to match recent changes in the audit interface in sshd.

Note that the argument to enable it has changed, use
$ ./configure --with-audit=bsm [other flags]

A patched snapshot tarball is also available at:
http://www.zip.com.au/~dtucker/tmp/openssh-audit-bsm.tar.gz

Anyone interested in seeing this in the next release really should test this...
Comment 41 Damien Miller 2005-02-14 12:10:50 AEDT
Comment on attachment 820 [details]
Use audit hooks for BSM auditting

>Index: audit-bsm.c
...
>+/*
>+ * Copyright 1988-2002 Sun Microsystems, Inc.  All rights reserved.
>+ * Use is subject to license terms.

If you have made substantial changes to this file, you should assert copyright
too.

>+#ifndef HAVE_GETTEXT
>+# define gettext(a)	(a)
>+#endif

Is this necessary for auditing? Can we just scrub out the couple of gettext
references? We don't internationalise any other messages from sshd...

>Index: audit-bsm.h
...
>+#include "includes.h"
>+#ifdef USE_BSM_AUDIT
>+
>+#ifndef AUE_openssh
>+# define AUE_openssh     32800
>+#endif
>+#include <bsm/audit.h>
>+#include <bsm/libbsm.h>
>+#include <bsm/audit_uevents.h>
>+#include <bsm/audit_record.h>
>+#include <locale.h>
>+
>+#if defined(HAVE_GETAUDIT_ADDR)
>+#define	AuditInfoStruct		auditinfo_addr
>+#define AuditInfoTermID		au_tid_addr_t
>+#define GetAuditFunc(a,b)	getaudit_addr((a),(b))
>+#define GetAuditFuncText	"getaudit_addr"
>+#define SetAuditFunc(a,b)	setaudit_addr((a),(b))
>+#define SetAuditFuncText	"setaudit_addr"
>+#define AUToSubjectFunc		au_to_subject_ex
>+#define AUToReturnFunc(a,b)	au_to_return32((a), (int32_t)(b))
>+#else
>+#define	AuditInfoStruct		auditinfo
>+#define AuditInfoTermID		au_tid_t
>+#define GetAuditFunc(a,b)	getaudit(a)
>+#define GetAuditFuncText	"getaudit"
>+#define SetAuditFunc(a,b)	setaudit(a)
>+#define SetAuditFuncText	"setaudit"
>+#define AUToSubjectFunc		au_to_subject
>+#define AUToReturnFunc(a,b)	au_to_return((a), (u_int)(b))
>+#endif
>+
>+extern int	cannot_audit(int);
>+extern void	aug_init(void);
>+extern dev_t	aug_get_port(void);
>+extern int 	aug_get_machine(char *, u_int32_t *, u_int32_t *);
>+extern void	aug_save_auid(au_id_t);
>+extern void	aug_save_uid(uid_t);
>+extern void	aug_save_euid(uid_t);
>+extern void	aug_save_gid(gid_t);
>+extern void	aug_save_egid(gid_t);
>+extern void	aug_save_pid(pid_t);
>+extern void	aug_save_asid(au_asid_t);
>+extern void	aug_save_tid(dev_t, unsigned int);
>+extern void	aug_save_tid_ex(dev_t, u_int32_t *, u_int32_t);
>+extern int	aug_save_me(void);
>+extern int	aug_save_namask(void);
>+extern void	aug_save_event(au_event_t);
>+extern void	aug_save_sorf(int);
>+extern void	aug_save_text(char *);
>+extern void	aug_save_text1(char *);
>+extern void	aug_save_text2(char *);
>+extern void	aug_save_na(int);
>+extern void	aug_save_user(char *);
>+extern void	aug_save_path(char *);
>+extern int	aug_save_policy(void);
>+extern void	aug_save_afunc(int (*)(int));
>+extern int	aug_audit(void);
>+extern int	aug_na_selected(void);
>+extern int	aug_selected(void);
>+extern int	aug_daemon_session(void);

Wouldn't most of this stuff be better off living in audit-bsm.c? It isn't used
elsewhere in the tree.

>Index: configure.ac
...
>+		# These are optional
>+		AC_CHECK_FUNCS(getaudit_addr gettext)

Ditto comment about gettext above.
Comment 42 Darren Tucker 2005-02-15 19:32:11 AEDT
(In reply to comment #41)
> If you have made substantial changes to this file, you should assert copyright
> too.

My main contribution appears to be repeated use of the "d" key :-)

> Is this necessary for auditing? Can we just scrub out the couple of gettext
> references? We don't internationalise any other messages from sshd...

I don't know if it's required but it was in the original patches.  I just made
it optional.

> >Index: audit-bsm.h
[...]
> Wouldn't most of this stuff be better off living in audit-bsm.c? It isn't used
> elsewhere in the tree.

The original idea was to move the OS interface out of the way so I could
concentrate on the code.  It can go back into audit-bsm.c.
Comment 43 Darren Tucker 2005-02-15 19:47:13 AEDT
Created attachment 826 [details]
Use audit hooks for BSM auditting

Update with djm's feedback.  Also removed all of the #ifdef'ed out code.
Comment 44 Damien Miller 2005-02-15 20:22:58 AEDT
Comment on attachment 826 [details]
Use audit hooks for BSM auditting

>Index: LICENCE
>===================================================================
>RCS file: /usr/local/src/security/openssh/cvs/openssh_cvs/LICENCE,v
>retrieving revision 1.17
>diff -u -p -r1.17 LICENCE
>--- LICENCE	5 Nov 2004 09:00:03 -0000	1.17
>+++ LICENCE	30 Jan 2005 12:15:38 -0000
>@@ -203,6 +203,7 @@ OpenSSH contains no GPL code.
> 	Wayne Schroeder
> 	William Jones
> 	Darren Tucker
>+	Sun Microsystems
> 
>      * Redistribution and use in source and binary forms, with or without
>      * modification, are permitted provided that the following conditions
>Index: Makefile.in
>===================================================================
>RCS file: /usr/local/src/security/openssh/cvs/openssh_cvs/Makefile.in,v
>retrieving revision 1.268
>diff -u -p -r1.268 Makefile.in
>--- Makefile.in	2 Feb 2005 13:20:53 -0000	1.268
>+++ Makefile.in	2 Feb 2005 13:27:40 -0000
>@@ -85,7 +85,8 @@ SSHDOBJS=sshd.o auth-rhosts.o auth-passw
> 	monitor_mm.o monitor.o monitor_wrap.o kexdhs.o kexgexs.o \
> 	auth-krb5.o \
> 	auth2-gss.o gss-serv.o gss-serv-krb5.o \
>-	loginrec.o auth-pam.o auth-shadow.o auth-sia.o md5crypt.o audit.o
>+	loginrec.o auth-pam.o auth-shadow.o auth-sia.o md5crypt.o \
>+	audit.o audit-bsm.o
> 
> MANPAGES	= scp.1.out ssh-add.1.out ssh-agent.1.out ssh-keygen.1.out ssh-keyscan.1.out ssh.1.out sshd.8.out sftp-server.8.out sftp.1.out ssh-rand-helper.8.out ssh-keysign.8.out sshd_config.5.out ssh_config.5.out
> MANPAGES_IN	= scp.1 ssh-add.1 ssh-agent.1 ssh-keygen.1 ssh-keyscan.1 ssh.1 sshd.8 sftp-server.8 sftp.1 ssh-rand-helper.8 ssh-keysign.8 sshd_config.5 ssh_config.5
>Index: README.platform
>===================================================================
>RCS file: /usr/local/src/security/openssh/cvs/openssh_cvs/README.platform,v
>retrieving revision 1.2
>diff -u -p -r1.2 README.platform
>--- README.platform	23 Apr 2004 08:57:13 -0000	1.2
>+++ README.platform	30 Jan 2005 12:15:38 -0000
>@@ -23,8 +23,20 @@ openssl-devel, zlib, minres, minires-dev
> 
> Solaris
> -------
>-Currently, sshd does not support BSM auditting.  This can show up as errors
>-when editting cron entries via crontab.  See.
>-http://bugzilla.mindrot.org/show_bug.cgi?id=125
>+If you enable BSM auditing on Solaris, you need to update audit_event(4)
>+for praudit(1m) to give sensible output.  The following line needs to be
>+added to /etc/security/audit_event:
>+
>+	32800:AUE_openssh:OpenSSH login:lo
>+
>+If the contrib/buildpkg.sh script is used, the included postinstall
>+script will add the line for you.
>+
>+The BSM audit event range available for third party TCB applications is
>+32768 - 65535.  Event number 32800 has been choosen for AUE_openssh.
>+There is no official registry of 3rd party event numbers, so if this
>+number is already in use on your system, you may change it at build time
>+by configure'ing --with-cflags=-DAUE_openssh=32801 then rebuilding.
>+
> 
> $Id: README.platform,v 1.2 2004/04/23 08:57:13 dtucker Exp $
>Index: audit-bsm.c
>===================================================================
>RCS file: audit-bsm.c
>diff -N audit-bsm.c
>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ audit-bsm.c	15 Feb 2005 08:41:13 -0000
>@@ -0,0 +1,329 @@
>+/* $Id$ */
>+
>+/*
>+ * TODO
>+ *
>+ * - deal with overlap between this and sys_auth_allowed_user
>+ *   sys_auth_record_login and record_failed_login.
>+ */
>+
>+/*
>+ * Copyright 1988-2002 Sun Microsystems, Inc.  All rights reserved.
>+ * Use is subject to license terms.
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *    notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *    notice, this list of conditions and the following disclaimer in the
>+ *    documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
>+ * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
>+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
>+ * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
>+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
>+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
>+ * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>+ *
>+ */
>+/* #pragma ident	"@(#)bsmaudit.c	1.1	01/09/17 SMI" */
>+
>+#include "includes.h"
>+#if defined(USE_BSM_AUDIT)
>+
>+#include "ssh.h"
>+#include "log.h"
>+#include "auth.h"
>+#include "xmalloc.h"
>+
>+#ifndef AUE_openssh
>+# define AUE_openssh     32800
>+#endif
>+#include <bsm/audit.h>
>+#include <bsm/libbsm.h>
>+#include <bsm/audit_uevents.h>
>+#include <bsm/audit_record.h>
>+#include <locale.h>
>+
>+#if defined(HAVE_GETAUDIT_ADDR)
>+#define	AuditInfoStruct		auditinfo_addr
>+#define AuditInfoTermID		au_tid_addr_t
>+#define GetAuditFunc(a,b)	getaudit_addr((a),(b))
>+#define GetAuditFuncText	"getaudit_addr"
>+#define SetAuditFunc(a,b)	setaudit_addr((a),(b))
>+#define SetAuditFuncText	"setaudit_addr"
>+#define AUToSubjectFunc		au_to_subject_ex
>+#define AUToReturnFunc(a,b)	au_to_return32((a), (int32_t)(b))
>+#else
>+#define	AuditInfoStruct		auditinfo
>+#define AuditInfoTermID		au_tid_t
>+#define GetAuditFunc(a,b)	getaudit(a)
>+#define GetAuditFuncText	"getaudit"
>+#define SetAuditFunc(a,b)	setaudit(a)
>+#define SetAuditFuncText	"setaudit"
>+#define AUToSubjectFunc		au_to_subject
>+#define AUToReturnFunc(a,b)	au_to_return((a), (u_int)(b))
>+#endif
>+
>+extern int	cannot_audit(int);
>+extern void	aug_init(void);
>+extern dev_t	aug_get_port(void);
>+extern int 	aug_get_machine(char *, u_int32_t *, u_int32_t *);
>+extern void	aug_save_auid(au_id_t);
>+extern void	aug_save_uid(uid_t);
>+extern void	aug_save_euid(uid_t);
>+extern void	aug_save_gid(gid_t);
>+extern void	aug_save_egid(gid_t);
>+extern void	aug_save_pid(pid_t);
>+extern void	aug_save_asid(au_asid_t);
>+extern void	aug_save_tid(dev_t, unsigned int);
>+extern void	aug_save_tid_ex(dev_t, u_int32_t *, u_int32_t);
>+extern int	aug_save_me(void);
>+extern int	aug_save_namask(void);
>+extern void	aug_save_event(au_event_t);
>+extern void	aug_save_sorf(int);
>+extern void	aug_save_text(char *);
>+extern void	aug_save_text1(char *);
>+extern void	aug_save_text2(char *);
>+extern void	aug_save_na(int);
>+extern void	aug_save_user(char *);
>+extern void	aug_save_path(char *);
>+extern int	aug_save_policy(void);
>+extern void	aug_save_afunc(int (*)(int));
>+extern int	aug_audit(void);
>+extern int	aug_na_selected(void);
>+extern int	aug_selected(void);
>+extern int	aug_daemon_session(void);
>+
>+#ifndef HAVE_GETTEXT
>+# define gettext(a)	(a)
>+#endif
>+
>+extern Authctxt *the_authctxt;
>+static AuditInfoTermID ssh_bsm_tid;
>+
>+/* Below is the low-level BSM interface code */
>+
>+/*
>+ * Check if the specified event is selected (enabled) for auditting.

s/auditting/auditing/

I think configure should print a "read the README.bsm" or something if BSM is
enabled.

Otherwise OK.
Comment 45 Darren Tucker 2005-02-23 21:58:46 AEDT
Thanks, this has now been applied and it's in the snaps:
ftp://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/snapshot/

The package stuff needs to be re-done.
Comment 46 Phil Dibowitz 2005-02-25 13:09:53 AEDT
So we've done some internal testing with the latest snapshot over the last two
days, and things look good. It's not a thorough test, but the logging is as we
would expect, and everything else looks as expected.

A huge thanks to all the people who helped with this.
Comment 47 Darren Tucker 2005-03-05 10:12:08 AEDT
Created attachment 845 [details]
allow audit events earlier

Remaining issues:
1) Unpermitted monitor requests under some conditions.	Believed fixed by the
attached patch, awaiting confirmation.	(reported by Phil Dibowitz):
fatal: monitor_read: unpermitted request 56	   
fatal: mm_request_send: write

2) Another error reported by Matt Goebel: on disconnect without successful
authentication.  Similar to above but does not have the "unpermitted request". 
Not fixed by attached patch, so is either a different issue to above, or is and
the above does not fix it:
fatal: mm_request_send: write

3) If GNU libintl is found by configure then the library must be added by hand
to the LIBS list.  Also reported by Matt Goebel.
(either fix configure or remove gettext() from the BSM code).
Comment 48 Phil Dibowitz 2005-03-05 10:41:10 AEDT
I was in the midst of a campus-wide upgrade to 3.9 from 3.8. I'm recompiling the
test version with the patch now, and I'll report back shortly.

- Phil
Comment 49 Darren Tucker 2005-03-05 11:35:35 AEDT
Created attachment 846 [details]
send audit close event earlier

I think this will fix item 2 from comment #47.
Comment 50 Phil Dibowitz 2005-03-05 13:02:11 AEDT
Yup, the patch does solve my issue (issue #1). Thanks.

- Phil
Comment 51 Darren Tucker 2005-03-06 23:06:04 AEDT
I've committed patches #845 and #846, they'll be in tomorrow's snaps.
Comment 52 Darren Tucker 2005-03-07 16:16:47 AEDT
This will be in the 4.0p1 release.  Thanks to all who participated.

The GNU libintl gettext() thing still needs to be sorted (we'll probably remove
the gettext calls unless someone can tell us why it's required for BSM).
Comment 53 Darren Tucker 2005-03-10 09:06:53 AEDT
With the release of OpenSSH 4.0, these bugs are now closed. For details, see:
http://www.openssh.com/txt/release-4.0