On Mon, 27 Dec 2021, Damien Miller wrote:
> On Sun, 26 Dec 2021, Nam Nguyen wrote:
>
> > >Synopsis: ssh anoncvs regression: Connection closed
> > >Category: user
> > >Environment:
> > System : OpenBSD 7.0
> > Details : OpenBSD 7.0-current (GENERIC) #198: Sat Dec 25 16:22:02
> > MST 2021
> >
> > [email protected]:/usr/src/sys/arch/amd64/compile/GENERIC
> >
> > Architecture: OpenBSD.amd64
> > Machine : amd64
> > >Description:
> > I have an anoncvs instance that stopped working upon upgrading to
> > -current. This looks like a regression with openssh, which closes the
> > connection.
> >
> > $ cvs -d [email protected]:/cvs checkout -P nvi/
> > Connection closed by 45.77.165.128 port 22
> > cvs [checkout aborted]: end of file from server (consult above messages if
> > any)
>
> Thanks for the report. Do you control the server too? If so, are you able to
> capture a debug trace from sshd there? An easy way to do this is to run
> sshd on a separate port in non-forking mode and connect to that, e.g.
> "/usr/bin/sshd -dddp2222" and "ssh -p 2222 user@host".
Never mind, I have reproduced the problem locally. I broke "none"
authentication in the process of merging the agent restriction changes.
Unfortunately we don't have good coverage of this authentication method in
the regress tests.
The specific problem was that I was relying on authmethod_lookup() in
userauth_finish() to get the canonical authentication method name, but
this lookup function implicitly checks whether the authentication method
is enabled, and I didn't realise that userauth_none() disables itself
after being called once.
The solution is to split the lookup and enable tests into separate
functions.
This should fix it:
diff --git a/auth2.c b/auth2.c
index d6cc3d7..e68952a 100644
--- a/auth2.c
+++ b/auth2.c
@@ -89,6 +89,7 @@ static int input_service_request(int, u_int32_t, struct ssh
*);
static int input_userauth_request(int, u_int32_t, struct ssh *);
/* helper */
+static Authmethod *authmethod_byname(const char *);
static Authmethod *authmethod_lookup(Authctxt *, const char *);
static char *authmethods_get(Authctxt *authctxt);
@@ -345,9 +346,10 @@ userauth_finish(struct ssh *ssh, int authenticated, const
char *packet_method,
}
if (authctxt->postponed)
fatal("INTERNAL ERROR: authenticated and postponed");
- if ((m = authmethod_lookup(authctxt, method)) == NULL)
+ /* prefer primary authmethod name to possible synonym */
+ if ((m = authmethod_byname(method)) == NULL)
fatal("INTERNAL ERROR: bad method %s", method);
- method = m->name; /* prefer primary name to possible synonym */
+ method = m->name;
}
/* Special handling for root */
@@ -457,25 +459,42 @@ authmethods_get(Authctxt *authctxt)
}
static Authmethod *
-authmethod_lookup(Authctxt *authctxt, const char *name)
+authmethod_byname(const char *name)
{
int i;
- if (name != NULL)
- for (i = 0; authmethods[i] != NULL; i++)
- if (authmethods[i]->enabled != NULL &&
- *(authmethods[i]->enabled) != 0 &&
- (strcmp(name, authmethods[i]->name) == 0 ||
- (authmethods[i]->synonym != NULL &&
- strcmp(name, authmethods[i]->synonym) == 0)) &&
- auth2_method_allowed(authctxt,
- authmethods[i]->name, NULL))
- return authmethods[i];
- debug2("Unrecognized authentication method name: %s",
- name ? name : "NULL");
+ if (name == NULL)
+ fatal_f("NULL authentication method name");
+ for (i = 0; authmethods[i] != NULL; i++) {
+ if (strcmp(name, authmethods[i]->name) == 0 ||
+ (authmethods[i]->synonym != NULL &&
+ strcmp(name, authmethods[i]->synonym) == 0))
+ return authmethods[i];
+ }
+ debug_f("unrecognized authentication method name: %s", name);
return NULL;
}
+static Authmethod *
+authmethod_lookup(Authctxt *authctxt, const char *name)
+{
+ Authmethod *method;
+
+ if ((method = authmethod_byname(name)) == NULL)
+ return NULL;
+
+ if (method->enabled == NULL || *(method->enabled) == 0) {
+ debug3_f("method %s not enabled", name);
+ return NULL;
+ }
+ if (!auth2_method_allowed(authctxt, method->name, NULL)) {
+ debug3_f("method %s not allowed "
+ "by AuthenticationMethods", name);
+ return NULL;
+ }
+ return method;
+}
+
/*
* Check a comma-separated list of methods for validity. Is need_enable is
* non-zero, then also require that the methods are enabled.
