3 files changed, 94 insertions, 74 deletions

diff --git a/ChangeLog b/ChangeLog
index dc8048939..873e1459d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -33,6 +33,22 @@
      Merge duplicate host key file checks, based in part on a patch from Rob
      Holland via bz #1348 .  Also checks for non-regular files during protocol
      1 RSA auth.  ok djm@
+   - djm@cvs.openbsd.org 2008/07/02 12:36:39
+     [auth2-none.c auth2.c]
+     Make protocol 2 MaxAuthTries behaviour a little more sensible:
+     Check whether client has exceeded MaxAuthTries before running
+     an authentication method and skip it if they have, previously it
+     would always allow one try (for "none" auth).
+     Preincrement failure count before post-auth test - previously this
+     checked and postincremented, also to allow one "none" try.
+     Together, these two changes always count the "none" auth method
+     which could be skipped by a malicious client (e.g. an SSH worm)
+     to get an extra attempt at a real auth method. They also make
+     MaxAuthTries=0 a useful way to block users entirely (esp. in a
+     sshd_config Match block).
+     Also, move sending of any preauth banner from "none" auth method
+     to the first call to input_userauth_request(), so worms that skip
+     the "none" method get to see it too.
 
 20080630
  - (djm) OpenBSD CVS Sync
@@ -4516,4 +4532,4 @@
    OpenServer 6 and add osr5bigcrypt support so when someone migrates
    passwords between UnixWare and OpenServer they will still work. OK dtucker@
 
-$Id: ChangeLog,v 1.5047 2008/07/02 12:37:30 dtucker Exp $
+$Id: ChangeLog,v 1.5048 2008/07/02 12:56:09 dtucker Exp $
diff --git a/auth2-none.c b/auth2-none.c
index 28e593e6c..10accfe55 100644
--- a/auth2-none.c
+++ b/auth2-none.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth2-none.c,v 1.14 2007/08/23 03:22:16 djm Exp $ */
+/* $OpenBSD: auth2-none.c,v 1.15 2008/07/02 12:36:39 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  *
@@ -31,9 +31,10 @@
 
 #include <fcntl.h>
 #include <stdarg.h>
-#include <unistd.h>
 #include <string.h>
+#include <unistd.h>
 
+#include "atomicio.h"
 #include "xmalloc.h"
 #include "key.h"
 #include "hostfile.h"
@@ -42,7 +43,6 @@
 #include "log.h"
 #include "buffer.h"
 #include "servconf.h"
-#include "atomicio.h"
 #include "compat.h"
 #include "ssh2.h"
 #ifdef GSSAPI
@@ -56,77 +56,11 @@ extern ServerOptions options;
 /* "none" is allowed only one time */
 static int none_enabled = 1;
 
-char *
-auth2_read_banner(void)
-{
-        struct stat st;
-        char *banner = NULL;
-        size_t len, n;
-        int fd;
-
-        if ((fd = open(options.banner, O_RDONLY)) == -1)
-                return (NULL);
-        if (fstat(fd, &st) == -1) {
-                close(fd);
-                return (NULL);
-        }
-        if (st.st_size > 1*1024*1024) {
-                close(fd);
-                return (NULL);
-        }
-
-        len = (size_t)st.st_size;               /* truncate */
-        banner = xmalloc(len + 1);
-        n = atomicio(read, fd, banner, len);
-        close(fd);
-
-        if (n != len) {
-                xfree(banner);
-                return (NULL);
-        }
-        banner[n] = '\0';
-
-        return (banner);
-}
-
-void
-userauth_send_banner(const char *msg)
-{
-        if (datafellows & SSH_BUG_BANNER)
-                return;
-
-        packet_start(SSH2_MSG_USERAUTH_BANNER);
-        packet_put_cstring(msg);
-        packet_put_cstring("");         /* language, unused */
-        packet_send();
-        debug("%s: sent", __func__);
-}
-
-static void
-userauth_banner(void)
-{
-        char *banner = NULL;
-
-        if (options.banner == NULL ||
-            strcasecmp(options.banner, "none") == 0 ||
-            (datafellows & SSH_BUG_BANNER) != 0)
-                return;
-
-        if ((banner = PRIVSEP(auth2_read_banner())) == NULL)
-                goto done;
-        userauth_send_banner(banner);
-
-done:
-        if (banner)
-                xfree(banner);
-}
-
 static int
 userauth_none(Authctxt *authctxt)
 {
         none_enabled = 0;
         packet_check_eom();
-        userauth_banner();
 #ifdef HAVE_CYGWIN
         if (check_nt_auth(1, authctxt->pw) == 0)
                 return (0);
diff --git a/auth2.c b/auth2.c
index 03d7f09dc..31f01f9fb 100644
--- a/auth2.c
+++ b/auth2.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: auth2.c,v 1.116 2007/09/29 00:25:51 dtucker Exp $ */
+/* $OpenBSD: auth2.c,v 1.117 2008/07/02 12:36:39 djm Exp $ */
 /*
  * Copyright (c) 2000 Markus Friedl.  All rights reserved.
  *
@@ -26,10 +26,14 @@
 #include "includes.h"
 
 #include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/uio.h>
 
+#include <fcntl.h>
 #include <pwd.h>
 #include <stdarg.h>
 #include <string.h>
+#include <unistd.h>
 
 #include "xmalloc.h"
 #include "ssh2.h"
@@ -88,10 +92,74 @@ static void input_userauth_request(int, u_int32_t, void *);
 static Authmethod *authmethod_lookup(const char *);
 static char *authmethods_get(void);
 
+char *
+auth2_read_banner(void)
+{
+        struct stat st;
+        char *banner = NULL;
+        size_t len, n;
+        int fd;
+
+        if ((fd = open(options.banner, O_RDONLY)) == -1)
+                return (NULL);
+        if (fstat(fd, &st) == -1) {
+                close(fd);
+                return (NULL);
+        }
+        if (st.st_size > 1*1024*1024) {
+                close(fd);
+                return (NULL);
+        }
+
+        len = (size_t)st.st_size;               /* truncate */
+        banner = xmalloc(len + 1);
+        n = atomicio(read, fd, banner, len);
+        close(fd);
+
+        if (n != len) {
+                xfree(banner);
+                return (NULL);
+        }
+        banner[n] = '\0';
+
+        return (banner);
+}
+
+void
+userauth_send_banner(const char *msg)
+{
+        if (datafellows & SSH_BUG_BANNER)
+                return;
+
+        packet_start(SSH2_MSG_USERAUTH_BANNER);
+        packet_put_cstring(msg);
+        packet_put_cstring("");         /* language, unused */
+        packet_send();
+        debug("%s: sent", __func__);
+}
+
+static void
+userauth_banner(void)
+{
+        char *banner = NULL;
+
+        if (options.banner == NULL ||
+            strcasecmp(options.banner, "none") == 0 ||
+            (datafellows & SSH_BUG_BANNER) != 0)
+                return;
+
+        if ((banner = PRIVSEP(auth2_read_banner())) == NULL)
+                goto done;
+        userauth_send_banner(banner);
+
+done:
+        if (banner)
+                xfree(banner);
+}
+
 /*
  * loop until authctxt->success == TRUE
  */
-
 void
 do_authentication2(Authctxt *authctxt)
 {
@@ -179,6 +247,7 @@ input_userauth_request(int type, u_int32_t seq, void *ctxt)
                 authctxt->style = style ? xstrdup(style) : NULL;
                 if (use_privsep)
                         mm_inform_authserv(service, style);
+                userauth_banner();
         } else if (strcmp(user, authctxt->user) != 0 ||
             strcmp(service, authctxt->service) != 0) {
                 packet_disconnect("Change of username or service not allowed: "
@@ -197,7 +266,7 @@ input_userauth_request(int type, u_int32_t seq, void *ctxt)
 
         /* try to authenticate user */
         m = authmethod_lookup(method);
-        if (m != NULL) {
+        if (m != NULL && authctxt->failures < options.max_authtries) {
                 debug2("input_userauth_request: try method %s", method);
                 authenticated = m->userauth(authctxt);
         }
@@ -264,7 +333,7 @@ userauth_finish(Authctxt *authctxt, int authenticated, char *method)
                 /* now we can break out */
                 authctxt->success = 1;
         } else {
-                if (authctxt->failures++ > options.max_authtries) {
+                if (++authctxt->failures > options.max_authtries) {
 #ifdef SSH_AUDIT_EVENTS
                         PRIVSEP(audit_event(SSH_LOGIN_EXCEED_MAXTRIES));
 #endif
@@ -320,3 +389,4 @@ authmethod_lookup(const char *name)
             name ? name : "NULL");
         return NULL;
 }
+