From 21a9525a737e3db268cabdd82c0c60504a54444f Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 24 Apr 2018 17:52:12 -0700 Subject: [PATCH 1/8] Version of HMACAuth with exposed lock Client is responsible for locking between adding and retrieving. Fixes a likely race. --- libraries/networking/src/HMACAuth.cpp | 11 ++++++----- libraries/networking/src/HMACAuth.h | 2 ++ libraries/networking/src/LimitedNodeList.cpp | 5 +++-- libraries/networking/src/NLPacket.cpp | 1 + 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/libraries/networking/src/HMACAuth.cpp b/libraries/networking/src/HMACAuth.cpp index 42b5c48d93..e276e64b40 100644 --- a/libraries/networking/src/HMACAuth.cpp +++ b/libraries/networking/src/HMACAuth.cpp @@ -68,7 +68,7 @@ bool HMACAuth::setKey(const char* keyValue, int keyLen) { return false; } - QMutexLocker lock(&_lock); + //QMutexLocker lock(&_lock); return (bool) HMAC_Init_ex(_hmacContext, keyValue, keyLen, sslStruct, nullptr); } @@ -78,16 +78,17 @@ bool HMACAuth::setKey(const QUuid& uidKey) { } bool HMACAuth::addData(const char* data, int dataLen) { - QMutexLocker lock(&_lock); + //QMutexLocker lock(&_lock); return (bool) HMAC_Update(_hmacContext, reinterpret_cast(data), dataLen); } HMACAuth::HMACHash HMACAuth::result() { HMACHash hashValue(EVP_MAX_MD_SIZE); unsigned int hashLen; - QMutexLocker lock(&_lock); - HMAC_Final(_hmacContext, &hashValue[0], &hashLen); - hashValue.resize((size_t) hashLen); + //QMutexLocker lock(&_lock); + if (HMAC_Final(_hmacContext, &hashValue[0], &hashLen)) { + hashValue.resize((size_t)hashLen); + } // Clear state for possible reuse. HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); return hashValue; diff --git a/libraries/networking/src/HMACAuth.h b/libraries/networking/src/HMACAuth.h index 0bf7a86ec1..ba1ec78214 100644 --- a/libraries/networking/src/HMACAuth.h +++ b/libraries/networking/src/HMACAuth.h @@ -26,6 +26,8 @@ public: explicit HMACAuth(AuthMethod authMethod = MD5); ~HMACAuth(); + QMutex& getLock() { return _lock; } + bool setKey(const char* keyValue, int keyLen); bool setKey(const QUuid& uidKey); bool addData(const char* data, int dataLen); diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 8d177ca534..d66dd644d5 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -333,13 +333,14 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe QByteArray packetHeaderHash = NLPacket::verificationHashInHeader(packet); QByteArray expectedHash = NLPacket::hashForPacketAndHMAC(packet, sourceNode->getAuthenticateHash()); - // check if the md5 hash in the header matches the hash we would expect + // check if the HMAC-md5 hash in the header matches the hash we would expect if (packetHeaderHash != expectedHash) { static QMultiMap hashDebugSuppressMap; if (!hashDebugSuppressMap.contains(sourceID, headerType)) { - qCDebug(networking) << packetHeaderHash << expectedHash; qCDebug(networking) << "Packet hash mismatch on" << headerType << "- Sender" << sourceID; + qCDebug(networking) << "Packet len:" << packet.getDataSize() << "Expected hash:" << + expectedHash.toHex() << "Actual:" << packetHeaderHash.toHex(); hashDebugSuppressMap.insert(sourceID, headerType); } diff --git a/libraries/networking/src/NLPacket.cpp b/libraries/networking/src/NLPacket.cpp index 3355e1cd6b..09a7b78840 100644 --- a/libraries/networking/src/NLPacket.cpp +++ b/libraries/networking/src/NLPacket.cpp @@ -157,6 +157,7 @@ QByteArray NLPacket::hashForPacketAndHMAC(const udt::Packet& packet, HMACAuth& h + NUM_BYTES_LOCALID + NUM_BYTES_MD5_HASH; // add the packet payload and the connection UUID + QMutexLocker hashLock(&hash.getLock()); hash.addData(packet.getData() + offset, packet.getDataSize() - offset); auto hashResult { hash.result() }; return QByteArray((const char*) hashResult.data(), (int) hashResult.size()); From 3233cc43ac8d12a48e500130c6b7a91fbcff448c Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 25 Apr 2018 10:30:50 -0700 Subject: [PATCH 2/8] Revert "Merge pull request #12981 from birarda/bug/revert-hmac" This reverts commit 91b93db0c5f27a5b5c0282780ee56102078224be, reversing changes made to 6062f21da114f34164ba80db7e31bf45b959626e. --- libraries/networking/src/HMACAuth.cpp | 94 +++++++++++++++++++ libraries/networking/src/HMACAuth.h | 40 ++++++++ libraries/networking/src/LimitedNodeList.cpp | 36 +++---- libraries/networking/src/LimitedNodeList.h | 10 +- libraries/networking/src/NLPacket.cpp | 16 ++-- libraries/networking/src/NLPacket.h | 8 +- libraries/networking/src/Node.cpp | 14 ++- libraries/networking/src/Node.h | 5 +- .../networking/src/udt/PacketHeaders.cpp | 2 +- 9 files changed, 186 insertions(+), 39 deletions(-) create mode 100644 libraries/networking/src/HMACAuth.cpp create mode 100644 libraries/networking/src/HMACAuth.h diff --git a/libraries/networking/src/HMACAuth.cpp b/libraries/networking/src/HMACAuth.cpp new file mode 100644 index 0000000000..42b5c48d93 --- /dev/null +++ b/libraries/networking/src/HMACAuth.cpp @@ -0,0 +1,94 @@ +// +// HMACAuth.cpp +// libraries/networking/src +// +// Created by Simon Walton on 3/19/2018. +// Copyright 2018 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#include +#include + +#include "HMACAuth.h" + +#include + +#if OPENSSL_VERSION_NUMBER >= 0x10100000 +HMACAuth::HMACAuth(AuthMethod authMethod) + : _hmacContext(HMAC_CTX_new()) + , _authMethod(authMethod) { } + +HMACAuth::~HMACAuth() +{ + HMAC_CTX_free(_hmacContext); +} + +#else + +HMACAuth::HMACAuth(AuthMethod authMethod) + : _hmacContext(new HMAC_CTX()) + , _authMethod(authMethod) { + HMAC_CTX_init(_hmacContext); +} + +HMACAuth::~HMACAuth() { + HMAC_CTX_cleanup(_hmacContext); + delete _hmacContext; +} +#endif + +bool HMACAuth::setKey(const char* keyValue, int keyLen) { + const EVP_MD* sslStruct = nullptr; + + switch (_authMethod) { + case MD5: + sslStruct = EVP_md5(); + break; + + case SHA1: + sslStruct = EVP_sha1(); + break; + + case SHA224: + sslStruct = EVP_sha224(); + break; + + case SHA256: + sslStruct = EVP_sha256(); + break; + + case RIPEMD160: + sslStruct = EVP_ripemd160(); + break; + + default: + return false; + } + + QMutexLocker lock(&_lock); + return (bool) HMAC_Init_ex(_hmacContext, keyValue, keyLen, sslStruct, nullptr); +} + +bool HMACAuth::setKey(const QUuid& uidKey) { + const QByteArray rfcBytes(uidKey.toRfc4122()); + return setKey(rfcBytes.constData(), rfcBytes.length()); +} + +bool HMACAuth::addData(const char* data, int dataLen) { + QMutexLocker lock(&_lock); + return (bool) HMAC_Update(_hmacContext, reinterpret_cast(data), dataLen); +} + +HMACAuth::HMACHash HMACAuth::result() { + HMACHash hashValue(EVP_MAX_MD_SIZE); + unsigned int hashLen; + QMutexLocker lock(&_lock); + HMAC_Final(_hmacContext, &hashValue[0], &hashLen); + hashValue.resize((size_t) hashLen); + // Clear state for possible reuse. + HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); + return hashValue; +} diff --git a/libraries/networking/src/HMACAuth.h b/libraries/networking/src/HMACAuth.h new file mode 100644 index 0000000000..0bf7a86ec1 --- /dev/null +++ b/libraries/networking/src/HMACAuth.h @@ -0,0 +1,40 @@ +// +// HMACAuth.h +// libraries/networking/src +// +// Created by Simon Walton on 3/19/2018. +// Copyright 2018 High Fidelity, Inc. +// +// Distributed under the Apache License, Version 2.0. +// See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html +// + +#ifndef hifi_HMACAuth_h +#define hifi_HMACAuth_h + +#include +#include +#include + +class QUuid; + +class HMACAuth { +public: + enum AuthMethod { MD5, SHA1, SHA224, SHA256, RIPEMD160 }; + using HMACHash = std::vector; + + explicit HMACAuth(AuthMethod authMethod = MD5); + ~HMACAuth(); + + bool setKey(const char* keyValue, int keyLen); + bool setKey(const QUuid& uidKey); + bool addData(const char* data, int dataLen); + HMACHash result(); + +private: + QMutex _lock; + struct hmac_ctx_st* _hmacContext; + AuthMethod _authMethod; +}; + +#endif // hifi_HMACAuth_h diff --git a/libraries/networking/src/LimitedNodeList.cpp b/libraries/networking/src/LimitedNodeList.cpp index 92385e99b0..8d177ca534 100644 --- a/libraries/networking/src/LimitedNodeList.cpp +++ b/libraries/networking/src/LimitedNodeList.cpp @@ -36,6 +36,7 @@ #include "HifiSockAddr.h" #include "NetworkLogging.h" #include "udt/Packet.h" +#include "HMACAuth.h" static Setting::Handle LIMITED_NODELIST_LOCAL_PORT("LimitedNodeList.LocalPort", 0); @@ -330,7 +331,7 @@ bool LimitedNodeList::packetSourceAndHashMatchAndTrackBandwidth(const udt::Packe if (verifiedPacket && !ignoreVerification) { QByteArray packetHeaderHash = NLPacket::verificationHashInHeader(packet); - QByteArray expectedHash = NLPacket::hashForPacketAndSecret(packet, sourceNode->getConnectionSecret()); + QByteArray expectedHash = NLPacket::hashForPacketAndHMAC(packet, sourceNode->getAuthenticateHash()); // check if the md5 hash in the header matches the hash we would expect if (packetHeaderHash != expectedHash) { @@ -370,15 +371,15 @@ void LimitedNodeList::collectPacketStats(const NLPacket& packet) { _numCollectedBytes += packet.getDataSize(); } -void LimitedNodeList::fillPacketHeader(const NLPacket& packet, const QUuid& connectionSecret) { +void LimitedNodeList::fillPacketHeader(const NLPacket& packet, HMACAuth* hmacAuth) { if (!PacketTypeEnum::getNonSourcedPackets().contains(packet.getType())) { packet.writeSourceID(getSessionLocalID()); } - if (!connectionSecret.isNull() + if (hmacAuth && !PacketTypeEnum::getNonSourcedPackets().contains(packet.getType()) && !PacketTypeEnum::getNonVerifiedPackets().contains(packet.getType())) { - packet.writeVerificationHashGivenSecret(connectionSecret); + packet.writeVerificationHash(*hmacAuth); } } @@ -394,17 +395,17 @@ qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const Node& emit dataSent(destinationNode.getType(), packet.getDataSize()); destinationNode.recordBytesSent(packet.getDataSize()); - return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), destinationNode.getConnectionSecret()); + return sendUnreliablePacket(packet, *destinationNode.getActiveSocket(), &destinationNode.getAuthenticateHash()); } qint64 LimitedNodeList::sendUnreliablePacket(const NLPacket& packet, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret) { + HMACAuth* hmacAuth) { Q_ASSERT(!packet.isPartOfMessage()); Q_ASSERT_X(!packet.isReliable(), "LimitedNodeList::sendUnreliablePacket", "Trying to send a reliable packet unreliably."); collectPacketStats(packet); - fillPacketHeader(packet, connectionSecret); + fillPacketHeader(packet, hmacAuth); return _nodeSocket.writePacket(packet, sockAddr); } @@ -417,7 +418,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.getConnectionSecret()); + 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; @@ -425,18 +426,18 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const Node& } qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret) { + HMACAuth* hmacAuth) { Q_ASSERT(!packet->isPartOfMessage()); if (packet->isReliable()) { collectPacketStats(*packet); - fillPacketHeader(*packet, connectionSecret); + fillPacketHeader(*packet, hmacAuth); auto size = packet->getDataSize(); _nodeSocket.writePacket(std::move(packet), sockAddr); return size; } else { - return sendUnreliablePacket(*packet, sockAddr, connectionSecret); + return sendUnreliablePacket(*packet, sockAddr, hmacAuth); } } @@ -445,13 +446,14 @@ qint64 LimitedNodeList::sendUnreliableUnorderedPacketList(NLPacketList& packetLi if (activeSocket) { qint64 bytesSent = 0; - auto connectionSecret = destinationNode.getConnectionSecret(); + auto& connectionHash = destinationNode.getAuthenticateHash(); // close the last packet in the list packetList.closeCurrentPacket(); while (!packetList._packets.empty()) { - bytesSent += sendPacket(packetList.takeFront(), *activeSocket, connectionSecret); + bytesSent += sendPacket(packetList.takeFront(), *activeSocket, + &connectionHash); } emit dataSent(destinationNode.getType(), bytesSent); @@ -464,14 +466,14 @@ qint64 LimitedNodeList::sendUnreliableUnorderedPacketList(NLPacketList& packetLi } qint64 LimitedNodeList::sendUnreliableUnorderedPacketList(NLPacketList& packetList, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret) { + HMACAuth* hmacAuth) { qint64 bytesSent = 0; // close the last packet in the list packetList.closeCurrentPacket(); while (!packetList._packets.empty()) { - bytesSent += sendPacket(packetList.takeFront(), sockAddr, connectionSecret); + bytesSent += sendPacket(packetList.takeFront(), sockAddr, hmacAuth); } return bytesSent; @@ -499,7 +501,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.getConnectionSecret()); + fillPacketHeader(*nlPacket, &destinationNode.getAuthenticateHash()); } return _nodeSocket.writePacketList(std::move(packetList), *activeSocket); @@ -522,7 +524,7 @@ qint64 LimitedNodeList::sendPacket(std::unique_ptr packet, const Node& auto& destinationSockAddr = (overridenSockAddr.isNull()) ? *destinationNode.getActiveSocket() : overridenSockAddr; - return sendPacket(std::move(packet), destinationSockAddr, destinationNode.getConnectionSecret()); + return sendPacket(std::move(packet), destinationSockAddr, &destinationNode.getAuthenticateHash()); } int LimitedNodeList::updateNodeWithDataFromPacket(QSharedPointer message, SharedNodePointer sendingNode) { diff --git a/libraries/networking/src/LimitedNodeList.h b/libraries/networking/src/LimitedNodeList.h index 3d6fd0cd91..05374bbfbb 100644 --- a/libraries/networking/src/LimitedNodeList.h +++ b/libraries/networking/src/LimitedNodeList.h @@ -138,19 +138,17 @@ public: // use sendUnreliablePacket to send an unreliable packet (that you do not need to move) // either to a node (via its active socket) or to a manual sockaddr qint64 sendUnreliablePacket(const NLPacket& packet, const Node& destinationNode); - qint64 sendUnreliablePacket(const NLPacket& packet, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret = QUuid()); + qint64 sendUnreliablePacket(const NLPacket& packet, const HifiSockAddr& sockAddr, HMACAuth* hmacAuth = nullptr); // use sendPacket to send a moved unreliable or reliable NL packet to a node's active socket or manual sockaddr qint64 sendPacket(std::unique_ptr packet, const Node& destinationNode); - qint64 sendPacket(std::unique_ptr packet, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret = QUuid()); + qint64 sendPacket(std::unique_ptr packet, const HifiSockAddr& sockAddr, HMACAuth* hmacAuth = nullptr); // use sendUnreliableUnorderedPacketList to unreliably send separate packets from the packet list // either to a node's active socket or to a manual sockaddr qint64 sendUnreliableUnorderedPacketList(NLPacketList& packetList, const Node& destinationNode); qint64 sendUnreliableUnorderedPacketList(NLPacketList& packetList, const HifiSockAddr& sockAddr, - const QUuid& connectionSecret = QUuid()); + HMACAuth* hmacAuth = nullptr); // use sendPacketList to send reliable packet lists (ordered or unordered) to a node's active socket // or to a manual sock addr @@ -372,7 +370,7 @@ protected: qint64 writePacket(const NLPacket& packet, const HifiSockAddr& destinationSockAddr, const QUuid& connectionSecret = QUuid()); void collectPacketStats(const NLPacket& packet); - void fillPacketHeader(const NLPacket& packet, const QUuid& connectionSecret = QUuid()); + void fillPacketHeader(const NLPacket& packet, HMACAuth* hmacAuth = nullptr); void setLocalSocket(const HifiSockAddr& sockAddr); diff --git a/libraries/networking/src/NLPacket.cpp b/libraries/networking/src/NLPacket.cpp index ac3fbc966b..3355e1cd6b 100644 --- a/libraries/networking/src/NLPacket.cpp +++ b/libraries/networking/src/NLPacket.cpp @@ -11,6 +11,8 @@ #include "NLPacket.h" +#include "HMACAuth.h" + int NLPacket::localHeaderSize(PacketType type) { bool nonSourced = PacketTypeEnum::getNonSourcedPackets().contains(type); bool nonVerified = PacketTypeEnum::getNonVerifiedPackets().contains(type); @@ -150,18 +152,14 @@ QByteArray NLPacket::verificationHashInHeader(const udt::Packet& packet) { return QByteArray(packet.getData() + offset, NUM_BYTES_MD5_HASH); } -QByteArray NLPacket::hashForPacketAndSecret(const udt::Packet& packet, const QUuid& connectionSecret) { - QCryptographicHash hash(QCryptographicHash::Md5); - +QByteArray NLPacket::hashForPacketAndHMAC(const udt::Packet& packet, HMACAuth& hash) { int offset = Packet::totalHeaderSize(packet.isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion) + NUM_BYTES_LOCALID + NUM_BYTES_MD5_HASH; // add the packet payload and the connection UUID hash.addData(packet.getData() + offset, packet.getDataSize() - offset); - hash.addData(connectionSecret.toRfc4122()); - - // return the hash - return hash.result(); + auto hashResult { hash.result() }; + return QByteArray((const char*) hashResult.data(), (int) hashResult.size()); } void NLPacket::writeTypeAndVersion() { @@ -214,14 +212,14 @@ void NLPacket::writeSourceID(LocalID sourceID) const { _sourceID = sourceID; } -void NLPacket::writeVerificationHashGivenSecret(const QUuid& connectionSecret) const { +void NLPacket::writeVerificationHash(HMACAuth& hmacAuth) const { Q_ASSERT(!PacketTypeEnum::getNonSourcedPackets().contains(_type) && !PacketTypeEnum::getNonVerifiedPackets().contains(_type)); auto offset = Packet::totalHeaderSize(isPartOfMessage()) + sizeof(PacketType) + sizeof(PacketVersion) + NUM_BYTES_LOCALID; - QByteArray verificationHash = hashForPacketAndSecret(*this, connectionSecret); + QByteArray verificationHash = hashForPacketAndHMAC(*this, hmacAuth); memcpy(_packet.get() + offset, verificationHash.data(), verificationHash.size()); } diff --git a/libraries/networking/src/NLPacket.h b/libraries/networking/src/NLPacket.h index 9b07bc6581..4103f2068e 100644 --- a/libraries/networking/src/NLPacket.h +++ b/libraries/networking/src/NLPacket.h @@ -18,6 +18,8 @@ #include "udt/Packet.h" +class HMACAuth; + class NLPacket : public udt::Packet { Q_OBJECT public: @@ -69,7 +71,7 @@ public: static LocalID sourceIDInHeader(const udt::Packet& packet); static QByteArray verificationHashInHeader(const udt::Packet& packet); - static QByteArray hashForPacketAndSecret(const udt::Packet& packet, const QUuid& connectionSecret); + static QByteArray hashForPacketAndHMAC(const udt::Packet& packet, HMACAuth& hash); PacketType getType() const { return _type; } void setType(PacketType type); @@ -78,9 +80,9 @@ public: void setVersion(PacketVersion version); LocalID getSourceID() const { return _sourceID; } - + void writeSourceID(LocalID sourceID) const; - void writeVerificationHashGivenSecret(const QUuid& connectionSecret) const; + void writeVerificationHash(HMACAuth& hmacAuth) const; protected: diff --git a/libraries/networking/src/Node.cpp b/libraries/networking/src/Node.cpp index 73b7c44e7e..7979b36e30 100644 --- a/libraries/networking/src/Node.cpp +++ b/libraries/networking/src/Node.cpp @@ -86,10 +86,10 @@ NodeType_t NodeType::fromString(QString type) { Node::Node(const QUuid& uuid, NodeType_t type, const HifiSockAddr& publicSocket, - const HifiSockAddr& localSocket, QObject* parent) : + const HifiSockAddr& localSocket, QObject* parent) : NetworkPeer(uuid, publicSocket, localSocket, parent), _type(type), - _pingMs(-1), // "Uninitialized" + _authenticateHash(new HMACAuth), _pingMs(-1), // "Uninitialized" _clockSkewUsec(0), _mutex(), _clockSkewMovingPercentile(30, 0.8f) // moving 80th percentile of 30 samples @@ -108,6 +108,7 @@ void Node::setType(char type) { _symmetricSocket.setObjectName(typeString); } + void Node::updateClockSkewUsec(qint64 clockSkewSample) { _clockSkewMovingPercentile.updatePercentile(clockSkewSample); _clockSkewUsec = (quint64)_clockSkewMovingPercentile.getValueAtPercentile(); @@ -194,3 +195,12 @@ QDebug operator<<(QDebug debug, const Node& node) { debug.nospace() << node.getPublicSocket() << "/" << node.getLocalSocket(); return debug.nospace(); } + +void Node::setConnectionSecret(const QUuid& connectionSecret) { + if (_connectionSecret == connectionSecret) { + return; + } + + _connectionSecret = connectionSecret; + _authenticateHash->setKey(_connectionSecret); +} diff --git a/libraries/networking/src/Node.h b/libraries/networking/src/Node.h index 93b6a649d4..5b3b559582 100644 --- a/libraries/networking/src/Node.h +++ b/libraries/networking/src/Node.h @@ -33,6 +33,7 @@ #include "SimpleMovingAverage.h" #include "MovingPercentile.h" #include "NodePermissions.h" +#include "HMACAuth.h" class Node : public NetworkPeer { Q_OBJECT @@ -55,7 +56,8 @@ public: void setIsUpstream(bool isUpstream) { _isUpstream = isUpstream; } const QUuid& getConnectionSecret() const { return _connectionSecret; } - void setConnectionSecret(const QUuid& connectionSecret) { _connectionSecret = connectionSecret; } + void setConnectionSecret(const QUuid& connectionSecret); + HMACAuth& getAuthenticateHash() const { return *_authenticateHash; } NodeData* getLinkedData() const { return _linkedData.get(); } void setLinkedData(std::unique_ptr linkedData) { _linkedData = std::move(linkedData); } @@ -97,6 +99,7 @@ private: NodeType_t _type; QUuid _connectionSecret; + std::unique_ptr _authenticateHash; std::unique_ptr _linkedData; bool _isReplicated { false }; int _pingMs; diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index 98b0e1d892..db466a0403 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -91,7 +91,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::Ping: return static_cast(PingVersion::IncludeConnectionID); default: - return 20; + return 19; } } From 4fa11f116fb17847c5bdcee167301f16d94cc2ce Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 25 Apr 2018 14:41:16 -0700 Subject: [PATCH 3/8] Add & use HMACAuth method that calculates hash atomically To avoid issues with race conditions involving separate addData() and result() calls use a method that does both in the lock. --- libraries/networking/src/HMACAuth.cpp | 25 +++++++++++++++++++++---- libraries/networking/src/HMACAuth.h | 5 +++-- libraries/networking/src/NLPacket.cpp | 7 ++++--- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/libraries/networking/src/HMACAuth.cpp b/libraries/networking/src/HMACAuth.cpp index e276e64b40..8aa2fea6a9 100644 --- a/libraries/networking/src/HMACAuth.cpp +++ b/libraries/networking/src/HMACAuth.cpp @@ -68,7 +68,7 @@ bool HMACAuth::setKey(const char* keyValue, int keyLen) { return false; } - //QMutexLocker lock(&_lock); + QMutexLocker lock(&_lock); return (bool) HMAC_Init_ex(_hmacContext, keyValue, keyLen, sslStruct, nullptr); } @@ -78,14 +78,14 @@ bool HMACAuth::setKey(const QUuid& uidKey) { } bool HMACAuth::addData(const char* data, int dataLen) { - //QMutexLocker lock(&_lock); + QMutexLocker lock(&_lock); return (bool) HMAC_Update(_hmacContext, reinterpret_cast(data), dataLen); } HMACAuth::HMACHash HMACAuth::result() { HMACHash hashValue(EVP_MAX_MD_SIZE); - unsigned int hashLen; - //QMutexLocker lock(&_lock); + unsigned int hashLen; + QMutexLocker lock(&_lock); if (HMAC_Final(_hmacContext, &hashValue[0], &hashLen)) { hashValue.resize((size_t)hashLen); } @@ -93,3 +93,20 @@ HMACAuth::HMACHash HMACAuth::result() { HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); return hashValue; } + +bool HMACAuth::calculateHash(HMACHash& hashResult, const char* data, int dataLen) { + QMutexLocker lock(&_lock); + if (!HMAC_Update(_hmacContext, reinterpret_cast(data), dataLen)) { + return false; + } + + hashResult.resize(EVP_MAX_MD_SIZE); + unsigned int hashLen; + if (HMAC_Final(_hmacContext, &hashResult[0], &hashLen)) { + hashResult.resize((size_t)hashLen); + // Clear state for possible reuse. + HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); + return true; + } + return false; +} diff --git a/libraries/networking/src/HMACAuth.h b/libraries/networking/src/HMACAuth.h index ba1ec78214..ec06bfbda2 100644 --- a/libraries/networking/src/HMACAuth.h +++ b/libraries/networking/src/HMACAuth.h @@ -26,10 +26,11 @@ public: explicit HMACAuth(AuthMethod authMethod = MD5); ~HMACAuth(); - QMutex& getLock() { return _lock; } - bool setKey(const char* keyValue, int keyLen); bool setKey(const QUuid& uidKey); + // Calculate complete hash in one. + bool calculateHash(HMACHash& hashResult, const char* data, int dataLen); + // Append data to be hashed. bool addData(const char* data, int dataLen); HMACHash result(); diff --git a/libraries/networking/src/NLPacket.cpp b/libraries/networking/src/NLPacket.cpp index 09a7b78840..f946e97bf4 100644 --- a/libraries/networking/src/NLPacket.cpp +++ b/libraries/networking/src/NLPacket.cpp @@ -157,9 +157,10 @@ QByteArray NLPacket::hashForPacketAndHMAC(const udt::Packet& packet, HMACAuth& h + NUM_BYTES_LOCALID + NUM_BYTES_MD5_HASH; // add the packet payload and the connection UUID - QMutexLocker hashLock(&hash.getLock()); - hash.addData(packet.getData() + offset, packet.getDataSize() - offset); - auto hashResult { hash.result() }; + HMACAuth::HMACHash hashResult; + if (!hash.calculateHash(hashResult, packet.getData() + offset, packet.getDataSize() - offset)) { + return QByteArray(); + } return QByteArray((const char*) hashResult.data(), (int) hashResult.size()); } From 66bd424ae4002a75cd3003f4e97df1322dd2143f Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 25 Apr 2018 14:51:11 -0700 Subject: [PATCH 4/8] Bump default packet version for new HMAC --- libraries/networking/src/udt/PacketHeaders.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index db466a0403..98b0e1d892 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -91,7 +91,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::Ping: return static_cast(PingVersion::IncludeConnectionID); default: - return 19; + return 20; } } From 9ef56c44a3da38161741cf4dc62f49e786c8e5b1 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 25 Apr 2018 17:08:08 -0700 Subject: [PATCH 5/8] 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; From 00d2f9494bbc493a8efad13ea2cbe29a1983339c Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Fri, 4 May 2018 15:07:20 -0700 Subject: [PATCH 6/8] Change HMACAuth::calculateHash to just call existing methods --- libraries/networking/src/HMACAuth.cpp | 17 ++++------------- libraries/networking/src/HMACAuth.h | 2 +- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/libraries/networking/src/HMACAuth.cpp b/libraries/networking/src/HMACAuth.cpp index 8e57978b5a..515af9a070 100644 --- a/libraries/networking/src/HMACAuth.cpp +++ b/libraries/networking/src/HMACAuth.cpp @@ -106,21 +106,12 @@ 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"; + if (!addData(data, dataLen)) { + qCWarning(networking) << "Error occured calling HMACAuth::addData()"; assert(false); return false; } - hashResult.resize(EVP_MAX_MD_SIZE); - unsigned int hashLen; - if (HMAC_Final(_hmacContext, &hashResult[0], &hashLen)) { - hashResult.resize((size_t)hashLen); - // Clear state for possible reuse. - HMAC_Init_ex(_hmacContext, nullptr, 0, nullptr, nullptr); - return true; - } - qCWarning(networking) << "Error occured calling HMAC_Final"; - assert(false); - return false; + hashResult = result(); + return true; } diff --git a/libraries/networking/src/HMACAuth.h b/libraries/networking/src/HMACAuth.h index ec06bfbda2..59870bb7b6 100644 --- a/libraries/networking/src/HMACAuth.h +++ b/libraries/networking/src/HMACAuth.h @@ -35,7 +35,7 @@ public: HMACHash result(); private: - QMutex _lock; + QMutex _lock { QMutex::Recursive }; struct hmac_ctx_st* _hmacContext; AuthMethod _authMethod; }; From 0349847857fb2393c0894426b22011a60c2475a5 Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Tue, 8 May 2018 17:46:25 -0700 Subject: [PATCH 7/8] Add comment re threading issues --- libraries/networking/src/HMACAuth.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/HMACAuth.h b/libraries/networking/src/HMACAuth.h index 59870bb7b6..1346bdee17 100644 --- a/libraries/networking/src/HMACAuth.h +++ b/libraries/networking/src/HMACAuth.h @@ -30,8 +30,12 @@ public: bool setKey(const QUuid& uidKey); // Calculate complete hash in one. bool calculateHash(HMACHash& hashResult, const char* data, int dataLen); - // Append data to be hashed. + + // Append to data to be hashed. bool addData(const char* data, int dataLen); + // Get the resulting hash from calls to addData(). + // Note that only one hash may be calculated at a time for each + // HMACAuth instance if this interface is used. HMACHash result(); private: From 9b6225db40bfa1f69707e2a1507a007f2eed9b4f Mon Sep 17 00:00:00 2001 From: Simon Walton Date: Wed, 9 May 2018 14:29:10 -0700 Subject: [PATCH 8/8] Bump default packet version 20 -> 21 --- libraries/networking/src/udt/PacketHeaders.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index 98b0e1d892..e7a793d267 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -91,7 +91,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::Ping: return static_cast(PingVersion::IncludeConnectionID); default: - return 20; + return 21; } }