From 88f76ac7601c3a5536e6638903ce0573af583435 Mon Sep 17 00:00:00 2001 From: Stephen Birarda Date: Thu, 4 Oct 2018 14:57:08 -0700 Subject: [PATCH] suppress traits larger than INT16_MAX bytes --- .../src/avatars/AvatarMixerClientData.cpp | 44 ++++++++++++++----- .../src/avatars/AvatarMixerSlave.cpp | 3 ++ libraries/avatars/src/AvatarData.cpp | 41 +++++++++++------ libraries/avatars/src/AvatarTraits.h | 1 + libraries/networking/src/NodeList.cpp | 1 - 5 files changed, 64 insertions(+), 26 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 187c9ed0f2..09bdfbc564 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -112,6 +112,11 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message, AvatarTraits::TraitWireSize traitSize; message.readPrimitive(&traitSize); + if (traitSize < -1 || traitSize > message.getBytesLeftToRead()) { + qWarning() << "Refusing to process simple trait of size" << traitSize << "from" << message.getSenderSockAddr(); + break; + } + if (packetTraitVersion > _lastReceivedTraitVersions[traitType]) { _avatar->processTrait(traitType, message.read(traitSize)); _lastReceivedTraitVersions[traitType] = packetTraitVersion; @@ -128,26 +133,41 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message, } else { AvatarTraits::TraitInstanceID instanceID = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID)); + if (message.getBytesLeftToRead() == 0) { + qWarning () << "Received an instanced trait with no size from" << message.getSenderSockAddr(); + break; + } + AvatarTraits::TraitWireSize traitSize; message.readPrimitive(&traitSize); - auto& instanceVersionRef = _lastReceivedTraitVersions.getInstanceValueRef(traitType, instanceID); + if (traitSize < -1 || traitSize > message.getBytesLeftToRead()) { + qWarning() << "Refusing to process instanced trait of size" << traitSize << "from" << message.getSenderSockAddr(); + break; + } - if (packetTraitVersion > instanceVersionRef) { - if (traitSize == AvatarTraits::DELETED_TRAIT_SIZE) { - _avatar->processDeletedTraitInstance(traitType, instanceID); + if (traitType == AvatarTraits::AvatarEntity) { + auto& instanceVersionRef = _lastReceivedTraitVersions.getInstanceValueRef(traitType, instanceID); - // to track a deleted instance but keep version information - // the avatar mixer uses the negative value of the sent version - instanceVersionRef = -packetTraitVersion; + if (packetTraitVersion > instanceVersionRef) { + if (traitSize == AvatarTraits::DELETED_TRAIT_SIZE) { + _avatar->processDeletedTraitInstance(traitType, instanceID); + + // to track a deleted instance but keep version information + // the avatar mixer uses the negative value of the sent version + instanceVersionRef = -packetTraitVersion; + } else { + _avatar->processTraitInstance(traitType, instanceID, message.read(traitSize)); + instanceVersionRef = packetTraitVersion; + } + + anyTraitsChanged = true; } else { - _avatar->processTraitInstance(traitType, instanceID, message.read(traitSize)); - instanceVersionRef = packetTraitVersion; + message.seek(message.getPosition() + traitSize); } - - anyTraitsChanged = true; } else { - message.seek(message.getPosition() + traitSize); + qWarning() << "Refusing to process traits packet with instanced trait of unprocessable type from" << message.getSenderSockAddr(); + break; } } } diff --git a/assignment-client/src/avatars/AvatarMixerSlave.cpp b/assignment-client/src/avatars/AvatarMixerSlave.cpp index 9c88580b99..b6ec006c39 100644 --- a/assignment-client/src/avatars/AvatarMixerSlave.cpp +++ b/assignment-client/src/avatars/AvatarMixerSlave.cpp @@ -152,6 +152,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis }); if (!isDeleted && (sentInstanceIt == sentIDValuePairs.end() || receivedVersion > sentInstanceIt->value)) { + // this instance version exists and has never been sent or is newer so we need to send it bytesWritten += sendingAvatar->packTraitInstance(traitType, instanceID, traitsPacketList, receivedVersion); @@ -161,6 +162,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis sentIDValuePairs.emplace_back(instanceID, receivedVersion); } } else if (isDeleted && sentInstanceIt != sentIDValuePairs.end() && absoluteReceivedVersion > sentInstanceIt->value) { + // this instance version was deleted and we haven't sent the delete to this client yet bytesWritten += AvatarTraits::packInstancedTraitDelete(traitType, instanceID, traitsPacketList, absoluteReceivedVersion); @@ -180,6 +182,7 @@ qint64 AvatarMixerSlave::addChangedTraitsToBulkPacket(AvatarMixerClientData* lis listeningNodeData->setLastOtherAvatarTraitsSendPoint(otherNodeLocalID, timeOfLastTraitsChange); } + return bytesWritten; } diff --git a/libraries/avatars/src/AvatarData.cpp b/libraries/avatars/src/AvatarData.cpp index 2168dff1f6..62eaee0f46 100644 --- a/libraries/avatars/src/AvatarData.cpp +++ b/libraries/avatars/src/AvatarData.cpp @@ -1802,15 +1802,24 @@ QUrl AvatarData::getWireSafeSkeletonModelURL() const { qint64 AvatarData::packTrait(AvatarTraits::TraitType traitType, ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion) { - qint64 bytesWritten = 0; - bytesWritten += destination.writePrimitive(traitType); - if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) { - bytesWritten += destination.writePrimitive(traitVersion); - } + qint64 bytesWritten = 0; if (traitType == AvatarTraits::SkeletonModelURL) { + QByteArray encodedSkeletonURL = getWireSafeSkeletonModelURL().toEncoded(); + + if (encodedSkeletonURL.size() > AvatarTraits::MAXIMUM_TRAIT_SIZE) { + qWarning() << "Refusing to pack simple trait" << traitType << "of size" << encodedSkeletonURL.size() + << "bytes since it exceeds the maximum size" << AvatarTraits::MAXIMUM_TRAIT_SIZE << "bytes"; + return 0; + } + + bytesWritten += destination.writePrimitive(traitType); + + if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) { + bytesWritten += destination.writePrimitive(traitVersion); + } AvatarTraits::TraitWireSize encodedURLSize = encodedSkeletonURL.size(); bytesWritten += destination.writePrimitive(encodedURLSize); @@ -1825,14 +1834,6 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr ExtendedIODevice& destination, AvatarTraits::TraitVersion traitVersion) { qint64 bytesWritten = 0; - bytesWritten += destination.writePrimitive(traitType); - - if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) { - bytesWritten += destination.writePrimitive(traitVersion); - } - - bytesWritten += destination.write(traitInstanceID.toRfc4122()); - if (traitType == AvatarTraits::AvatarEntity) { // grab a read lock on the avatar entities and check for entity data for the given ID QByteArray entityBinaryData; @@ -1843,6 +1844,20 @@ qint64 AvatarData::packTraitInstance(AvatarTraits::TraitType traitType, AvatarTr } }); + if (entityBinaryData.size() > AvatarTraits::MAXIMUM_TRAIT_SIZE) { + qWarning() << "Refusing to pack instanced trait" << traitType << "of size" << entityBinaryData.size() + << "bytes since it exceeds the maximum size " << AvatarTraits::MAXIMUM_TRAIT_SIZE << "bytes"; + return 0; + } + + bytesWritten += destination.writePrimitive(traitType); + + if (traitVersion > AvatarTraits::DEFAULT_TRAIT_VERSION) { + bytesWritten += destination.writePrimitive(traitVersion); + } + + bytesWritten += destination.write(traitInstanceID.toRfc4122()); + if (!entityBinaryData.isNull()) { AvatarTraits::TraitWireSize entityBinarySize = entityBinaryData.size(); diff --git a/libraries/avatars/src/AvatarTraits.h b/libraries/avatars/src/AvatarTraits.h index f0c807a432..5e28515d12 100644 --- a/libraries/avatars/src/AvatarTraits.h +++ b/libraries/avatars/src/AvatarTraits.h @@ -39,6 +39,7 @@ namespace AvatarTraits { using TraitWireSize = int16_t; const TraitWireSize DELETED_TRAIT_SIZE = -1; + const TraitWireSize MAXIMUM_TRAIT_SIZE = INT16_MAX; inline qint64 packInstancedTraitDelete(TraitType traitType, TraitInstanceID instanceID, ExtendedIODevice& destination, TraitVersion traitVersion = NULL_TRAIT_VERSION) { diff --git a/libraries/networking/src/NodeList.cpp b/libraries/networking/src/NodeList.cpp index e458ffab7e..917a4ca791 100644 --- a/libraries/networking/src/NodeList.cpp +++ b/libraries/networking/src/NodeList.cpp @@ -332,7 +332,6 @@ void NodeList::sendDomainServerCheckIn() { qCDebug(networking) << "Local domain-server port read from shared memory (or default) is" << domainPort; _domainHandler.setPort(domainPort); } - } // check if we're missing a keypair we need to verify ourselves with the domain-server