From d36fd6eaa396d4cdc163513e42d2d89545b16381 Mon Sep 17 00:00:00 2001 From: Clement Date: Thu, 25 Apr 2019 16:18:34 -0700 Subject: [PATCH] Ensure we don't read past the end of packet buffers --- .../src/avatars/AvatarMixerClientData.cpp | 30 ++++++++++-- libraries/avatars/src/AvatarHashMap.cpp | 46 ++++++++++++++++++- libraries/avatars/src/ClientTraitsHandler.cpp | 17 ++++++- libraries/networking/src/ReceivedMessage.cpp | 25 ++++++---- 4 files changed, 102 insertions(+), 16 deletions(-) diff --git a/assignment-client/src/avatars/AvatarMixerClientData.cpp b/assignment-client/src/avatars/AvatarMixerClientData.cpp index 1b86e0dff2..678365d9e5 100644 --- a/assignment-client/src/avatars/AvatarMixerClientData.cpp +++ b/assignment-client/src/avatars/AvatarMixerClientData.cpp @@ -155,6 +155,12 @@ int AvatarMixerClientData::parseData(ReceivedMessage& message, const SlaveShared void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message, const SlaveSharedData& slaveSharedData, Node& sendingNode) { + // Trying to read more bytes than available, bail + if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitVersion))) { + qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr(); + return; + } + // pull the trait version from the message AvatarTraits::TraitVersion packetTraitVersion; message.readPrimitive(&packetTraitVersion); @@ -164,10 +170,22 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message, while (message.getBytesLeftToRead() > 0) { // for each trait in the packet, apply it if the trait version is newer than what we have + // Trying to read more bytes than available, bail + if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitType))) { + qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr(); + return; + } + AvatarTraits::TraitType traitType; message.readPrimitive(&traitType); if (AvatarTraits::isSimpleTrait(traitType)) { + // Trying to read more bytes than available, bail + if (message.getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitWireSize))) { + qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr(); + return; + } + AvatarTraits::TraitWireSize traitSize; message.readPrimitive(&traitSize); @@ -190,13 +208,15 @@ void AvatarMixerClientData::processSetTraitsMessage(ReceivedMessage& message, message.seek(message.getPosition() + traitSize); } } 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; + // Trying to read more bytes than available, bail + if (message.getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID + + sizeof(AvatarTraits::TraitWireSize))) { + qWarning() << "Refusing to process malformed traits packet from" << message.getSenderSockAddr(); + return; } + AvatarTraits::TraitInstanceID instanceID = QUuid::fromRfc4122(message.readWithoutCopy(NUM_BYTES_RFC4122_UUID)); + AvatarTraits::TraitWireSize traitSize; message.readPrimitive(&traitSize); diff --git a/libraries/avatars/src/AvatarHashMap.cpp b/libraries/avatars/src/AvatarHashMap.cpp index a8f1448a3f..29a40c5b6b 100644 --- a/libraries/avatars/src/AvatarHashMap.cpp +++ b/libraries/avatars/src/AvatarHashMap.cpp @@ -332,6 +332,12 @@ void AvatarHashMap::processAvatarIdentityPacket(QSharedPointer void AvatarHashMap::processBulkAvatarTraits(QSharedPointer message, SharedNodePointer sendingNode) { AvatarTraits::TraitMessageSequence seq; + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < (qint64)sizeof(AvatarTraits::TraitMessageSequence)) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + message->readPrimitive(&seq); auto traitsAckPacket = NLPacket::create(PacketType::BulkAvatarTraitsAck, sizeof(AvatarTraits::TraitMessageSequence), true); @@ -344,7 +350,14 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess nodeList->sendPacket(std::move(traitsAckPacket), *avatarMixer); } - while (message->getBytesLeftToRead()) { + while (message->getBytesLeftToRead() > 0) { + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID + + sizeof(AvatarTraits::TraitType))) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + // read the avatar ID to figure out which avatar this is for auto avatarID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); @@ -360,6 +373,12 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess auto& lastProcessedVersions = _processedTraitVersions[avatarID]; while (traitType != AvatarTraits::NullTrait && message->getBytesLeftToRead() > 0) { + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitVersion))) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + AvatarTraits::TraitVersion packetTraitVersion; message->readPrimitive(&packetTraitVersion); @@ -367,8 +386,20 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess bool skipBinaryTrait = false; if (AvatarTraits::isSimpleTrait(traitType)) { + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitWireSize))) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + message->readPrimitive(&traitBinarySize); + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < traitBinarySize) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + // check if this trait version is newer than what we already have for this avatar if (packetTraitVersion > lastProcessedVersions[traitType]) { auto traitData = message->read(traitBinarySize); @@ -379,11 +410,24 @@ void AvatarHashMap::processBulkAvatarTraits(QSharedPointer mess skipBinaryTrait = true; } } else { + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < qint64(NUM_BYTES_RFC4122_UUID + + sizeof(AvatarTraits::TraitWireSize))) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + AvatarTraits::TraitInstanceID traitInstanceID = QUuid::fromRfc4122(message->readWithoutCopy(NUM_BYTES_RFC4122_UUID)); message->readPrimitive(&traitBinarySize); + // Trying to read more bytes than available, bail + if (traitBinarySize < -1 || message->getBytesLeftToRead() < traitBinarySize) { + qWarning() << "Malformed bulk trait packet, bailling"; + return; + } + auto& processedInstanceVersion = lastProcessedVersions.getInstanceValueRef(traitType, traitInstanceID); if (packetTraitVersion > processedInstanceVersion) { if (traitBinarySize == AvatarTraits::DELETED_TRAIT_SIZE) { diff --git a/libraries/avatars/src/ClientTraitsHandler.cpp b/libraries/avatars/src/ClientTraitsHandler.cpp index f6bd66e89a..4f913de484 100644 --- a/libraries/avatars/src/ClientTraitsHandler.cpp +++ b/libraries/avatars/src/ClientTraitsHandler.cpp @@ -147,7 +147,16 @@ int ClientTraitsHandler::sendChangedTraitsToMixer() { void ClientTraitsHandler::processTraitOverride(QSharedPointer message, SharedNodePointer sendingNode) { if (sendingNode->getType() == NodeType::AvatarMixer) { Lock lock(_traitLock); - while (message->getBytesLeftToRead()) { + + while (message->getBytesLeftToRead() > 0) { + // Trying to read more bytes than available, bail + if (message->getBytesLeftToRead() < qint64(sizeof(AvatarTraits::TraitType) + + sizeof(AvatarTraits::TraitVersion) + + sizeof(AvatarTraits::TraitWireSize))) { + qWarning() << "Malformed trait override packet, bailling"; + return; + } + AvatarTraits::TraitType traitType; message->readPrimitive(&traitType); @@ -157,6 +166,12 @@ void ClientTraitsHandler::processTraitOverride(QSharedPointer m AvatarTraits::TraitWireSize traitBinarySize; message->readPrimitive(&traitBinarySize); + // Trying to read more bytes than available, bail + if (traitBinarySize < -1 || message->getBytesLeftToRead() < traitBinarySize) { + qWarning() << "Malformed trait override packet, bailling"; + return; + } + // only accept an override if this is for a trait type we override // and the version matches what we last sent for skeleton if (traitType == AvatarTraits::SkeletonModelURL diff --git a/libraries/networking/src/ReceivedMessage.cpp b/libraries/networking/src/ReceivedMessage.cpp index e1a036b041..e70301dab1 100644 --- a/libraries/networking/src/ReceivedMessage.cpp +++ b/libraries/networking/src/ReceivedMessage.cpp @@ -9,9 +9,10 @@ // See the accompanying file LICENSE or http://www.apache.org/licenses/LICENSE-2.0.html // - #include "ReceivedMessage.h" +#include + #include "QSharedPointer" int receivedMessageMetaTypeId = qRegisterMetaType("ReceivedMessage*"); @@ -83,20 +84,26 @@ void ReceivedMessage::appendPacket(NLPacket& packet) { } qint64 ReceivedMessage::peek(char* data, qint64 size) { - memcpy(data, _data.constData() + _position, size); - return size; + size_t bytesLeft = _data.size() - _position; + size_t sizeRead = std::min((size_t)size, bytesLeft); + memcpy(data, _data.constData() + _position, sizeRead); + return sizeRead; } qint64 ReceivedMessage::read(char* data, qint64 size) { - memcpy(data, _data.constData() + _position, size); - _position += size; - return size; + size_t bytesLeft = _data.size() - _position; + size_t sizeRead = std::min((size_t)size, bytesLeft); + memcpy(data, _data.constData() + _position, sizeRead); + _position += sizeRead; + return sizeRead; } qint64 ReceivedMessage::readHead(char* data, qint64 size) { - memcpy(data, _headData.constData() + _position, size); - _position += size; - return size; + size_t bytesLeft = _headData.size() - _position; + size_t sizeRead = std::min((size_t)size, bytesLeft); + memcpy(data, _headData.constData() + _position, sizeRead); + _position += sizeRead; + return sizeRead; } QByteArray ReceivedMessage::peek(qint64 size) {