From 9d90809e6db7889a0a0a46f2eaa67bc08448358b Mon Sep 17 00:00:00 2001 From: Christian Kandeler Date: Thu, 28 Apr 2011 10:57:17 +0200 Subject: [PATCH] SSH: Fix key re-exchange bug. We used the encrypted client payload by mistake. The bug only manifests itself from the second key exchange on, because during the first one, no encryption is active, so the "encrypted" payload is the same as the non-encrypted one. --- src/libs/utils/ssh/sshkeyexchange.cpp | 35 ++++++++++++++++++++---- src/libs/utils/ssh/sshoutgoingpacket.cpp | 4 ++- src/libs/utils/ssh/sshoutgoingpacket_p.h | 2 +- src/libs/utils/ssh/sshpacket.cpp | 8 ++---- src/libs/utils/ssh/sshpacket_p.h | 3 +- src/libs/utils/ssh/sshsendfacility.cpp | 6 ++-- src/libs/utils/ssh/sshsendfacility_p.h | 2 +- 7 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/libs/utils/ssh/sshkeyexchange.cpp b/src/libs/utils/ssh/sshkeyexchange.cpp index 1112cd2b974..1165569ea00 100644 --- a/src/libs/utils/ssh/sshkeyexchange.cpp +++ b/src/libs/utils/ssh/sshkeyexchange.cpp @@ -44,6 +44,9 @@ #include #include +#ifdef CREATOR_SSH_DEBUG +#include +#endif #include using namespace Botan; @@ -65,6 +68,18 @@ namespace { Q_UNUSED(list); #endif } + +#ifdef CREATOR_SSH_DEBUG + void printData(const char *name, const QByteArray &data) + { + std::cerr << std::hex; + qDebug("The client thinks the %s has length %d and is:", name, data.count()); + for (int i = 0; i < data.count(); ++i) + std::cerr << (static_cast(data.at(i)) & 0xff) << ' '; + std::cerr << std::endl; + } +#endif + } // anonymous namespace SshKeyExchange::SshKeyExchange(SshSendFacility &sendFacility) @@ -77,9 +92,7 @@ SshKeyExchange::~SshKeyExchange() {} void SshKeyExchange::sendKexInitPacket(const QByteArray &serverId) { m_serverId = serverId; - const AbstractSshPacket::Payload &payload - = m_sendFacility.sendKeyExchangeInitPacket(); - m_clientKexInitPayload = QByteArray(payload.data, payload.size); + m_clientKexInitPayload = m_sendFacility.sendKeyExchangeInitPacket(); } bool SshKeyExchange::sendDhInitPacket(const SshIncomingPacket &serverKexInit) @@ -132,8 +145,7 @@ bool SshKeyExchange::sendDhInitPacket(const SshIncomingPacket &serverKexInit) m_dhKey.reset(new DH_PrivateKey(rng, DL_Group(botanKeyExchangeAlgoName(keyAlgo)))); - const AbstractSshPacket::Payload &payload = serverKexInit.payLoad(); - m_serverKexInitPayload = QByteArray(payload.data, payload.size); + m_serverKexInitPayload = serverKexInit.payLoad(); m_sendFacility.sendKeyDhInitPacket(m_dhKey->get_y()); return kexInitParams.firstKexPacketFollows; } @@ -165,6 +177,19 @@ void SshKeyExchange::sendNewKeysPacket(const SshIncomingPacket &dhReply, concatenatedData.size()); m_h = convertByteArray(hashResult); +#ifdef CREATOR_SSH_DEBUG + printData("Client Id", AbstractSshPacket::encodeString(clientId)); + printData("Server Id", AbstractSshPacket::encodeString(m_serverId)); + printData("Client Payload", AbstractSshPacket::encodeString(m_clientKexInitPayload)); + printData("Server payload", AbstractSshPacket::encodeString(m_serverKexInitPayload)); + printData("K_S", reply.k_s); + printData("y", AbstractSshPacket::encodeMpInt(m_dhKey->get_y())); + printData("f", AbstractSshPacket::encodeMpInt(reply.f)); + printData("K", m_k); + printData("Concatenated data", concatenatedData); + printData("H", m_h); +#endif // CREATOR_SSH_DEBUG + QScopedPointer sigKey; QScopedPointer verifier; if (m_serverHostKeyAlgo == SshCapabilities::PubKeyDss) { diff --git a/src/libs/utils/ssh/sshoutgoingpacket.cpp b/src/libs/utils/ssh/sshoutgoingpacket.cpp index 8983ca4c701..2604b900728 100644 --- a/src/libs/utils/ssh/sshoutgoingpacket.cpp +++ b/src/libs/utils/ssh/sshoutgoingpacket.cpp @@ -55,7 +55,7 @@ quint32 SshOutgoingPacket::macLength() const return m_encrypter.macLength(); } -void SshOutgoingPacket::generateKeyExchangeInitPacket() +QByteArray SshOutgoingPacket::generateKeyExchangeInitPacket() { const QByteArray &supportedkeyExchangeMethods = encodeNameList(SshCapabilities::KeyExchangeMethods); @@ -81,7 +81,9 @@ void SshOutgoingPacket::generateKeyExchangeInitPacket() m_data.append(supportedLanguages).append(supportedLanguages); appendBool(false); // No guessed packet. m_data.append(QByteArray(4, 0)); // Reserved. + QByteArray payload = m_data.mid(PayloadOffset); finalize(); + return payload; } void SshOutgoingPacket::generateKeyDhInitPacket(const Botan::BigInt &e) diff --git a/src/libs/utils/ssh/sshoutgoingpacket_p.h b/src/libs/utils/ssh/sshoutgoingpacket_p.h index e103b9c7c39..e527d6e02a7 100644 --- a/src/libs/utils/ssh/sshoutgoingpacket_p.h +++ b/src/libs/utils/ssh/sshoutgoingpacket_p.h @@ -48,7 +48,7 @@ public: SshOutgoingPacket(const SshEncryptionFacility &encrypter, const quint32 &seqNr); - void generateKeyExchangeInitPacket(); + QByteArray generateKeyExchangeInitPacket(); // Returns payload. void generateKeyDhInitPacket(const Botan::BigInt &e); void generateNewKeysPacket(); void generateDisconnectPacket(SshErrorCode reason, diff --git a/src/libs/utils/ssh/sshpacket.cpp b/src/libs/utils/ssh/sshpacket.cpp index 9b5fdfdbaf6..0b97cd6c9b0 100644 --- a/src/libs/utils/ssh/sshpacket.cpp +++ b/src/libs/utils/ssh/sshpacket.cpp @@ -86,12 +86,10 @@ SshPacketType AbstractSshPacket::type() const return static_cast(m_data.at(TypeOffset)); } -AbstractSshPacket::Payload AbstractSshPacket::payLoad() const +QByteArray AbstractSshPacket::payLoad() const { - Payload p; - p.data = m_data.constData() + PayloadOffset; - p.size = length() - paddingLength() - 1; - return p; + return QByteArray(m_data.constData() + PayloadOffset, + length() - paddingLength() - 1); } void AbstractSshPacket::printRawBytes() const diff --git a/src/libs/utils/ssh/sshpacket_p.h b/src/libs/utils/ssh/sshpacket_p.h index bf70a8d3407..fab9243b9cc 100644 --- a/src/libs/utils/ssh/sshpacket_p.h +++ b/src/libs/utils/ssh/sshpacket_p.h @@ -113,8 +113,7 @@ public: const QByteArray &rawData() const { return m_data; } - struct Payload { const char *data; quint32 size; }; - Payload payLoad() const; + QByteArray payLoad() const; protected: AbstractSshPacket(); diff --git a/src/libs/utils/ssh/sshsendfacility.cpp b/src/libs/utils/ssh/sshsendfacility.cpp index fc816c1033a..c0b433fbdf1 100644 --- a/src/libs/utils/ssh/sshsendfacility.cpp +++ b/src/libs/utils/ssh/sshsendfacility.cpp @@ -74,11 +74,11 @@ void SshSendFacility::createAuthenticationKey(const QByteArray &privKeyFileConte m_encrypter.createAuthenticationKey(privKeyFileContents); } -SshOutgoingPacket::Payload SshSendFacility::sendKeyExchangeInitPacket() +QByteArray SshSendFacility::sendKeyExchangeInitPacket() { - m_outgoingPacket.generateKeyExchangeInitPacket(); + const QByteArray &payLoad = m_outgoingPacket.generateKeyExchangeInitPacket(); sendPacket(); - return m_outgoingPacket.payLoad(); + return payLoad; } void SshSendFacility::sendKeyDhInitPacket(const Botan::BigInt &e) diff --git a/src/libs/utils/ssh/sshsendfacility_p.h b/src/libs/utils/ssh/sshsendfacility_p.h index d14b7391020..993ce017d11 100644 --- a/src/libs/utils/ssh/sshsendfacility_p.h +++ b/src/libs/utils/ssh/sshsendfacility_p.h @@ -55,7 +55,7 @@ public: void recreateKeys(const SshKeyExchange &keyExchange); void createAuthenticationKey(const QByteArray &privKeyFileContents); - SshOutgoingPacket::Payload sendKeyExchangeInitPacket(); + QByteArray sendKeyExchangeInitPacket(); void sendKeyDhInitPacket(const Botan::BigInt &e); void sendNewKeysPacket(); void sendDisconnectPacket(SshErrorCode reason,