You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mina.apache.org by lg...@apache.org on 2019/10/17 06:02:43 UTC
[mina-sshd] 03/06: [SSHD-948] Do not accept password authentication
if the session is not encrypted
This is an automated email from the ASF dual-hosted git repository.
lgoldstein pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 0f7091fe60b7c13db03556754a6f7ca9e975e54d
Author: Lyor Goldstein <lg...@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:dev@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:dev@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);
}
}
};