From e2cb3acdaa3b2870dc7c234785d68366497f7bcc Mon Sep 17 00:00:00 2001
From: Christian Kandeler <christian.kandeler@digia.com>
Date: Wed, 26 Sep 2012 10:57:19 +0200
Subject: [PATCH] SSH: Fix parsing of server id.
The current implementation is a bit sloppy, so one can construct certain
invalid strings that will be accepted as well as certain valid ones that
will be rejected. The new checks are much more precise.
Most importantly in practice, the version string "1.99" sent by some
older servers is now accepted, as required by the RFC.
Change-Id: Ib35c27d3a3bc4aea259b9a0dad66817435d95c0a
Reviewed-by: Tobias Hunger <tobias.hunger@digia.com>
---
src/libs/ssh/sshconnection.cpp | 78 ++++++++++++++++++++++++++--------
src/libs/ssh/sshconnection_p.h | 1 +
2 files changed, 61 insertions(+), 18 deletions(-)
diff --git a/src/libs/ssh/sshconnection.cpp b/src/libs/ssh/sshconnection.cpp
index ebf233d3b6d..7d748d02677 100644
--- a/src/libs/ssh/sshconnection.cpp
+++ b/src/libs/ssh/sshconnection.cpp
@@ -44,6 +44,7 @@
#include <QMutex>
#include <QMutexLocker>
#include <QNetworkProxy>
+#include <QRegExp>
#include <QTcpSocket>
/*!
@@ -320,7 +321,7 @@ void SshConnectionPrivate::handleIncomingData()
qDebug("state = %d, remote data size = %d", m_state,
m_incomingData.count());
#endif
- if (m_state == SocketConnected)
+ if (m_serverId.isEmpty())
handleServerId();
handlePackets();
} catch (SshServerException &e) {
@@ -335,39 +336,78 @@ void SshConnectionPrivate::handleIncomingData()
}
}
+// RFC 4253, 4.2.
void SshConnectionPrivate::handleServerId()
{
#ifdef CREATOR_SSH_DEBUG
qDebug("%s: incoming data size = %d, incoming data = '%s'",
Q_FUNC_INFO, m_incomingData.count(), m_incomingData.data());
#endif
- const int idOffset = m_incomingData.indexOf("SSH-");
- if (idOffset == -1)
+ const int newLinePos = m_incomingData.indexOf('\n');
+ if (newLinePos == -1)
+ return; // Not enough data yet.
+
+ // Lines not starting with "SSH-" are ignored.
+ if (!m_incomingData.startsWith("SSH-")) {
+ m_incomingData.remove(0, newLinePos + 1);
+ m_serverHasSentDataBeforeId = true;
return;
- m_incomingData.remove(0, idOffset);
- if (m_incomingData.size() < 7)
- return;
- const QByteArray &version = m_incomingData.mid(4, 3);
- if (version != "2.0") {
+ }
+
+ if (newLinePos > 255 - 1) {
+ throw SshServerException(SSH_DISCONNECT_PROTOCOL_ERROR,
+ "Identification string too long.",
+ tr("Server identification string is %1 characters long, but the maximum "
+ "allowed length is 255.").arg(newLinePos + 1));
+ }
+
+ const bool hasCarriageReturn = m_incomingData.at(newLinePos - 1) == '\r';
+ m_serverId = m_incomingData.left(newLinePos);
+ if (hasCarriageReturn)
+ m_serverId.chop(1);
+ m_incomingData.remove(0, newLinePos + 1);
+
+ if (m_serverId.contains('\0')) {
+ throw SshServerException(SSH_DISCONNECT_PROTOCOL_ERROR,
+ "Identification string contains illegal NUL character.",
+ tr("Server identification string contains illegal NUL character."));
+ }
+
+ // "printable US-ASCII characters, with the exception of whitespace characters
+ // and the minus sign"
+ QString legalString = QLatin1String("[]!\"#$!&'()*+,./0-9:;<=>?@A-Z[\\\\^_`a-z{|}~]+");
+ const QRegExp versionIdpattern(QString::fromLatin1("SSH-(%1)-%1(?: .+)?").arg(legalString));
+ if (!versionIdpattern.exactMatch(QString::fromLatin1(m_serverId))) {
+ throw SshServerException(SSH_DISCONNECT_PROTOCOL_ERROR,
+ "Identification string is invalid.",
+ tr("Server Identification string '%1' is invalid.")
+ .arg(QString::fromLatin1(m_serverId)));
+ }
+ const QString serverProtoVersion = versionIdpattern.cap(1);
+ if (serverProtoVersion != QLatin1String("2.0") && serverProtoVersion != QLatin1String("1.99")) {
throw SshServerException(SSH_DISCONNECT_PROTOCOL_VERSION_NOT_SUPPORTED,
"Invalid protocol version.",
- tr("Invalid protocol version: Expected '2.0', got '%1'.")
- .arg(SshPacketParser::asUserString(version)));
+ tr("Server protocol version is '%1', but needs to be 2.0 or 1.99.")
+ .arg(serverProtoVersion));
}
- const int endOffset = m_incomingData.indexOf("\r\n");
- if (endOffset == -1)
- return;
- if (m_incomingData.at(7) != '-') {
+
+ // Disable this check to accept older OpenSSH servers that do this wrong.
+ if (serverProtoVersion == QLatin1String("2.0") && !hasCarriageReturn) {
+ throw SshServerException(SSH_DISCONNECT_PROTOCOL_ERROR,
+ "Identification string is invalid.",
+ tr("Server identification string is invalid (missing carriage return)."));
+ }
+
+ if (serverProtoVersion == QLatin1String("1.99") && m_serverHasSentDataBeforeId) {
throw SshServerException(SSH_DISCONNECT_PROTOCOL_ERROR,
- "Invalid server id.", tr("Invalid server id '%1'.")
- .arg(SshPacketParser::asUserString(m_incomingData)));
+ "No extra data preceding identification string allowed for 1.99.",
+ tr("Server reports protocol version 1.99, but sends data "
+ "before the identification string, which is not allowed."));
}
m_keyExchange.reset(new SshKeyExchange(m_sendFacility));
- m_serverId = m_incomingData.left(endOffset);
m_keyExchange->sendKexInitPacket(m_serverId);
m_keyExchangeState = KexInitSent;
- m_incomingData.remove(0, endOffset + 2);
}
void SshConnectionPrivate::handlePackets()
@@ -645,6 +685,8 @@ void SshConnectionPrivate::connectToHost()
m_error = SshNoError;
m_ignoreNextPacket = false;
m_errorString.clear();
+ m_serverId.clear();
+ m_serverHasSentDataBeforeId = false;
try {
if (m_connParams.authenticationType == SshConnectionParameters::AuthenticationByKey)
diff --git a/src/libs/ssh/sshconnection_p.h b/src/libs/ssh/sshconnection_p.h
index bdf0c26c960..797d7364374 100644
--- a/src/libs/ssh/sshconnection_p.h
+++ b/src/libs/ssh/sshconnection_p.h
@@ -165,6 +165,7 @@ private:
SshConnection *m_conn;
quint64 m_lastInvalidMsgSeqNr;
QByteArray m_serverId;
+ bool m_serverHasSentDataBeforeId;
};
} // namespace Internal
--
GitLab