From 9ef56c44a3da38161741cf4dc62f49e786c8e5b1 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 25 Apr 2018 17:08:08 -0700 Subject: [PATCH] Merge PR12964 to not use verification if not keyed https://github.com/highfidelity/hifi/pull/12964 --- libraries/networking/src/HMACAuth.cpp | 20 +++++++++++++++++--- libraries/networking/src/LimitedNodeList.cpp | 20 ++++++++++++-------- libraries/networking/src/Node.cpp | 6 +++++- libraries/networking/src/Node.h | 4 ++-- 4 files changed, 36 insertions(+), 14 deletions(-) diff --git a/libraries/networking/src/HMACAuth.cpp b/libraries/networking/src/HMACAuth.cpp index 8aa2fea6a9..8e57978b5a 100644 --- a/libraries/networking/src/HMACAuth.cpp +++ b/libraries/networking/src/HMACAuth.cpp @@ -9,12 +9,14 @@ // See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html // +#include "HMACAuth.h" + #include #include -#include "HMACAuth.h" - #include +#include "NetworkLogging.h" +#include #if OPENSSL_VERSION_NUMBER >= 0x10100000 HMACAuth::HMACAuth(AuthMethod authMethod) @@ -86,9 +88,17 @@ HMACAuth::HMACHash HMACAuth::result() { HMACHash hashValue(EVP_MAX_MD_SIZE); unsigned int hashLen; QMutexLocker lock(&_lock); - if (HMAC_Final(_hmacContext, &hashValue[0], &hashLen)) { + + auto hmacResult = HMAC_Final(_hmacContext, &hashValue[0], &hashLen); + + if (hmacResult) { hashValue.resize((size_t)hashLen); + } else { + // the HMAC_FINAL call failed - should not be possible to get into this state + qCWarning(networking) << "Error occured calling HMAC_Final"; + assert(hmacResult); } + // Clear state for possible reuse. HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); return hashValue; @@ -97,6 +107,8 @@ HMACAuth::HMACHash HMACAuth::result() { bool HMACAuth::calculateHash(HMACHash& hashResult, const char* data, int dataLen) { QMutexLocker lock(&_lock); if (!HMAC_Update(_hmacContext, reinterpret_cast(data), dataLen)) { + qCWarning(networking) << "Error occured calling HMAC_Update"; + assert(false); return false; } @@ -108,5 +120,7 @@ bool HMACAuth::calculateHash(HMACHash& hashResult, const char* data, int dataLen HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); return true; } + qCWarning(networking) << "Error occured calling HMAC_Final"; + assert(false); return false; } diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index d66dd644d5..5457e206f5 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -331,10 +331,14 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe if (verifiedPacket && !ignoreVerification) { QByteArray packetHeaderHash = NLPacket::verificationHashInHeader(packet); - QByteArray expectedHash = NLPacket::hashForPacketAndHMAC(packet, sourceNode->getAuthenticateHash()); + QByteArray expectedHash; + auto sourceNodeHMACAuth = sourceNode->getAuthenticateHash(); + if (sourceNode->getAuthenticateHash()) { + expectedHash = NLPacket::hashForPacketAndHMAC(packet, *sourceNodeHMACAuth); + } // check if the HMAC-md5 hash in the header matches the hash we would expect - if (packetHeaderHash != expectedHash) { + if (!sourceNodeHMACAuth || packetHeaderHash != expectedHash) { static QMultiMap hashDebugSuppressMap; if (!hashDebugSuppressMap.contains(sourceID, headerType)) { @@ -396,7 +400,7 @@ qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const Node& emit dataSent(destinationNode.getType(), packet.getDataSize()); destinationNode.recordBytesSent(packet.getDataSize()); - return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), &destinationNode.getAuthenticateHash()); + return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), destinationNode.getAuthenticateHash()); } qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const HifiSockAddr& sockAddr, @@ -419,7 +423,7 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const Node& emit dataSent(destinationNode.getType(), packet->getDataSize()); destinationNode.recordBytesSent(packet->getDataSize()); - return sendPacket(std::move(packet), *activeSocket, &destinationNode.getAuthenticateHash()); + return sendPacket(std::move(packet), *activeSocket, destinationNode.getAuthenticateHash()); } else { qCDebug(networking) << "LimitedNodeList::sendPacket called without active socket for node" << destinationNode << "- not sending"; return ERROR_SENDING_PACKET_BYTES; @@ -447,14 +451,14 @@ qint64 LimitedNodeList::sendUnreliableUnorderedPacketList(NLPacketList& packetLi if (activeSocket) { qint64 bytesSent = 0; - auto& connectionHash = destinationNode.getAuthenticateHash(); + auto connectionHash = destinationNode.getAuthenticateHash(); // close the last packet in the list packetList.closeCurrentPacket(); while (!packetList._packets.empty()) { bytesSent += sendPacket(packetList.takeFront(), *activeSocket, - &connectionHash); + connectionHash); } emit dataSent(destinationNode.getType(), bytesSent); @@ -502,7 +506,7 @@ qint64 LimitedNodeList::sendPacketList(std::unique_ptr packetList, for (std::unique_ptr& packet : packetList->_packets) { NLPacket* nlPacket = static_cast(packet.get()); collectPacketStats(*nlPacket); - fillPacketHeader(*nlPacket, &destinationNode.getAuthenticateHash()); + fillPacketHeader(*nlPacket, destinationNode.getAuthenticateHash()); } return _nodeSocket.writePacketList(std::move(packetList), *activeSocket); @@ -525,7 +529,7 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const Node& auto& destinationSockAddr = (overridenSockAddr.isNull()) ? *destinationNode.getActiveSocket() : overridenSockAddr; - return sendPacket(std::move(packet), destinationSockAddr, &destinationNode.getAuthenticateHash()); + return sendPacket(std::move(packet), destinationSockAddr, destinationNode.getAuthenticateHash()); } int LimitedNodeList::updateNodeWithDataFromPacket(QSharedPointer message, SharedNodePointer sendingNode) { diff --git a/libraries/networking/src/Node.cpp b/libraries/networking/src/Node.cpp index 7979b36e30..8fd8131b10 100644 --- a/libraries/networking/src/Node.cpp +++ b/libraries/networking/src/Node.cpp @@ -89,7 +89,7 @@ Node::Node(const QUuid& uuid, NodeType_t type, const HifiSockAddr& publicSocket, const HifiSockAddr& localSocket, QObject* parent) : NetworkPeer(uuid, publicSocket, localSocket, parent), _type(type), - _authenticateHash(new HMACAuth), _pingMs(-1), // "Uninitialized" + _pingMs(-1), // "Uninitialized" _clockSkewUsec(0), _mutex(), _clockSkewMovingPercentile(30, 0.8f) // moving 80th percentile of 30 samples @@ -201,6 +201,10 @@ void Node::setConnectionSecret(const QUuid& connectionSecret) { return; } + if (!_authenticateHash) { + _authenticateHash.reset(new HMACAuth()); + } + _connectionSecret = connectionSecret; _authenticateHash->setKey(_connectionSecret); } diff --git a/libraries/networking/src/Node.h b/libraries/networking/src/Node.h index 5b3b559582..bcfe525aa8 100644 --- a/libraries/networking/src/Node.h +++ b/libraries/networking/src/Node.h @@ -57,7 +57,7 @@ public: const QUuid& getConnectionSecret() const { return _connectionSecret; } void setConnectionSecret(const QUuid& connectionSecret); - HMACAuth& getAuthenticateHash() const { return *_authenticateHash; } + HMACAuth* getAuthenticateHash() const { return _authenticateHash.get(); } NodeData* getLinkedData() const { return _linkedData.get(); } void setLinkedData(std::unique_ptr linkedData) { _linkedData = std::move(linkedData); } @@ -99,7 +99,7 @@ private: NodeType_t _type; QUuid _connectionSecret; - std::unique_ptr _authenticateHash; + std::unique_ptr _authenticateHash { nullptr }; std::unique_ptr _linkedData; bool _isReplicated { false }; int _pingMs;