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 719cc3e28dc2c3ed2f00dc5a0ceb0d0c30409919
Author: Lyor Goldstein <lgoldst...@apache.org>
AuthorDate: Wed Oct 16 09:03:25 2019 +0300
[SSHD-948] Do not accept password authentication if the session is not
encrypted
---
CHANGES.md | 4 ++-
.../apache/sshd/common/session/SessionContext.java | 22 ++++++++++++
.../client/auth/password/UserAuthPassword.java | 39 +++++++++++++++++-----
.../sshd/common/auth/UserAuthMethodFactory.java | 39 ++++++++++++++++++++++
.../server/auth/password/UserAuthPassword.java | 25 ++++++++++----
.../sshd/server/session/AbstractServerSession.java | 15 +++++----
.../sshd/common/auth/AuthenticationTest.java | 3 +-
7 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/CHANGES.md b/CHANGES.md
index 73664a9..b9f692d 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -80,10 +80,12 @@ exchange via properties.
* [SSHD-943](https://issues.apache.org/jira/browse/SSHD-943) - Provide session
instance when KEX factory is invoked in order to create a KeyExchange instance.
-* [SSHD-945](https://issues.apache.org/jira/browse/SSHD-945) - Added
sshd-contrib code that uses SHA1 with DSA regardless of its key length
+* [SSHD-945](https://issues.apache.org/jira/browse/SSHD-945) - Added
sshd-contrib code that uses SHA1 with DSA regardless of its key length.
* [SSHD-946](https://issues.apache.org/jira/browse/SSHD-946) - Supporting
'encrypt-then-MAC' mode.
* [SSHD-947](https://issues.apache.org/jira/browse/SSHD-947) - Added
configuration allowing the user to specify whether client should wait
for the server's identification before sending KEX-INIT message.
+* [SSHD-948](https://issues.apache.org/jira/browse/SSHD-948) - Do not accept
password authentication if the session is not encrypted.
+
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 23d5c60..c6ab1fc 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
@@ -23,6 +23,7 @@ import java.util.Map;
import org.apache.sshd.common.AttributeStore;
import org.apache.sshd.common.auth.UsernameHolder;
+import org.apache.sshd.common.cipher.BuiltinCiphers;
import org.apache.sshd.common.kex.KexProposalOption;
import org.apache.sshd.common.kex.KexState;
import org.apache.sshd.common.util.GenericUtils;
@@ -125,4 +126,25 @@ public interface SessionContext
return GenericUtils.isNotEmpty(version)
&& (version.startsWith(DEFAULT_SSH_VERSION_PREFIX) ||
version.startsWith(FALLBACK_SSH_VERSION_PREFIX));
}
+
+ /**
+ * @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".
+ */
+ static boolean isSecureSessionTransport(SessionContext session) {
+ if (session == null) {
+ return false;
+ }
+
+ for (KexProposalOption opt : KexProposalOption.CIPHER_PROPOSALS) {
+ String value = session.getNegotiatedKexParameter(opt);
+ if (GenericUtils.isEmpty(value)
+ || BuiltinCiphers.Constants.NONE.equalsIgnoreCase(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 6785305..119ad99 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
@@ -27,12 +27,14 @@ import org.apache.sshd.client.auth.keyboard.UserInteraction;
import org.apache.sshd.client.session.ClientSession;
import org.apache.sshd.common.RuntimeSshException;
import org.apache.sshd.common.SshConstants;
+import org.apache.sshd.common.auth.UserAuthMethodFactory;
import org.apache.sshd.common.io.IoWriteFuture;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.buffer.Buffer;
/**
- * Implements the "password" authentication mechanism
+ * Implements the client-side "password" authentication mechanism
+ *
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
public class UserAuthPassword extends AbstractUserAuth {
@@ -53,6 +55,13 @@ public class UserAuthPassword extends AbstractUserAuth {
@Override
protected boolean sendAuthDataRequest(ClientSession session, String
service) throws Exception {
+ if (!UserAuthMethodFactory.isSecureAuthenticationTransport(session)) {
+ if (log.isDebugEnabled()) {
+ log.debug("sendAuthDataRequest({})[{}] session is not secure",
session, service);
+ }
+ return false;
+ }
+
if ((passwords == null) || (!passwords.hasNext())) {
if (log.isDebugEnabled()) {
log.debug("sendAuthDataRequest({})[{}] no more passwords to
send", session, service);
@@ -64,17 +73,30 @@ public class UserAuthPassword extends AbstractUserAuth {
current = passwords.next();
String username = session.getUsername();
Buffer buffer =
session.createBuffer(SshConstants.SSH_MSG_USERAUTH_REQUEST,
- username.length() + service.length() + getName().length() +
current.length() + Integer.SIZE);
+ username.length() + service.length()
+ + GenericUtils.length(getName()) + current.length()
+ + Integer.SIZE /* a few extra encoding fields overhead */);
sendPassword(buffer, session, current, current);
return true;
}
@Override
- protected boolean processAuthDataRequest(ClientSession session, String
service, Buffer buffer) throws Exception {
+ protected boolean processAuthDataRequest(
+ ClientSession session, String service, Buffer buffer)
+ throws Exception {
int cmd = buffer.getUByte();
if (cmd != SshConstants.SSH_MSG_USERAUTH_PASSWD_CHANGEREQ) {
- throw new IllegalStateException("processAuthDataRequest(" +
session + ")[" + service + "]"
- + " received unknown packet: cmd=" +
SshConstants.getCommandMessageName(cmd));
+ throw new IllegalStateException(
+ "processAuthDataRequest(" + session + ")[" + service + "]"
+ + " received unknown packet: cmd=" +
SshConstants.getCommandMessageName(cmd));
+ }
+
+ boolean debugEnabled = log.isDebugEnabled();
+ if (!UserAuthMethodFactory.isSecureAuthenticationTransport(session)) {
+ if (debugEnabled) {
+ log.debug("processAuthDataRequest({})[{}] session is not
secure", session, service);
+ }
+ return false;
}
String prompt = buffer.getString();
@@ -82,7 +104,6 @@ public class UserAuthPassword extends AbstractUserAuth {
UserInteraction ui = session.getUserInteraction();
boolean interactive;
String password;
- boolean debugEnabled = log.isDebugEnabled();
try {
interactive = (ui != null) && ui.isInteractionAllowed(session);
password = interactive ? ui.getUpdatedPassword(session, prompt,
lang) : null;
@@ -131,14 +152,16 @@ public class UserAuthPassword extends AbstractUserAuth {
* on the success/failure of the request packet being sent
* @throws IOException If failed to send the message.
*/
- protected IoWriteFuture sendPassword(Buffer buffer, ClientSession session,
String oldPassword, String newPassword) throws IOException {
+ protected IoWriteFuture sendPassword(
+ Buffer buffer, ClientSession session, String oldPassword, String
newPassword)
+ throws IOException {
String username = session.getUsername();
String service = getService();
String name = getName();
boolean modified = !Objects.equals(oldPassword, newPassword);
if (log.isDebugEnabled()) {
log.debug("sendPassword({})[{}] send SSH_MSG_USERAUTH_REQUEST for
{} - modified={}",
- session, service, name, modified);
+ session, service, name, modified);
}
buffer = session.createBuffer(SshConstants.SSH_MSG_USERAUTH_REQUEST,
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 4004120..37a8768 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
@@ -23,6 +23,7 @@ import java.io.IOException;
import java.util.Collection;
import org.apache.sshd.common.NamedResource;
+import org.apache.sshd.common.PropertyResolverUtils;
import org.apache.sshd.common.session.SessionContext;
/**
@@ -55,6 +56,14 @@ public interface UserAuthMethodFactory<S extends
SessionContext, M extends UserA
String HOST_BASED = "hostbased";
/**
+ * If set to {@code true} then {@link
#isSecureAuthenticationTransport(SessionContext)}
+ * returns {@code true} even if transport is insecure.
+ */
+ String ALLOW_INSECURE_AUTH = "allow-insecure-auth";
+
+ boolean DEFAULT_ALLOW_INSECURE_AUTH = false;
+
+ /**
* @param session The session for which authentication is required
* @return The authenticator instance
* @throws IOException If failed to create the instance
@@ -82,4 +91,34 @@ public interface UserAuthMethodFactory<S extends
SessionContext, M extends UserA
return null;
}
}
+
+ /**
+ * According to <A
HREF="https://tools.ietf.org/html/rfc4252#section-8";>RFC 4252 - section 8</A>:
+ * <PRE>
+ * Both the server and the client should check whether the underlying
+ * transport layer provides confidentiality (i.e., if encryption is
+ * being used). If no confidentiality is provided ("none" cipher),
+ * password authentication SHOULD be disabled. If there is no
+ * confidentiality or no MAC, password change SHOULD be disabled.
+ * </PRE>
+ *
+ * @param session The {@link SessionContext} being used for authentication
+ * @return {@code true} if the context is not {@code null} and the ciphers
+ * have been established to anything other than "none".
+ * @see {@value #ALLOW_INSECURE_AUTH}
+ * @see SessionContext#isSecureSessionTransport(SessionContext)
+ */
+ static boolean isSecureAuthenticationTransport(SessionContext session) {
+ if (session == null) {
+ return false;
+ }
+
+ boolean allowInsecure = PropertyResolverUtils.getBooleanProperty(
+ session, ALLOW_INSECURE_AUTH, DEFAULT_ALLOW_INSECURE_AUTH);
+ if (allowInsecure) {
+ return true;
+ }
+
+ return SessionContext.isSecureSessionTransport(session);
+ }
}
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 4a39af7..6c5be5e 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
@@ -20,6 +20,7 @@ package org.apache.sshd.server.auth.password;
import org.apache.sshd.common.RuntimeSshException;
import org.apache.sshd.common.SshConstants;
+import org.apache.sshd.common.auth.UserAuthMethodFactory;
import org.apache.sshd.common.util.GenericUtils;
import org.apache.sshd.common.util.ValidateUtils;
import org.apache.sshd.common.util.buffer.Buffer;
@@ -27,7 +28,7 @@ import org.apache.sshd.server.auth.AbstractUserAuth;
import org.apache.sshd.server.session.ServerSession;
/**
- * TODO Add javadoc
+ * Implements the server-side "password" authentication mechanism
*
* @author <a href="mailto:d...@mina.apache.org";>Apache MINA SSHD Project</a>
*/
@@ -45,7 +46,8 @@ public class UserAuthPassword extends AbstractUserAuth {
boolean newPassword = buffer.getBoolean();
String password = buffer.getString();
if (newPassword) {
- return handleClientPasswordChangeRequest(buffer,
getServerSession(), getUsername(), password, buffer.getString());
+ return handleClientPasswordChangeRequest(
+ buffer, getServerSession(), getUsername(), password,
buffer.getString());
} else {
return checkPassword(buffer, getServerSession(), getUsername(),
password);
}
@@ -68,9 +70,18 @@ public class UserAuthPassword extends AbstractUserAuth {
* {@link PasswordChangeRequiredException} which is handled internally)
* @see #handleServerPasswordChangeRequest(Buffer, ServerSession, String,
String, PasswordChangeRequiredException)
*/
- protected Boolean checkPassword(Buffer buffer, ServerSession session,
String username, String password) throws Exception {
- PasswordAuthenticator auth = session.getPasswordAuthenticator();
+ protected Boolean checkPassword(
+ 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) {
log.debug("checkPassword({}) no password authenticator",
session);
@@ -84,13 +95,14 @@ public class UserAuthPassword extends AbstractUserAuth {
authed = auth.authenticate(username, password, session);
} catch (Error e) {
log.warn("checkPassword({}) failed ({}) to consult
authenticator: {}",
- session, e.getClass().getSimpleName(),
e.getMessage());
+ session, e.getClass().getSimpleName(), e.getMessage());
if (debugEnabled) {
log.debug("checkPassword(" + session + ") authenticator
failure details", e);
}
throw new RuntimeSshException(e);
}
+
if (debugEnabled) {
log.debug("checkPassword({}) authentication result: {}",
session, authed);
}
@@ -99,6 +111,7 @@ public class UserAuthPassword extends AbstractUserAuth {
if (debugEnabled) {
log.debug("checkPassword({}) password change required: {}",
session, e.getMessage());
}
+
return handleServerPasswordChangeRequest(buffer, session,
username, password, e);
}
}
@@ -143,7 +156,7 @@ public class UserAuthPassword extends AbstractUserAuth {
String lang = e.getLanguage();
if (log.isDebugEnabled()) {
log.debug("handlePasswordChangeRequest({}) password change
required - prompt={}, lang={}",
- session, prompt, lang);
+ session, prompt, lang);
}
buffer =
session.createBuffer(SshConstants.SSH_MSG_USERAUTH_PASSWD_CHANGEREQ,
diff --git
a/sshd-core/src/main/java/org/apache/sshd/server/session/AbstractServerSession.java
b/sshd-core/src/main/java/org/apache/sshd/server/session/AbstractServerSession.java
index 08d2d74..7da497a 100644
---
a/sshd-core/src/main/java/org/apache/sshd/server/session/AbstractServerSession.java
+++
b/sshd-core/src/main/java/org/apache/sshd/server/session/AbstractServerSession.java
@@ -242,7 +242,7 @@ public abstract class AbstractServerSession extends
AbstractSession implements S
FactoryManager factoryManager = getFactoryManager();
currentService = ServiceFactory.create(
factoryManager.getServiceFactories(),
- ValidateUtils.checkNotNullAndNotEmpty(name, "No service name"),
+ ValidateUtils.checkNotNullAndNotEmpty(name, "No service name
specified"),
this);
/*
* According to RFC4253:
@@ -404,7 +404,8 @@ public abstract class AbstractServerSession extends
AbstractSession implements S
* @param provided The available signature types - may be {@code
null}/empty
* @return The resolved proposal - {@code null} by default
*/
- protected String resolveEmptySignaturesProposal(Iterable<String>
supported, Iterable<String> provided) {
+ protected String resolveEmptySignaturesProposal(
+ Iterable<String> supported, Iterable<String> provided) {
if (log.isDebugEnabled()) {
log.debug("resolveEmptySignaturesProposal({})[{}] none of the keys
appears in supported list: {}",
this, provided, supported);
@@ -470,7 +471,8 @@ public abstract class AbstractServerSession extends
AbstractSession implements S
if (err != null) {
IoSession networkSession = getIoSession();
- networkSession.writePacket(new ByteArrayBuffer((err.getMessage() +
"\n").getBytes(StandardCharsets.UTF_8)))
+ networkSession.writePacket(
+ new ByteArrayBuffer((err.getMessage() +
"\n").getBytes(StandardCharsets.UTF_8)))
.addListener(future -> close(true));
throw err;
}
@@ -483,7 +485,8 @@ public abstract class AbstractServerSession extends
AbstractSession implements S
}
@Override
- protected void receiveKexInit(Map<KexProposalOption, String> proposal,
byte[] seed) throws IOException {
+ protected void receiveKexInit(Map<KexProposalOption, String> proposal,
byte[] seed)
+ throws IOException {
mergeProposals(clientProposal, proposal);
setClientKexData(seed);
}
@@ -538,9 +541,7 @@ public abstract class AbstractServerSession extends
AbstractSession implements S
}
/**
- * Returns the session id.
- *
- * @return The session id.
+ * @return The underlying {@link IoSession} id.
*/
public long getId() {
IoSession networkSession = getIoSession();
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 07e1a76..8082416 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
@@ -197,7 +197,8 @@ public class AuthenticationTest extends BaseTestSupport {
assertNotEquals("Non-different passwords",
oldPassword, newPassword);
return checkPassword(buffer, session,
username, newPassword);
} else {
- return
super.handleClientPasswordChangeRequest(buffer, session, username, oldPassword,
newPassword);
+ return super.handleClientPasswordChangeRequest(
+ buffer, session, username, oldPassword,
newPassword);
}
}
};
