From 31e9f26772c5e3d9cde34624b32450863538b363 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 21 Dec 2017 11:34:03 -0800 Subject: [PATCH 1/8] fix use after free of pending message --- libraries/networking/src/udt/Connection.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/Connection.cpp b/libraries/networking/src/udt/Connection.cpp index 2f57523f79..77ed589e0b 100644 --- a/libraries/networking/src/udt/Connection.cpp +++ b/libraries/networking/src/udt/Connection.cpp @@ -191,6 +191,8 @@ void Connection::queueReceivedMessagePacket(std::unique_ptr packet) { pendingMessage.enqueuePacket(std::move(packet)); + bool processedLastOrOnly = false; + while (pendingMessage.hasAvailablePackets()) { auto packet = pendingMessage.removeNextPacket(); @@ -201,9 +203,13 @@ void Connection::queueReceivedMessagePacket(std::unique_ptr packet) { // if this was the last or only packet, then we can remove the pending message from our hash if (packetPosition == Packet::PacketPosition::LAST || packetPosition == Packet::PacketPosition::ONLY) { - _pendingReceivedMessages.erase(messageNumber); + processedLastOrOnly = true; } } + + if (processedLastOrOnly) { + _pendingReceivedMessages.erase(messageNumber); + } } void Connection::sync() { From 18993a8f72784415fc3e1f9129e38aaa58d6a573 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 21 Dec 2017 14:53:47 -0800 Subject: [PATCH 2/8] fix for AvatarData retrieval from SortableAvatar in PriorityQueue --- assignment-client/src/avatars/AvatarMixerSlave.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 47a81ba1fe..fb4b65726a 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -214,7 +214,7 @@ void AvatarMixerSlave::broadcastAvatarDataToAgent(const SharedNodePointer& node) uint64_t getTimestamp() const override { return _lastEncodeTime; } - const AvatarSharedPointer& getAvatar() const { return _avatar; } + AvatarSharedPointer getAvatar() const { return _avatar; } private: AvatarSharedPointer _avatar; @@ -326,7 +326,7 @@ void AvatarMixerSlave::broadcastAvatarDataToAgent(const SharedNodePointer& node) int remainingAvatars = (int)sortedAvatars.size(); while (!sortedAvatars.empty()) { - const auto& avatarData = sortedAvatars.top().getAvatar(); + const auto avatarData = sortedAvatars.top().getAvatar(); sortedAvatars.pop(); remainingAvatars--; From 7bf0cc2f63f2818ae115bcb8ff8104fed6ba8274 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 21 Dec 2017 15:17:18 -0800 Subject: [PATCH 3/8] cleanup other ref returns of shared pointers from sortables --- interface/src/avatar/AvatarManager.cpp | 6 +++--- libraries/entities-renderer/src/EntityTreeRenderer.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/interface/src/avatar/AvatarManager.cpp b/interface/src/avatar/AvatarManager.cpp index 8a294182bd..93caef555f 100644 --- a/interface/src/avatar/AvatarManager.cpp +++ b/interface/src/avatar/AvatarManager.cpp @@ -150,7 +150,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { glm::vec3 getPosition() const override { return _avatar->getWorldPosition(); } float getRadius() const override { return std::static_pointer_cast(_avatar)->getBoundingRadius(); } uint64_t getTimestamp() const override { return std::static_pointer_cast(_avatar)->getLastRenderUpdateTime(); } - const AvatarSharedPointer& getAvatar() const { return _avatar; } + AvatarSharedPointer getAvatar() const { return _avatar; } private: AvatarSharedPointer _avatar; }; @@ -185,7 +185,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { render::Transaction transaction; while (!sortedAvatars.empty()) { const SortableAvatar& sortData = sortedAvatars.top(); - const auto& avatar = std::static_pointer_cast(sortData.getAvatar()); + const auto avatar = std::static_pointer_cast(sortData.getAvatar()); bool ignoring = DependencyManager::get()->isPersonalMutingNode(avatar->getID()); if (ignoring) { @@ -239,7 +239,7 @@ void AvatarManager::updateOtherAvatars(float deltaTime) { sortedAvatars.pop(); while (inView && !sortedAvatars.empty()) { const SortableAvatar& newSortData = sortedAvatars.top(); - const auto& newAvatar = std::static_pointer_cast(newSortData.getAvatar()); + const auto newAvatar = std::static_pointer_cast(newSortData.getAvatar()); inView = newSortData.getPriority() > OUT_OF_VIEW_THRESHOLD; if (inView && newAvatar->hasNewJointData()) { numAVatarsNotUpdated++; diff --git a/libraries/entities-renderer/src/EntityTreeRenderer.cpp b/libraries/entities-renderer/src/EntityTreeRenderer.cpp index 5f7899ae74..a629b2046f 100644 --- a/libraries/entities-renderer/src/EntityTreeRenderer.cpp +++ b/libraries/entities-renderer/src/EntityTreeRenderer.cpp @@ -349,7 +349,7 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene float getRadius() const override { return 0.5f * _renderer->getEntity()->getQueryAACube().getScale(); } uint64_t getTimestamp() const override { return _renderer->getUpdateTime(); } - const EntityRendererPointer& getRenderer() const { return _renderer; } + EntityRendererPointer getRenderer() const { return _renderer; } private: EntityRendererPointer _renderer; }; @@ -382,7 +382,7 @@ void EntityTreeRenderer::updateChangedEntities(const render::ScenePointer& scene std::unordered_map::iterator itr; size_t numSorted = sortedRenderables.size(); while (!sortedRenderables.empty() && usecTimestampNow() < expiry) { - const EntityRendererPointer& renderable = sortedRenderables.top().getRenderer(); + const auto renderable = sortedRenderables.top().getRenderer(); renderable->updateInScene(scene, transaction); _renderablesToUpdate.erase(renderable->getEntity()->getID()); sortedRenderables.pop(); From 05b45f2e7ec94ed1ebdf5d216eef215acfbfe3fd Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 21 Dec 2017 15:40:08 -0800 Subject: [PATCH 4/8] adjust example and add clarifying comments to PrioritySortUtil --- libraries/shared/src/PrioritySortUtil.h | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/libraries/shared/src/PrioritySortUtil.h b/libraries/shared/src/PrioritySortUtil.h index dc6a877bb9..279fa42ea4 100644 --- a/libraries/shared/src/PrioritySortUtil.h +++ b/libraries/shared/src/PrioritySortUtil.h @@ -28,7 +28,7 @@ glm::vec3 getPosition() const override { return _thing->getPosition(); } float getRadius() const override { return 0.5f * _thing->getBoundingRadius(); } uint64_t getTimestamp() const override { return _thing->getLastTime(); } - const Thing& getThing() const { return _thing; } + Thing getThing() const { return _thing; } private: Thing _thing; }; @@ -43,6 +43,13 @@ (3) Loop over your priority queue and do timeboxed work: + NOTE: Be careful using references to members of instances of T from std::priority_queue. + Under the hood std::priority_queue may re-use instances of T. + For example, after a pop() or a push() the top T may have the same memory address + as the top T before the pop() or push() (but point to a swapped instance of T). + This causes a reference to member variable of T to point to a different value + when operations taken on std::priority_queue shuffle around the instances of T. + uint64_t cutoffTime = usecTimestampNow() + TIME_BUDGET; while (!sortedThings.empty()) { const Thing& thing = sortedThings.top(); From a2f2c233371f4ff997d332a8ae77cda60f1d8aaa Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Wed, 20 Dec 2017 15:40:10 -0800 Subject: [PATCH 5/8] Change three lines of code. Fix one thing. Break another. --- libraries/entities/src/EntityItemProperties.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 9f7ba1cc80..6e7fe9c1b2 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -2513,7 +2513,8 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte return false; } - const unsigned char* key = reinterpret_cast(publicKey.toUtf8().constData()); + auto keyByteArray = publicKey.toUtf8(); + auto key = keyByteArray.constData(); int keyLength = publicKey.length(); BIO *bio = BIO_new_mem_buf((void*)key, keyLength); @@ -2531,14 +2532,14 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte // ECSDA verification prototype: note that type is currently ignored // int ECDSA_verify(int type, const unsigned char *dgst, int dgstlen, // const unsigned char *sig, int siglen, EC_KEY *eckey); - bool answer = ECDSA_verify(0, + int answer = ECDSA_verify(0, digest, digestLength, signature, signatureLength, ec); long error = ERR_get_error(); - if (error != 0) { + if (error != 0 || answer == -1) { const char* error_str = ERR_error_string(error, NULL); qCWarning(entities) << "ERROR while verifying signature! EC error:" << error_str << "\nKey:" << publicKey << "\nutf8 Key Length:" << keyLength @@ -2552,7 +2553,7 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte if (evp_key) { EVP_PKEY_free(evp_key); } - return answer; + return (answer == 1); } else { if (bio) { BIO_free(bio); From 373ef6c6ce11dceccfee7ae425e4e47aa9eee305 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Thu, 21 Dec 2017 13:48:35 -0800 Subject: [PATCH 6/8] Progress? --- interface/src/commerce/Ledger.cpp | 2 +- interface/src/commerce/Wallet.cpp | 9 +++++---- libraries/entities/src/EntityItemProperties.cpp | 8 ++++++-- libraries/entities/src/EntityTree.cpp | 7 ++++--- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/interface/src/commerce/Ledger.cpp b/interface/src/commerce/Ledger.cpp index b0d293584c..33c23ccf2f 100644 --- a/interface/src/commerce/Ledger.cpp +++ b/interface/src/commerce/Ledger.cpp @@ -61,7 +61,7 @@ void Ledger::send(const QString& endpoint, const QString& success, const QString void Ledger::signedSend(const QString& propertyName, const QByteArray& text, const QString& key, const QString& endpoint, const QString& success, const QString& fail, const bool controlled_failure) { auto wallet = DependencyManager::get(); - QString signature = key.isEmpty() ? "" : wallet->signWithKey(text, key); + QString signature = wallet->signWithKey(text, key); QJsonObject request; request[propertyName] = QString(text); if (!controlled_failure) { diff --git a/interface/src/commerce/Wallet.cpp b/interface/src/commerce/Wallet.cpp index 69914e97a4..2f8a470b51 100644 --- a/interface/src/commerce/Wallet.cpp +++ b/interface/src/commerce/Wallet.cpp @@ -547,13 +547,16 @@ QStringList Wallet::listPublicKeys() { // the horror of code pages and so on (changing the bytes) by just returning a base64 // encoded string representing the signature (suitable for http, etc...) QString Wallet::signWithKey(const QByteArray& text, const QString& key) { - qCInfo(commerce) << "Signing text" << text << "with key" << key; EC_KEY* ecPrivateKey = NULL; + + auto keyFilePathString = keyFilePath().toStdString(); if ((ecPrivateKey = readPrivateKey(keyFilePath().toStdString().c_str()))) { unsigned char* sig = new unsigned char[ECDSA_size(ecPrivateKey)]; unsigned int signatureBytes = 0; + qCInfo(commerce) << "Signing text" << text << "with key at" << ecPrivateKey; + QByteArray hashedPlaintext = QCryptographicHash::hash(text, QCryptographicHash::Sha256); @@ -746,12 +749,10 @@ void Wallet::handleChallengeOwnershipPacket(QSharedPointer pack } EC_KEY_free(ec); - QByteArray ba = sig.toLocal8Bit(); - const char *sigChar = ba.data(); QByteArray textByteArray; if (status > -1) { - textByteArray = QByteArray(sigChar, (int) strlen(sigChar)); + textByteArray = sig.toUtf8(); } textByteArraySize = textByteArray.size(); int certIDSize = certID.size(); diff --git a/libraries/entities/src/EntityItemProperties.cpp b/libraries/entities/src/EntityItemProperties.cpp index 6e7fe9c1b2..9982f4d6a1 100644 --- a/libraries/entities/src/EntityItemProperties.cpp +++ b/libraries/entities/src/EntityItemProperties.cpp @@ -2540,11 +2540,15 @@ bool EntityItemProperties::verifySignature(const QString& publicKey, const QByte ec); long error = ERR_get_error(); if (error != 0 || answer == -1) { - const char* error_str = ERR_error_string(error, NULL); - qCWarning(entities) << "ERROR while verifying signature! EC error:" << error_str + qCWarning(entities) << "ERROR while verifying signature!" << "\nKey:" << publicKey << "\nutf8 Key Length:" << keyLength << "\nDigest:" << digest << "\nDigest Length:" << digestLength << "\nSignature:" << signature << "\nSignature Length:" << signatureLength; + while (error != 0) { + const char* error_str = ERR_error_string(error, NULL); + qCWarning(entities) << "EC error:" << error_str; + error = ERR_get_error(); + } } EC_KEY_free(ec); if (bio) { diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index e62399ce95..3d5bf1dd12 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -1189,13 +1189,14 @@ bool EntityTree::verifyNonce(const QString& certID, const QString& nonce, Entity key = sent.second; } - QString annotatedKey = "-----BEGIN PUBLIC KEY-----\n" + key.insert(64, "\n") + "\n-----END PUBLIC KEY-----"; - bool verificationSuccess = EntityItemProperties::verifySignature(annotatedKey.toUtf8(), actualNonce.toUtf8(), nonce.toUtf8()); + QString annotatedKey = "-----BEGIN PUBLIC KEY-----\n" + key.insert(64, "\n") + "\n-----END PUBLIC KEY-----\n"; + QByteArray hashedActualNonce = QCryptographicHash::hash(QByteArray::fromBase64(actualNonce.toUtf8()), QCryptographicHash::Sha256); + bool verificationSuccess = EntityItemProperties::verifySignature(annotatedKey.toUtf8(), hashedActualNonce, QByteArray::fromBase64(nonce.toUtf8())); if (verificationSuccess) { qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "succeeded."; } else { - qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed for nonce" << actualNonce << "key" << key << "signature" << nonce; + qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed.\nHashed actual nonce (digest):" << hashedActualNonce << "\nSent nonce (signature)" << nonce << "\nKey" << key; } return verificationSuccess; From 07a1cf434a2484b2493be7ed1d64932642119047 Mon Sep 17 00:00:00 2001 From: Zach Fox Date: Thu, 21 Dec 2017 15:01:49 -0800 Subject: [PATCH 7/8] IT'S WORKING!!! --- interface/src/commerce/Ledger.cpp | 2 +- interface/src/commerce/Wallet.cpp | 2 +- libraries/entities/src/EntityTree.cpp | 5 +++-- libraries/networking/src/udt/PacketHeaders.cpp | 2 +- libraries/networking/src/udt/PacketHeaders.h | 3 ++- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/interface/src/commerce/Ledger.cpp b/interface/src/commerce/Ledger.cpp index 33c23ccf2f..2865392e9a 100644 --- a/interface/src/commerce/Ledger.cpp +++ b/interface/src/commerce/Ledger.cpp @@ -61,7 +61,7 @@ void Ledger::send(const QString& endpoint, const QString& success, const QString void Ledger::signedSend(const QString& propertyName, const QByteArray& text, const QString& key, const QString& endpoint, const QString& success, const QString& fail, const bool controlled_failure) { auto wallet = DependencyManager::get(); - QString signature = wallet->signWithKey(text, key); + QString signature = wallet->signWithKey(text, key); QJsonObject request; request[propertyName] = QString(text); if (!controlled_failure) { diff --git a/interface/src/commerce/Wallet.cpp b/interface/src/commerce/Wallet.cpp index 2f8a470b51..602684756f 100644 --- a/interface/src/commerce/Wallet.cpp +++ b/interface/src/commerce/Wallet.cpp @@ -555,7 +555,7 @@ QString Wallet::signWithKey(const QByteArray& text, const QString& key) { unsigned int signatureBytes = 0; - qCInfo(commerce) << "Signing text" << text << "with key at" << ecPrivateKey; + qCInfo(commerce) << "Hashing and signing plaintext" << text << "with key at address" << ecPrivateKey; QByteArray hashedPlaintext = QCryptographicHash::hash(text, QCryptographicHash::Sha256); diff --git a/libraries/entities/src/EntityTree.cpp b/libraries/entities/src/EntityTree.cpp index 3d5bf1dd12..5afea35d1c 100644 --- a/libraries/entities/src/EntityTree.cpp +++ b/libraries/entities/src/EntityTree.cpp @@ -1190,13 +1190,14 @@ bool EntityTree::verifyNonce(const QString& certID, const QString& nonce, Entity } QString annotatedKey = "-----BEGIN PUBLIC KEY-----\n" + key.insert(64, "\n") + "\n-----END PUBLIC KEY-----\n"; - QByteArray hashedActualNonce = QCryptographicHash::hash(QByteArray::fromBase64(actualNonce.toUtf8()), QCryptographicHash::Sha256); + QByteArray hashedActualNonce = QCryptographicHash::hash(QByteArray(actualNonce.toUtf8()), QCryptographicHash::Sha256); bool verificationSuccess = EntityItemProperties::verifySignature(annotatedKey.toUtf8(), hashedActualNonce, QByteArray::fromBase64(nonce.toUtf8())); if (verificationSuccess) { qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "succeeded."; } else { - qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed.\nHashed actual nonce (digest):" << hashedActualNonce << "\nSent nonce (signature)" << nonce << "\nKey" << key; + qCDebug(entities) << "Ownership challenge for Cert ID" << certID << "failed. Actual nonce:" << actualNonce << + "\nHashed actual nonce (digest):" << hashedActualNonce << "\nSent nonce (signature)" << nonce << "\nKey" << key; } return verificationSuccess; diff --git a/libraries/networking/src/udt/PacketHeaders.cpp b/libraries/networking/src/udt/PacketHeaders.cpp index 207ddf6bbb..0a75e8c31b 100644 --- a/libraries/networking/src/udt/PacketHeaders.cpp +++ b/libraries/networking/src/udt/PacketHeaders.cpp @@ -30,7 +30,7 @@ PacketVersion versionForPacketType(PacketType packetType) { case PacketType::EntityEdit: case PacketType::EntityData: case PacketType::EntityPhysics: - return static_cast(EntityVersion::HazeEffect); + return static_cast(EntityVersion::OwnershipChallengeFix); case PacketType::EntityQuery: return static_cast(EntityQueryPacketVersion::ConnectionIdentifier); diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index e5cb87c379..640d9962d2 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -199,7 +199,8 @@ QDebug operator<<(QDebug debug, const PacketType& type); enum class EntityVersion : PacketVersion { StrokeColorProperty = 77, HasDynamicOwnershipTests, - HazeEffect + HazeEffect, + OwnershipChallengeFix }; enum class EntityScriptCallMethodVersion : PacketVersion { From 95c7f876bfb4a76b983d927f4578a9b0e2de0644 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 21 Dec 2017 18:34:37 -0800 Subject: [PATCH 8/8] manually force OwnershipChallengeFix to avoid StaticCertJsonVersionOne --- libraries/networking/src/udt/PacketHeaders.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/networking/src/udt/PacketHeaders.h b/libraries/networking/src/udt/PacketHeaders.h index 640d9962d2..2c28f54014 100644 --- a/libraries/networking/src/udt/PacketHeaders.h +++ b/libraries/networking/src/udt/PacketHeaders.h @@ -197,7 +197,7 @@ uint qHash(const PacketType& key, uint seed); QDebug operator<<(QDebug debug, const PacketType& type); enum class EntityVersion : PacketVersion { - StrokeColorProperty = 77, + StrokeColorProperty = 0, HasDynamicOwnershipTests, HazeEffect, OwnershipChallengeFix