OpenSSH does not log failed attempts when key authentication is used

OpenSSH doesn't record badlogin attempts in btmp file for all possible
authentication methods supported ( given below ). This patch is an enhancement
to support this feature.

Authentication Methods Supported
---------------------------------
"gssapi-with-mic"
"publickey"
"hostbased"
"password"
"keyboard-interactive"
"challenge-response"
"keyboard-interactive/pam"

Created attachment 774
BTMP PATCH

BTMP Enhancement Patch attached .

Redhat (and maybe Solaris?) also have btmp, so setting platform to "All".

Just so it's clear: the btmp code in loginrec is derived from login from
util-linux correct? If so that's BSD licensed (w/advertising clause) with UCB
as the copyright holder. Since UCB rescinded the advertising clause I think
we're OK to use the code.

Comment on attachment 774
BTMP PATCH

>+ if ( getuid() != 0 ){
>+ debug("=== calling log_btmp uid %d ===\n", getuid());
>+ mm_log_btmp(authctxt->user,get_canonical_hostname(options.use_dns));
>+ } else {
>+ debug("=== calling log_btmp uid %d ===\n", getuid());
>+ og_btmp(authctxt->user,get_canonical_hostname(options.use_dns));
>+ }

This bit is unnecessary, the PRIVSEP macro is for this purpose, ie:

    debug("=== calling log_btmp uid %d ===\n", getuid());
    PRIVSEP(log_btmp(authctxt->user,get_canonical_hostname(options.use_dns)));

>+int mm_answer_log_btmp(int socket, Buffer *m){
[...]
>+ buffer_get(m, user ,1024);
>+ buffer_get(m, hostname ,1024);

Sending the username is unecessary as the monitor already knows it (ie
authctxt->user). Hostname can be determined too (see mm_record_login). As a
general rule, as little as possible should be sent from slave to monitor.

It's out of the scope of this bug but I'd prefer to see a generalized
audit_event like in bug #125 in both OpenBSD and Portable, which could be
easily extended to handle cases like this.

> the btmp code in loginrec is derived from login from util-linux correct?

Yes referred from util-linux .

> This bit is unnecessary, the PRIVSEP macro is for this purpose, ie:
> debug("=== calling log_btmp uid %d ===\n", getuid());
> PRIVSEP(log_btmp(authctxt->user,get_canonical_hostname(options.use_dns)));

I tried this, PRIVSEP macro may require a redefinition in auth.c. since it
reports a linker error "ld: Unsatisfied symbol "PRIVSEP" in file auth.o"

(In reply to comment #4)
> I tried this, PRIVSEP macro may require a redefinition in auth.c. since it
> reports a linker error "ld: Unsatisfied symbol "PRIVSEP" in file auth.o"

You just need to add '#include "monitor_wrap.h"' to the headers of auth.c.

Created attachment 776
License from util-linux-2.12p:login-utils/login.c

You must be careful about licensing when importing code. Fortunately this part
of util-linux has a compatible license, but you still must include it when
adding derived code into loginrec.c.

Created attachment 777
BTMP PATCH

Optimized BTMP patch attached .

> You just need to add '#include "monitor_wrap.h"' to the headers of auth.c.
Yes, including monitor_wrap.h header file in auth.c solved the problem and
also Optimized the patch further. Thanks

> you still must include it when adding derived code into loginrec.c.
Yes I have included util-linux compatible license in loginrec.c

Note: btmp Path ( /var/adm/btmp ) has been hardcoded in this patch.

Created attachment 780
Optimized BTMP PATCH

> Sending the username is unecessary as the monitor already knows it (ie
> authctxt->user). Hostname can be determined too (see mm_record_login). As a
> general rule, as little as possible should be sent from slave to monitor.

Yes, I have extracted user and hostname from authctxt->user and
get_canonical_hostname () in monitor.c function .

Created attachment 781
Optimized Patch Attached

> Sending the username is unecessary as the monitor already knows it (ie
> authctxt->user). Hostname can be determined too (see mm_record_login). As a

> general rule, as little as possible should be sent from slave to monitor.

Yes, I have extracted user and hostname from authctxt->user and
get_canonical_hostname () in monitor.c function .

Comment on attachment 781
Optimized Patch Attached

>--- Orginal/openssh/auth.c 2004-08-12 18:10:25.000000000 +0530
>+++ Patched/openssh/auth.c 2005-01-20 15:13:08.281151112 +0530
>@@ -50,6 +50,7 @@
> #include "misc.h"
> #include "bufaux.h"
> #include "packet.h"
>+#include "monitor_wrap.h"
>
> /* import */
> extern ServerOptions options;
>@@ -230,6 +231,18 @@
> else
> authmsg = authenticated ? "Accepted" : "Failed";
>
>+ if(!authenticated && !authctxt->postponed && (!strcmp(method, "gssapi-with-mic") || !strcmp(method, "publickey") || !strcmp(method, "hostbased"))){
>+ debug("=== calling log_btmp uid %d ===\n", getuid());
>+ PRIVSEP(log_btmp(authctxt->user,get_canonical_hostname(options.use_dns)));
>+ }
>+
>+
>+ if(!authenticated && !authctxt->postponed && (!strcmp(method, "password") || !strcmp(method, " keyboard-interactive") || !strcmp(method,"challenge-response") || !strcmp(method,"keyboard-interactive/pam"))){
>+ if ( getuid() == 0) {
>+ debug("=== calling log_btmp uid %d ===\n", getuid());

These two blocks can be merged. The logging is inconsistent with other debug
calls. The long list of strcmp looks fragile, if we add more auth methods.

>+#define _PATH_BTMP "/var/adm/btmp"

Most of the paths are defined in header files.

>+void
>+log_btmp(const char *username, const char *hostname) {

here (and elsewhere), you aren't following
http://www.openbsd.org/cgi-bin/man.cgi?query=style

In reply to comment #12)
> >+#define _PATH_BTMP "/var/adm/btmp"
>
> Most of the paths are defined in header files.

This one isn't (in the system headers, that is), at least as far as I can tell.
 I figured we'd stick it in the appropriate part of configure when the time
comes (but that's an unnecessary complication right now).

Created attachment 785
Modified BTMP PATCH attached

> if(!authenticated && !authctxt->postponed && (!strcmp(method,
"gssapi-with-mic") || !strcmp(method, "publickey") || !strcmp(method,
"hostbased"))){
> debug("=== calling log_btmp uid %d ===\n", getuid());
>
PRIVSEP(log_btmp(authctxt->user,get_canonical_hostname(options.use_dns)));
> }
>
>
> if(!authenticated && !authctxt->postponed && (!strcmp(method,
"password") || !strcmp(method, " keyboard-interactive") ||
!strcmp(method,"challenge-response") ||
!strcmp(method,"keyboard-interactive/pam"))){
> if ( getuid() == 0) {
> debug("=== calling log_btmp uid %d ===\n", getuid());

> These two blocks can be merged. The logging is inconsistent with other debug
calls. The long list of strcmp looks fragile, > if we add more auth methods.

THese two bloacks were merged as given below.

if (!authenticated && !authctxt->postponed && strcmp(method, "none")) {
      debug("Entering log_btmp uid %d ", getuid());
      if((!strcmp(method, "gssapi-with-mic") || !strcmp(method, "publickey") ||
!strcmp(method, "hostbased")))

PRIVSEP(log_btmp(authctxt->user,get_canonical_hostname(options.use_dns)));
      else if ( getuid()==0)

log_btmp(authctxt->user,get_canonical_hostname(options.use_dns));
}

>void
>log_btmp(const char *username, const char *hostname) {

These coding convention were corrected based on the source file style guide .

Created attachment 787
BTMP PATCH Attached

This patch has some correction with that of the pervious one:

1. buffer_append ( ) in mm_log_btmp ( ) is been replaced with
buffer_get_string ( ) to avoid some memory errors due to memcpy copy done from
random heap data.

2 UnWanted entries were removed from two tables 1. struct mon_table
mon_dispatch_postauth20[] 2. struct mon_table mon_dispatch_postauth15[] in
monitor.c .

Comment on attachment 787
BTMP PATCH Attached

I think we could commit the btmp logging part hooked up to CUSTOM_FAILED_LOGIN
(subject to some changes below) but I think the monitor parts should be
deferred until bug #125 is sorted.

>+#define _PATH_BTMP "/var/adm/btmp"

We'll put this in configure.ac. No big deal.

>+void
>+log_btmp(const char *username, const char *hostname)
>+{
[...]
>+ strcpy(ut.ut_line,"ssh:notty");

We don't use strcpy.

[lots of processing snipped]
>+ if (stat(_PATH_BTMP,&fst) == -1){
[...]
>+ fd = open(_PATH_BTMP, O_WRONLY | O_APPEND);

Not that this is a big deal but stat()ing the file then opening it is racy, and
building the record is a waste of time if you're not going to be able to write
it.

I think the sequence should be: open, fstat, construct record, write.

I'll do a patch with the above changes.

Created attachment 798
log failed password and kbdint to btmp on Linux and HP-UX

Adapted to use existing record_failed_login hook for password and
keyboard-interactive. Tested OK on Linux (RH9, IPv4 and IPv6) and HP-UX
(11.11, IPv4 only).

It won't do other auth types (pubkey, gssapi, hostbased) at the moment, however
this can be added later (after bug #125 is sorted).

It also extracts the remote socket address via getpeername rather than doing a
lookup on the textual hostname (which is wasteful and may not resolve to the
same address as the connection originated from).

Created attachment 799
btmp logging: normalise mapped 4in6 addresses too.

Comment on attachment 799
btmp logging: normalise mapped 4in6 addresses too.

looks sane to me

Patch #799 committed, thanks. It will be in the tomorrow's snapshot and the
next major release.

As to logging failures for the other auth types (pubkey, gssapi, hostbased), I
don't think that should be enabled by default since most clients will try those
as a matter of course and some environments might do lockouts based on the failures.

> + AC_DEFINE(_PATH_BTMP, "/var/log/btmp", [log for bad login attempts])

In HP-UX it is "/var/adm/btmp"

> As to logging failures for the other auth types (pubkey, gssapi, hostbased), I
> don't think that should be enabled by default

How about getting them under a sshd_config directive .

(In reply to comment #21)
> > + AC_DEFINE(_PATH_BTMP, "/var/log/btmp", [log for bad login attempts])
>
> In HP-UX it is "/var/adm/btmp"

The line you quoted is from the Linux block. On HP-UX it picks up the BTMP_FILE
definition from the system headers (see defines.h). BTMP_FILE is defined on my
11.11 box, if it needs to be added for other versions just let me know which.

> > As to logging failures for the other auth types (pubkey, gssapi, hostbased),
> > Idon't think that should be enabled by default
>
> How about getting them under a sshd_config directive .

I'm not sure. As a rule we try to keep the -Portable only config uptions to a
minimum to preserve our sanity. Maybe a compile-time option to begin with
("-DPARANOID_AUTH_RECORDING" or something).

Can we log bad login attempts of an invalid user ?

This patch log for "none" method when login attempt is made by a In-valid User.
But works correctly for existing valid user ( doesn't log for "none" method )

1. Disabling "none" doesn't solve
if (authenticated == 0 && !authctxt->postponed && strcmp(method, "none" ) && ...

2. Avoiding Invalid user doesn't solve
if (authenticated == 0 && authctxt->valid && !authctxt->postponed &&
strcmp(method, "none" ) && ....

(In reply to comment #23)
> This patch log for "none" method when login attempt is made by a invalid user.
 > But works correctly for existing valid user (doesn't log for "none" method )

That happens earlier than the auth loop (in getpwnamallow). I'm not sure I want
to change that right now as it's the only place guaranteed to be called for an
invalid user if they try only, eg pubkey authentication then disconnect (because
that occurs purely in the unprivileged child).

We may be able to address that with the AUDIT_EVENTS hooks but those aren't
enabled by default.

Created attachment 862
Patch for logging Bad key based Authentications

The attached patch is an enhancement over OpenSSH4.0p1 to support logging of
BAD login attempts for Keybased Authentications.