This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch SSHD-914
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 4607df6377e9d5bb1f098d7b15ab592a10c6e1ab
Author: Lyor Goldstein <lgoldst...@apache.org>
AuthorDate: Wed Oct 16 11:20:03 2019 +0300
[SSHD-948] Do not accept password change request if the session does not
use MAC(s) for packet validation
---
CHANGES.md | 4 ++
.../apache/sshd/common/session/SessionContext.java | 24 ++++++++++++
.../client/auth/password/UserAuthPassword.java | 7 ++++
.../sshd/common/auth/UserAuthMethodFactory.java | 29 ++++++++++++++
.../auth/password/PasswordAuthenticator.java | 20 +++++++++-
.../server/auth/password/UserAuthPassword.java | 38 +++++++++++++-----
.../server/jaas/JaasPasswordAuthenticator.java | 16 ++++----
.../sshd/common/auth/AuthenticationTest.java | 45 +++++++++-------------
.../server/jaas/JaasPasswordAuthenticatorTest.java | 14 ++++---
.../sshd/util/test/BogusPasswordAuthenticator.java | 6 ++-
.../auth/password/LdapPasswordAuthenticator.java | 9 +++--
11 files changed, 156 insertions(+), 56 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index b9f692d..6ac150d 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -40,6 +40,10 @@ of their invocation (if any)
* Default MAC(s) list is set according to the
[ssh_config(5)](https://www.freebsd.org/cgi/man.cgi?query=ssh_config&sektion=5)
order as **first** ones, where the supported MAC(s) that do no appear in it
come last.
+* `PasswordAuthenticator` has a `handleClientPasswordChangeRequest` method
that is invoked if
+a password change has been indicated by the user during authentication via the
"password"
+method - by default throws `UnsupportedOperationException`.
+
## Minor code helpers
* `SessionListener` supports `sessionPeerIdentificationReceived` method that
is invoked once successful
diff --git
a/sshd-common/src/main/java/org/apache/sshd/common/session/SessionContext.java
b/sshd-common/src/main/java/org/apache/sshd/common/session/SessionContext.java
index c6ab1fc..a394868 100644
---
a/sshd-common/src/main/java/org/apache/sshd/common/session/SessionContext.java
+++
b/sshd-common/src/main/java/org/apache/sshd/common/session/SessionContext.java
@@ -131,6 +131,8 @@ public interface SessionContext
* @param session The {@link SessionContext} to be examined
* @return {@code true} if the context is not {@code null} and the ciphers
* have been established to anything other than "none".
+ * @see #getNegotiatedKexParameter(KexProposalOption)
getNegotiatedKexParameter
+ * @see KexProposalOption#CIPHER_PROPOSALS CIPHER_PROPOSALS
*/
static boolean isSecureSessionTransport(SessionContext session) {
if (session == null) {
@@ -147,4 +149,26 @@ public interface SessionContext
return true;
}
+
+ /**
+ * @param session The {@link SessionContext} to be examined
+ * @return {@code true} if the context is not {@code null} and the MAC(s)
+ * used to verify packet integrity have been established.
+ * @see #getNegotiatedKexParameter(KexProposalOption)
getNegotiatedKexParameter
+ * @see KexProposalOption#MAC_PROPOSALS MAC_PROPOSALS
+ */
+ static boolean isDataIntegrityTransport(SessionContext session) {
+ if (session == null) {
+ return false;
+ }
+
+ for (KexProposalOption opt : KexProposalOption.MAC_PROPOSALS) {
+ String value = session.getNegotiatedKexParameter(opt);
+ if (GenericUtils.isEmpty(value)) {
+ return false;
+ }
+ }
+
+ return true;
+ }
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/client/auth/password/UserAuthPassword.java
b/sshd-core/src/main/java/org/apache/sshd/client/auth/password/UserAuthPassword.java
index 119ad99..08db76c 100644
---
a/sshd-core/src/main/java/org/apache/sshd/client/auth/password/UserAuthPassword.java
+++
b/sshd-core/src/main/java/org/apache/sshd/client/auth/password/UserAuthPassword.java
@@ -99,6 +99,13 @@ public class UserAuthPassword extends AbstractUserAuth {
return false;
}
+ if
(!UserAuthMethodFactory.isDataIntegrityAuthenticationTransport(session)) {
+ if (debugEnabled) {
+ log.debug("processAuthDataRequest({})[{}] session is not
validated via MAC", session, service);
+ }
+ return false;
+ }
+
String prompt = buffer.getString();
String lang = buffer.getString();
UserInteraction ui = session.getUserInteraction();
diff --git
a/sshd-core/src/main/java/org/apache/sshd/common/auth/UserAuthMethodFactory.java
b/sshd-core/src/main/java/org/apache/sshd/common/auth/UserAuthMethodFactory.java
index 37a8768..94b688c 100644
---
a/sshd-core/src/main/java/org/apache/sshd/common/auth/UserAuthMethodFactory.java
+++
b/sshd-core/src/main/java/org/apache/sshd/common/auth/UserAuthMethodFactory.java
@@ -64,6 +64,14 @@ public interface UserAuthMethodFactory<S extends
SessionContext, M extends UserA
boolean DEFAULT_ALLOW_INSECURE_AUTH = false;
/**
+ * If set to {@code true} then {@link
#isDataIntegrityAuthenticationTransport(SessionContext)}
+ * returns {@code true} even if transport has no MAC(s) to verify message
integrity
+ */
+ String ALLOW_NON_INTEGRITY_AUTH = "allow-non-integrity-auth";
+
+ boolean DEFAULT_ALLOW_NON_INTEGRITY_AUTH = false;
+
+ /**
* @param session The session for which authentication is required
* @return The authenticator instance
* @throws IOException If failed to create the instance
@@ -121,4 +129,25 @@ public interface UserAuthMethodFactory<S extends
SessionContext, M extends UserA
return SessionContext.isSecureSessionTransport(session);
}
+
+ /**
+ * @param session The {@link SessionContext} being used for authentication
+ * @return {@code true} if the context is not {@code null} and the MAC(s)
+ * used to verify packet integrity have been established.
+ * @see {@value #ALLOW_NON_INTEGRITY_AUTH}
+ * @see SessionContext#isDataIntegrityTransport(SessionContext)
+ */
+ static boolean isDataIntegrityAuthenticationTransport(SessionContext
session) {
+ if (session == null) {
+ return false;
+ }
+
+ boolean allowNonValidated = PropertyResolverUtils.getBooleanProperty(
+ session, ALLOW_NON_INTEGRITY_AUTH,
DEFAULT_ALLOW_NON_INTEGRITY_AUTH);
+ if (allowNonValidated) {
+ return true;
+ }
+
+ return SessionContext.isDataIntegrityTransport(session);
+ }
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/auth/password/PasswordAuthenticator.java
b/sshd-core/src/main/java/org/apache/sshd/server/auth/password/PasswordAuthenticator.java
index fcc91e1..9cf3d6c 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/auth/password/PasswordAuthenticator.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/auth/password/PasswordAuthenticator.java
@@ -40,5 +40,23 @@ public interface PasswordAuthenticator {
* @throws AsyncAuthException If the authentication is performed
asynchronously
*/
boolean authenticate(String username, String password, ServerSession
session)
- throws PasswordChangeRequiredException, AsyncAuthException;
+ throws PasswordChangeRequiredException, AsyncAuthException;
+
+ /**
+ * Invoked when the client sends a {@code SSH_MSG_USERAUTH_REQUEST}
indicating
+ * a password change. This can happen if the {@code authenticate} method
+ * threw {@link PasswordChangeRequiredException} thus telling the client
that
+ * it needs to provide a new password. Throws {@link
UnsupportedOperationException}
+ * by default.
+ *
+ * @param session The {@link ServerSession} attempting the authentication
+ * @param username The username credential
+ * @param oldPassword The old password
+ * @param newPassword The new password
+ * @return {@code true} if password change accepted
+ */
+ default boolean handleClientPasswordChangeRequest(
+ ServerSession session, String username, String oldPassword, String
newPassword) {
+ throw new UnsupportedOperationException("Password change not
supported");
+ }
}
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/auth/password/UserAuthPassword.java
b/sshd-core/src/main/java/org/apache/sshd/server/auth/password/UserAuthPassword.java
index 6c5be5e..cc94cb6 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/auth/password/UserAuthPassword.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/auth/password/UserAuthPassword.java
@@ -43,13 +43,22 @@ public class UserAuthPassword extends AbstractUserAuth {
public Boolean doAuth(Buffer buffer, boolean init) throws Exception {
ValidateUtils.checkTrue(init, "Instance not initialized");
+ ServerSession session = getServerSession();
+ if (!UserAuthMethodFactory.isSecureAuthenticationTransport(session)) {
+ if (log.isDebugEnabled()) {
+ log.debug("doAuth({}) session is not secure", session);
+ }
+ return false;
+ }
+
+ String username = getUsername();
boolean newPassword = buffer.getBoolean();
String password = buffer.getString();
if (newPassword) {
return handleClientPasswordChangeRequest(
- buffer, getServerSession(), getUsername(), password,
buffer.getString());
+ buffer, session, username, password, buffer.getString());
} else {
- return checkPassword(buffer, getServerSession(), getUsername(),
password);
+ return checkPassword(buffer, session, username, password);
}
}
@@ -74,13 +83,6 @@ public class UserAuthPassword extends AbstractUserAuth {
Buffer buffer, ServerSession session, String username, String
password)
throws Exception {
boolean debugEnabled = log.isDebugEnabled();
- if (!UserAuthMethodFactory.isSecureAuthenticationTransport(session)) {
- if (debugEnabled) {
- log.debug("checkPassword({}) session is not secure", session);
- }
- return false;
- }
-
PasswordAuthenticator auth = session.getPasswordAuthenticator();
if (auth == null) {
if (debugEnabled) {
@@ -132,7 +134,23 @@ public class UserAuthPassword extends AbstractUserAuth {
protected Boolean handleClientPasswordChangeRequest(
Buffer buffer, ServerSession session, String username, String
oldPassword, String newPassword)
throws Exception {
- throw new UnsupportedOperationException("Password change not
supported");
+ boolean debugEnabled = log.isDebugEnabled();
+ if
(!UserAuthMethodFactory.isDataIntegrityAuthenticationTransport(session)) {
+ if (debugEnabled) {
+ log.debug("handleClientPasswordChangeRequest({}) session is
not validated via MAC", session);
+ }
+ return false;
+ }
+
+ PasswordAuthenticator auth = session.getPasswordAuthenticator();
+ if (auth == null) {
+ if (debugEnabled) {
+ log.debug("handleClientPasswordChangeRequest({}) no password
authenticator", session);
+ }
+ return false;
+ }
+
+ return auth.handleClientPasswordChangeRequest(session, username,
oldPassword, newPassword);
}
/**
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java
b/sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java
index 30d0d89..70e3594 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticator.java
@@ -35,7 +35,6 @@ import org.apache.sshd.server.session.ServerSession;
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
public class JaasPasswordAuthenticator extends AbstractLoggingBean implements
PasswordAuthenticator {
-
private String domain;
public JaasPasswordAuthenticator() {
@@ -55,14 +54,11 @@ public class JaasPasswordAuthenticator extends
AbstractLoggingBean implements Pa
}
@Override
- public boolean authenticate(String username, String password,
ServerSession session) {
- return authenticate(username, password);
- }
-
- public boolean authenticate(final String username, final String password) {
+ public boolean authenticate(
+ String username, String password, ServerSession session) {
try {
Subject subject = new Subject();
- LoginContext loginContext = new LoginContext(domain, subject,
callbacks -> {
+ LoginContext loginContext = new LoginContext(getDomain(), subject,
callbacks -> {
for (Callback callback : callbacks) {
if (callback instanceof NameCallback) {
((NameCallback) callback).setName(username);
@@ -77,7 +73,11 @@ public class JaasPasswordAuthenticator extends
AbstractLoggingBean implements Pa
loginContext.logout();
return true;
} catch (Exception e) {
- log.error("Authentication failed with error", e);
+ log.warn("authenticate({}) failed ({}) to authenticate user={}:
{}",
+ session, e.getClass().getSimpleName(), username,
e.getMessage());
+ if (log.isDebugEnabled()) {
+ log.debug("authenticate(" + session + ")[" + username + "]
failure details", e);
+ }
return false;
}
}
diff --git
a/sshd-core/src/test/java/org/apache/sshd/common/auth/AuthenticationTest.java
b/sshd-core/src/test/java/org/apache/sshd/common/auth/AuthenticationTest.java
index 8082416..dd95792 100644
---
a/sshd-core/src/test/java/org/apache/sshd/common/auth/AuthenticationTest.java
+++
b/sshd-core/src/test/java/org/apache/sshd/common/auth/AuthenticationTest.java
@@ -174,37 +174,30 @@ public class AuthenticationTest extends BaseTestSupport {
public void testChangePassword() throws Exception {
PasswordAuthenticator delegate = sshd.getPasswordAuthenticator();
AtomicInteger attemptsCount = new AtomicInteger(0);
- sshd.setPasswordAuthenticator((username, password, session) -> {
- if (attemptsCount.incrementAndGet() == 1) {
- throw new
PasswordChangeRequiredException(attemptsCount.toString(),
+ AtomicInteger changesCount = new AtomicInteger(0);
+ sshd.setPasswordAuthenticator(new PasswordAuthenticator() {
+ @Override
+ public boolean authenticate(String username, String password,
ServerSession session) {
+ if (attemptsCount.incrementAndGet() == 1) {
+ throw new
PasswordChangeRequiredException(attemptsCount.toString(),
getCurrentTestName(),
ServerAuthenticationManager.DEFAULT_WELCOME_BANNER_LANGUAGE);
- }
+ }
- return delegate.authenticate(username, password, session);
- });
+ return delegate.authenticate(username, password, session);
+ }
- AtomicInteger changesCount = new AtomicInteger(0);
- sshd.setUserAuthFactories(Collections.singletonList(
- new org.apache.sshd.server.auth.password.UserAuthPasswordFactory()
{
- @Override
- public org.apache.sshd.server.auth.password.UserAuthPassword
createUserAuth(ServerSession session) throws IOException {
- return new
org.apache.sshd.server.auth.password.UserAuthPassword() {
- @Override
- protected Boolean handleClientPasswordChangeRequest(
- Buffer buffer, ServerSession session, String
username, String oldPassword, String newPassword)
- throws Exception {
- if (changesCount.incrementAndGet() == 1) {
- assertNotEquals("Non-different passwords",
oldPassword, newPassword);
- return checkPassword(buffer, session,
username, newPassword);
- } else {
- return super.handleClientPasswordChangeRequest(
- buffer, session, username, oldPassword,
newPassword);
- }
- }
- };
+ @Override
+ public boolean handleClientPasswordChangeRequest(
+ ServerSession session, String username, String
oldPassword, String newPassword) {
+ if (changesCount.incrementAndGet() == 1) {
+ assertNotEquals("Non-different passwords", oldPassword,
newPassword);
+ return authenticate(username, newPassword, session);
+ } else {
+ return
PasswordAuthenticator.super.handleClientPasswordChangeRequest(
+ session, username, oldPassword, newPassword);
}
}
- ));
+ });
PropertyResolverUtils.updateProperty(sshd,
ServerAuthenticationManager.AUTH_METHODS,
UserAuthPasswordFactory.NAME);
diff --git
a/sshd-core/src/test/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticatorTest.java
b/sshd-core/src/test/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticatorTest.java
index cd0114e..f87a2f1 100644
---
a/sshd-core/src/test/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticatorTest.java
+++
b/sshd-core/src/test/java/org/apache/sshd/server/jaas/JaasPasswordAuthenticatorTest.java
@@ -82,11 +82,13 @@ public class JaasPasswordAuthenticatorTest extends
BaseTestSupport {
@Test
public void testAuthenticator() {
JaasPasswordAuthenticator auth = new JaasPasswordAuthenticator();
- assertNull(auth.getDomain());
+ assertNull("Unexpected initial domain", auth.getDomain());
+
auth.setDomain("domain");
- assertEquals("domain", auth.getDomain());
- assertTrue(auth.authenticate("sshd", "sshd"));
- assertFalse(auth.authenticate("sshd", "dummy"));
+ assertEquals("Mismatched domain", "domain", auth.getDomain());
+
+ assertTrue(auth.authenticate("sshd", "sshd", null));
+ assertFalse(auth.authenticate("sshd", "dummy", null));
}
protected static class DummyLoginModule implements LoginModule {
@@ -102,7 +104,9 @@ public class JaasPasswordAuthenticatorTest extends
BaseTestSupport {
}
@Override
- public void initialize(Subject subject, CallbackHandler
callbackHandler, Map<String, ?> sharedState, Map<String, ?> options) {
+ public void initialize(
+ Subject subject, CallbackHandler callbackHandler,
+ Map<String, ?> sharedState, Map<String, ?> options) {
this.subject = subject;
this.callbackHandler = callbackHandler;
}
diff --git
a/sshd-core/src/test/java/org/apache/sshd/util/test/BogusPasswordAuthenticator.java
b/sshd-core/src/test/java/org/apache/sshd/util/test/BogusPasswordAuthenticator.java
index 47df909..1382ac9 100644
---
a/sshd-core/src/test/java/org/apache/sshd/util/test/BogusPasswordAuthenticator.java
+++
b/sshd-core/src/test/java/org/apache/sshd/util/test/BogusPasswordAuthenticator.java
@@ -23,7 +23,8 @@ import
org.apache.sshd.server.auth.password.PasswordAuthenticator;
import org.apache.sshd.server.session.ServerSession;
/**
- * TODO Add javadoc
+ * A test {@link PasswordAuthenticator} that accepts an authentication
+ * attempt if the username is not {@code null} and same as password
*
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
@@ -38,7 +39,8 @@ public class BogusPasswordAuthenticator extends
AbstractLoggingBean implements P
public boolean authenticate(String username, String password,
ServerSession session) {
boolean result = (username != null) && username.equals(password);
if (log.isDebugEnabled()) {
- log.debug("authenticate({}) {} / {} - success = {}", session,
username, password, result);
+ log.debug("authenticate({}) {} / {} - success={}",
+ session, username, password, result);
}
return result;
diff --git
a/sshd-ldap/src/main/java/org/apache/sshd/server/auth/password/LdapPasswordAuthenticator.java
b/sshd-ldap/src/main/java/org/apache/sshd/server/auth/password/LdapPasswordAuthenticator.java
index 83165e7..6413d79 100644
---
a/sshd-ldap/src/main/java/org/apache/sshd/server/auth/password/LdapPasswordAuthenticator.java
+++
b/sshd-ldap/src/main/java/org/apache/sshd/server/auth/password/LdapPasswordAuthenticator.java
@@ -62,14 +62,14 @@ public class LdapPasswordAuthenticator extends
LdapAuthenticator implements Pass
}
@Override
- public boolean authenticate(String username, String password,
ServerSession session) throws PasswordChangeRequiredException {
+ public boolean authenticate(String username, String password,
ServerSession session)
+ throws PasswordChangeRequiredException {
try {
Map<String, ?> attrs = resolveAttributes(username, password,
session);
return authenticate(username, password, session, attrs);
} catch (NamingException | RuntimeException e) {
log.warn("authenticate({}@{}) failed ({}) to query: {}",
- username, session, e.getClass().getSimpleName(),
e.getMessage());
-
+ username, session, e.getClass().getSimpleName(),
e.getMessage());
if (log.isDebugEnabled()) {
log.debug("authenticate(" + username + "@" + session + ")
query failure details", e);
}
@@ -78,7 +78,8 @@ public class LdapPasswordAuthenticator extends
LdapAuthenticator implements Pass
}
}
- protected boolean authenticate(String username, String password,
ServerSession session, Map<String, ?> attrs) {
+ protected boolean authenticate(
+ String username, String password, ServerSession session,
Map<String, ?> attrs) {
/*
* By default we assume that the user + password are the same for
* accessing the LDAP as the user's account, so the very LDAP query
